Skip to content

Skip logic if service does not exist #379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
UserNotFound merged 3 commits into from
Apr 4, 2025
Merged

Skip logic if service does not exist #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e4e7f2
skip if service dne
michaelwang13 Apr 2, 2025
deecae9
Prefer to behave like other database commands.
UserNotFound Apr 4, 2025
703d8df
Tests
UserNotFound Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/aptible/cli/helpers/vhost.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,18 @@ def each_service(resource, &block)
klass = resource.class

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

@eloisejones eloisejones Apr 4, 2025

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 •
edited
Loading

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
[~]$

# breaking change:we currently return an empty list if the App is
# not provisioned.
# if resource.services.empty?
# raise Thor::Error, 'App is not provisioned'
# end
resource.each_service(&block)
elsif klass == Aptible::Api::Database
if resource.service.nil?
raise Thor::Error, 'Database is not provisioned'
end

[resource.service].each(&block)
else
raise "Unexpected resource: #{klass}"
Expand Down
32 changes: 31 additions & 1 deletion spec/aptible/cli/subcommands/endpoints_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,19 @@ def stub_options(**opts)
end

let!(:db) { Fabricate(:database, handle: 'mydb', account: a1) }
let!(:incomplete) do
Fabricate(:database, handle: 'incomplete',
status: 'provisioning',
account: a1)
end

before do
allow(Aptible::Api::Database)
.to receive(:all)
.with(token: token, href: '/databases?per_page=5000&no_embed=true')
.and_return([db])
.and_return([db, incomplete])
allow(db).to receive(:class).and_return(Aptible::Api::Database)
allow(incomplete).to receive(:class).and_return(Aptible::Api::Database)
stub_options
end

Expand All @@ -76,6 +82,12 @@ def stub_options(**opts)
.to raise_error(/could not find database some/im)
end

it 'returns an error if the database is not fully provisioned' do
stub_options(database: incomplete.handle)
expect { subject.send('endpoints:database:create', 'incomplete') }
.to raise_error(/database is not provisioned/im)
end

it 'fails if the DB is not in the account' do
stub_options(environment: 'bar')
expect { subject.send('endpoints:database:create', 'mydb') }
Expand Down Expand Up @@ -137,6 +149,12 @@ def stub_options(**opts)
expect { subject.send('endpoints:database:modify', v.external_host) }
.to raise_error(/conflicting.*no-ip-whitelist.*ip-whitelist/im)
end

it 'returns an error if the database is not fully provisioned' do
stub_options(database: incomplete.handle)
expect { subject.send('endpoints:database:modify', 'something') }
.to raise_error(/database is not provisioned/im)
end
end

describe 'endpoints:list' do
Expand All @@ -156,6 +174,12 @@ def stub_options(**opts)
expect(lines[0]).not_to eq("\n")
expect(lines[-1]).not_to eq("\n")
end

it 'returns an error if the database is not fully provisioned' do
stub_options(database: incomplete.handle)
expect { subject.send('endpoints:list') }
.to raise_error(/database is not provisioned/im)
end
end

describe 'endpoints:deprovison' do
Expand All @@ -176,6 +200,12 @@ def stub_options(**opts)
expect { subject.send('endpoints:deprovision', 'foo.io') }
.to raise_error(/endpoint.*foo\.io.*does not exist/im)
end

it 'returns an error if the database is not fully provisioned' do
stub_options(database: incomplete.handle)
expect { subject.send('endpoints:deprovision', 'foo') }
.to raise_error(/database is not provisioned/im)
end
end
end

Expand Down
8 changes: 5 additions & 3 deletions spec/fabricators/database_fabricator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@ def provisioned?

after_create do |database, transients|
database.account.databases << database
database.service = transients[:service] || Fabricate(
:service, app: nil, database: database
)
unless status == 'provisioning'
database.service = transients[:service] || Fabricate(
:service, app: nil, database: database
)
end
end
end