Skip to content

Skip logic if service does not exist#379

Merged
UserNotFound merged 3 commits intomasterfrom
endpoint-list-logic
Apr 4, 2025
Merged

Skip logic if service does not exist#379
UserNotFound merged 3 commits intomasterfrom
endpoint-list-logic

Conversation

@michaelwang13
Copy link
Member

If a service doesn't exist, attempting to list vhosts for said service errors out.

Skip attempting to list/find vhosts for services that don't exist in endpoint:list.

eloisejones
eloisejones previously approved these changes Apr 3, 2025
Copy link

@eloisejones eloisejones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'm not offended if you wait for Alex :)

@UserNotFound
Copy link
Member

UserNotFound commented Apr 4, 2025

I'm going to join as an author (and duck out as reviewer) since I feel strongly about the solution, and Michael is OOO today.

I changed this from returning an empty result, to tell the customer the database isn't provisition. This is the same as the existing behavior for these commands in this scenario if the database is not properly provisioned with a sevice (either because it it's provisioned, failed to provision, deprovisioning, or failed to deprovision:

  • aptible endpoints:create --database foo
  • aptible endpoints:deprovision --database foo
  • aptible endpoints:database:modify foo ...

@UserNotFound UserNotFound requested review from eloisejones and removed request for UserNotFound April 4, 2025 19:04
Copy link

@eloisejones eloisejones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, otherwise lgtm. Thanks, Alex!

klass = resource.class

if klass == Aptible::Api::App
# We could also raise the error here, but it would technically be a

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding a test specifically for this case, if that's the behavior we expect? I didn't see a block for that case, but might have missed it.

Copy link
Member

@UserNotFound UserNotFound Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change anything about how commands for interacting with App endpoints worked here, and I didn't check for tests for that behavior but it probably doesn't exist because this is a nonsense/edge case:

EG, today: you can try to list endpoints for an app that has never been provisioned, and you just get an empty list back:

[~]$  aptible apps:create demo
App demo created!
[~]$ aptible endpoints:list --app demo

[~]$

What did you expect to get? The app is in a state where it's not possible to have endpoints, and any attempt to create or list isn't possible.

I'd argue this is better, and it's what i mentioned/showed in the comment (but didnt implement because it's technically a breaking change):

[~]$  aptible apps:create demo
App demo created!
[~]$ aptible endpoints:list --app demo
App is not provisioned
[~]$

@UserNotFound UserNotFound merged commit 94f85ef into master Apr 4, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants