Add cloud tenants to API #14731
Add cloud tenants to API #14731
Conversation
| module Subcollections | ||
| module CloudTenants | ||
| def cloud_tenants_query_resource(object) | ||
| object.respond_to?(:cloud_tenants) ? object.cloud_tenants : [] |
There was a problem hiding this comment.
Could you mix this into a collection that doesn't respond to :cloud_tenants? If not, does object.cloud_tenants suffice?
There was a problem hiding this comment.
I think the potential is definitely there if we discover we want to mix this into more places. I know that accommodating for cases like this is a pretty big deal in the UI, so using respond_to felt safer!
There was a problem hiding this comment.
I think if it's anticipating a need for it later, it would be better in the simpler form for now. If you can prove that it's needed in a later revision (through testing, which you'd need to provide anyway) you can always change that when it's needed. Thanks!
There was a problem hiding this comment.
Fair enough, I'll make the change!
config/api.yml
Outdated
| :subcollection_actions: | ||
| :get: | ||
| - :name: read | ||
| :identifier: cloud_tenant_show |
There was a problem hiding this comment.
Should this be cloud_tenant_show_list?
There was a problem hiding this comment.
Ahh, you're right - thanks!
There was a problem hiding this comment.
Turns out that this seems correct, at least for purposes of passing the providers_spec.rb. I'm following the existing pattern used by load balancers.
There was a problem hiding this comment.
Can you provide more details of the failure?
|
@imtayadeway Thanks for the review! I corrected the line in config/api.yml. Let me know if you feel strongly about respond_to! |
tzumainn
force-pushed
the
cloud-tenant-api
branch
from
1b30420
to
0aeb909
Compare
tzumainn
force-pushed
the
cloud-tenant-api
branch
from
0aeb909
to
b235985
Compare
|
@imtayadeway Thanks again! I've made a few more changes/comments. |
spec/requests/api/providers_spec.rb
Outdated
| end | ||
|
|
||
| it 'queries a single cloud tenant' do | ||
| api_basic_authorize subcollection_action_identifier(:providers, :cloud_tenants, :read, :get) |
There was a problem hiding this comment.
It is probably this one that is failing - it's pointing to the wrong identifier. This needs to be action_identifier(:cloud_tenants, :read, :subresource_actions, :get)
spec/requests/api/providers_spec.rb
Outdated
|
|
||
| it 'queries all cloud tenants' do | ||
| api_basic_authorize subcollection_action_identifier(:providers, :cloud_tenants, :read, :get) | ||
| expected = { |
There was a problem hiding this comment.
Nit, but can you group verification steps (this expected) together at the bottom?
|
@imtayadeway Ahhhh, I get it. Fix is as you predicted, and I've grouped the 'expect' code together as requested. Let me know if you'd like any other changes! |
| cloud_tenant = FactoryGirl.create(:cloud_tenant) | ||
| api_basic_authorize collection_action_identifier(:cloud_tenants, :read, :get) | ||
|
|
||
| expected = { |
There was a problem hiding this comment.
Small nit, but I think this would read better similarly grouped with the other verification steps
|
Great work, @tzumainn, this LGTM with just one nit above 👍 |
|
@imtayadeway whoops, makes sense to me - nit fixed. Thanks! |
|
Checked commits tzumainn/manageiq@023200e~...dece235 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
Thanks @tzumainn for the API Enhancement. |
|
@miq-bot add_label bug,fine/yes |
|
this is a dependency for ManageIQ/manageiq-ui-classic#1343 |
Add cloud tenants to API (cherry picked from commit 27e1ab0) https://bugzilla.redhat.com/show_bug.cgi?id=1458377
|
Fine backport details: |
Add cloud tenant show / show list, and subcollection to providers