Move authorization config for snapshotting to vms section #13761
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
imtayadeway
force-pushed
the
api/allow-collection-specific-subresource-config
branch
from
331dcf1
to
583e0a9
Compare
abellotti
reviewed
Feb 3, 2017
lib/api/collection_config.rb
Outdated
| @@ -53,12 +53,12 @@ def typed_collection_actions(collection_name, target) | |||
| self[collection_name]["#{target}_actions".to_sym] | |||
| end | |||
|
|
|||
| def typed_subcollection_actions(collection_name, subcollection_name) | |||
| self[collection_name]["#{subcollection_name}_subcollection_actions".to_sym] | |||
| def typed_subcollection_actions(collection_name, subcollection_name, target) | |||
There was a problem hiding this comment.
A lot of changes because of the required parameter, couldn't you make target optional and keep the signature backward compatible ?
Sometimes you have a subcollection whose role depends on the context of the "super"-collection, e.g. tagging vms requires a separate identifier to tagging hosts. This is fine and currently works, but we haven't implemented this correctly for subcollection members. This revision changes the way we look up subcollection role identifiers so that it knows the difference between subcollection and member actions.
This moves the snapshotting identifiers to the `:vms` section of the config, since they are wholly concerned with VMs. When instance snapshotting is implemented, they will need a whole new set of identifiers.
imtayadeway
force-pushed
the
api/allow-collection-specific-subresource-config
branch
from
583e0a9
to
f948fed
Compare
|
Checked commits imtayadeway/manageiq@ed579a7~...f948fed with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
Much nicer !! Thanks @imtayadeway for the update 🎵 |
abellotti
reviewed
Feb 8, 2017
| :identifier: vm_snapshot_delete | ||
| :delete: | ||
| - :name: delete | ||
| :identifier: vm_snapshot_delete |
There was a problem hiding this comment.
Can you double check that spec/requests/api/config_spec.rb's test "contains only valid miq_feature identifiers" also test subresource actions as you have above ? If not, could you create another PR to update that test accordingly ? Thanks 🎵
imtayadeway
deleted the
api/allow-collection-specific-subresource-config
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a two part move:
Sometimes you have a subcollection whose role depends on the context
of the "super"-collection, e.g. tagging vms requires a separate
identifier to tagging hosts. This is fine and currently works, but we
haven't implemented this correctly for subcollection members. This
revision changes the way we look up subcollection role identifiers so
that it knows the difference between subcollection and member actions.
This moves the snapshotting identifiers to the
:vmssection of theconfig, since they are wholly concerned with VMs. When instance
snapshotting is implemented, they will need a whole new set of
identifiers.
@miq-bot add-label api, refactoring, enhancement
@miq-bot assign @abellotti