-
Notifications
You must be signed in to change notification settings - Fork 991
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
Fixes #37824 - Hide taxonomies from parts of api documentation #10322
base: develop
Are you sure you want to change the base?
Fixes #37824 - Hide taxonomies from parts of api documentation #10322
Conversation
396604b
to
a6ec7b9
Compare
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.
Thanks, @adamlazik1, LGTM and works.
Although, I'd love to see an "automatic" solution instead of putting hide_taxonomy_options
wherever it makes sense, since it's prone to human eyes to overlook places. I've tried to come up with something easy, but failed. It looks it will take much more effort within Foreman project. I think we could add such feature in apipie-rails itself though...
For now we can deal with this, since it's easy and clean. For hammer though, we'd need to make some changes to be consistent with this. Currently it won't hide these params for these models.
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.
We have the taxonomy_scope
param group and when I look at the code that appears to be opt-in, but then there's also resource_description
that always declares them. Perhaps that part is wrong and we need to drop these lines:
foreman/app/controllers/api/v2/base_controller.rb
Lines 10 to 11 in e937bee
param :location_id, Integer, :required => false, :desc => N_("Set the current location context for the request") | |
param :organization_id, Integer, :required => false, :desc => N_("Set the current organization context for the request") |
I can try that and see what that does, but if there is potential for things breaking in plugins then I would rather not undertake this. |
@ekohl Seems like the solution would not be that simple. I removed the lines and ran this (hostgroups have the taxonomy_scope param group):
There is also a possibility of things breaking in plugins and I don't think this minor fix is worth updating all plugin repos. |
Some resources like user groups, external user groups, and architectures are not scoped by taxonomies, yet the the api endpoints associated with these resources accept the `organization-id` and `location-id` options. I didn't observe any effect of these options on the api call, except for when trying to create an external user group and providing either organization-id or location-id, which causes the action to fail with an error appearing in the logs: `undefined method external_usergroups for #<{Organization/Location}: ...` I have not, however, found any simple way of fixing this. All `Api::V2` controllers inherit from `Api::V2::BaseController`, where the taxonomy options are added through the `resource_description` method from Apipie. While this method can be overridden in child classes, there appears to be no way (at least I have not found such a way) of removing a parameter once it is added. The most correct solution would be of course to create a child class inheriting from BaseController, provide the resource description with taxonomy options there, and then have all taxonomy-scoped resource controllers inherit from it. The problem is that there are many plugins in which the controllers inherit from BaseController that would all need to be updated as well. I see too much potential for breaking because of a relatively harmless bug, so in my opinion the risk is not worth to fix the issue this way. Hence, I propose a partial solution. Hide the taxonomy options from the API documentation of the relevant resources. Hammer can also be updated to not display options with the `show => false` flag set. This would not completely solve the issue but in my opinion has the best effort/result/risk reduction ratio.
a6ec7b9
to
ec6120f
Compare
Some resources like user groups, external user groups, and architectures are not scoped by taxonomies, yet the the api endpoints associated with these resources accept the
organization-id
andlocation-id
options. I didn't observe any effect of these options on the api call, except for when trying to create an external user group and providing either organization-id or location-id, which causes the action to fail with an error appearing in the logs:undefined method external_usergroups for #<{Organization/Location}: ...
I have not, however, found any simple way of fixing this. All
Api::V2
controllers inherit fromApi::V2::BaseController
, where the taxonomy options are added through theresource_description
method from Apipie. While this method can be overridden in child classes, there appears to be no way (at least I have not found such a way) of removing a parameter once it is added.The most correct solution would be of course to create a child class inheriting from BaseController, provide the resource description with taxonomy options there, and then have all taxonomy-scoped resource controllers inherit from it. The problem is that there are many plugins in which the controllers inherit from BaseController that would all need to be updated as well. I see too much potential for breaking because of a relatively harmless bug, so in my opinion the risk is not worth to fix the issue this way.
Hence, I propose a partial solution. Hide the taxonomy options from the API documentation of the relevant resources. Hammer can also be updated to not display options with the
show => false
flag set.This would not completely solve the issue but in my opinion has the best effort/result/risk reduction ratio.