-
Notifications
You must be signed in to change notification settings - Fork 897
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.
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
Floating IPs: Initial API #15524
Floating IPs: Initial API #15524
Conversation
@chessbyte, could you please assign it? |
@gildub looks promising - can you add some tests exercising the behavior you've added? |
@imtayadeway, I added the test file, thanks. |
config/api.yml
Outdated
:identifier: floating_ip | ||
:options: | ||
- :collection | ||
- :subcollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this needed as part of this PR? I suspect that you want to add future support for other collections to have floating ips - if that's the case I would add this line only when you do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, removed
config/api.yml
Outdated
:get: | ||
- :name: read | ||
:identifier: floating_ip_show | ||
:subcollection_actions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, with everything on this line and below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - just needs a couple things:
-
it looks like you're adding subcollection configuration that's not being used - can you either add tests for this or save that for a future PR?
-
Tests look good, but I thing we need a few more to cover the main paths. Something like:
-
GET /api/floating_ips (200 path)
-
GET /api/floating_ips (403 path)
-
GET /api/floating_ips/:id (200 path)
-
GET /api/floating_ips/:id (403 path)
-
POST /api/floating_ips (with action=query) (200 path)
-
POST /api/floating_ips (with action=query) (403 path)
Thanks!
|
@@ -119,6 +119,11 @@ def test_collection_bulk_query(collection, collection_url, klass, id = nil) | |||
test_collection_query(:flavors, flavors_url, Flavor) | |||
end | |||
|
|||
it "query FloatingIps" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is duplicating some of the tests in the other spec file. If this wasn't obvious then I'd suggest keeping the other ones, but it's up to you 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imtayadeway, sure although I believe all those test could go in the collection but that would be the purpose of another PR ;)
Initial API service definition for FloatingIP with show list
Checked commits https://github.com/gildub/manageiq/compare/93f99db3725c702ab34d2db3211c46fa49823b7c~...9aa40772a22df014bebe3a1c7bbf4376d475b8b3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks!
Thanks @gildub for the API enhancement. This LGTM!! 👍 |
Initial API service definition for FloatingIP with show list