-
Notifications
You must be signed in to change notification settings - Fork 898
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
[api] Collections API for Cloud Volumes #14260
Conversation
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.
Hi @AparnaKarve looks good, couple of questions/comments.
FactoryGirl.create(:cloud_volume) | ||
test_collection_query(:cloud_volumes, cloud_volumes_url, CloudVolume) | ||
end | ||
|
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.
need to add the other test for bulk queries.
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.
also probably need just the resource gets tests, look at spec/requests/api/regions_spec.rb and create a spec/requests/api/cloud_volumes_spec.rb
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.
Will do.
config/api.yml
Outdated
:identifier: cloud_volume | ||
: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.
I didn't see who is declaring this as a 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.
is this needed a primary collection (i.e. I want to query all cloud volumes) or as a subcollection (I need to query cloud volumes pertaining to a CI) or both ?
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.
The current use case that I know needs it only as a primary collection. In that case, should I remove - :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.
If it's just as a primary collection then yes, subcollection should go. Thanks.
a5da93c
to
2df3a25
Compare
Hi @AparnaKarve I think this is good, we just need the it "bulk query CloudVolumes" test added to spec/requests/api/collections_spec.rb. Look at the later part of the file for the bulk queries. Thanks. |
2df3a25
to
0bbcd8a
Compare
Checked commits AparnaKarve/manageiq@1e6f2c9~...0bbcd8a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
Thanks @AparnaKarve for adding the test. LGTM!! 👍 |
Thanks for merging @abellotti |
Collections API for Cloud Volumes
/cc @abellotti @imtayadeway