Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow multiple role identifiers for cloud volume #299

Merged
merged 2 commits into from
Feb 1, 2018
Merged

Allow multiple role identifiers for cloud volume #299

merged 2 commits into from
Feb 1, 2018

Conversation

lpichler
Copy link
Contributor

pulled out from ManageIQ/manageiq#15249 - there is description
but the problem is solved only for storage managers.

@miq-bot add_label gaprindashvili/yes, blocker

links

https://bugzilla.redhat.com/show_bug.cgi?id=1538003

cc @gtanzillo

@miq-bot assign @abellotti

imtayadeway
imtayadeway suggested changes Jan 25, 2018
View reviewed changes
Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggests to me that there's a problem with our configuration - we have a concept of a role that we haven't identified, that is the parent of these two other identifiers. Could this be addressed by adding a parent identifier?

@lpichler lpichler force-pushed the allow_multiple_role_identifiers_for_cloud_volume branch from a0b59e7 to 7f188b8 Compare January 25, 2018 17:03
abellotti
abellotti reviewed Jan 26, 2018
View reviewed changes
spec/lib/api/api_config_spec.rb Outdated
let(:api_feature_identifiers) do
collection_settings.each_with_object(Set.new) do |(_, cfg), set|
set.add(cfg[:identifier]) if cfg[:identifier]
add_identifier_to_set(cfg[:identifier], set)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this line just be changed to:

Array(cfg[:identifier]).each { |id| set.add(id) }

Without introducing the add_identifier_to_set method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sure, it is much better.

spec/lib/api/api_config_spec.rb Outdated
@@ -29,7 +39,7 @@
cfg[action_type].each do |_, method_cfg|
method_cfg.each do |action_cfg|
next unless action_cfg[:identifier]
Array(action_cfg[:identifier]).each { |id| set.add(id) }
Array(action_cfg[:identifier]).each { |id| add_identifier_to_set(action_cfg[:identifier], set) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably don't need this as it already deals with the Array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed 👍

config/api.yml
:identifier: ems_infra_show_list
:identifier:
- ems_infra_show_list
- ems_block_storage_show_list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are Block Storage Managers providers ? i.e. subclass of ExtManagementSystem or Provider ? If so then this change is fine, i.e. User is authorized to see both variants as per the miq product feature selections and should be here since they are both accessible via /api/providers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, exactly, it is:

irb(main):013:0> ExtManagementSystem > ManageIQ::Providers::StorageManager
true

@lpichler
Copy link
Contributor Author

lpichler commented Jan 26, 2018 •
edited
Loading

This suggests to me that there's a problem with our configuration - we have a concept of a role that we haven't identified, that is the parent of these two other identifiers. Could this be addressed by adding a parent identifier?

Can show me an example of your idea?
Do you think for example to add children section to miq_product_features.yml ?

if so, it will cause that the added subtree will be displayed in the tree of product role feature when the role is edited and we don't need it.

@lpichler lpichler force-pushed the allow_multiple_role_identifiers_for_cloud_volume branch from 7f188b8 to a6af758 Compare January 26, 2018 14:43
@abellotti
Copy link
Member

Thanks @lpichler for updating and fixing this. LGTM!! 👍

@imtayadeway please take a 👀 Thanks!

@lpichler
Allow also storage manager features for /api/providers
a2a4e1c
@lpichler
Fix specs, as we can have array in :identifier
4202930 
to put each identifier from the array to
the result(returned in api_feature_identifiers)
@lpichler lpichler force-pushed the allow_multiple_role_identifiers_for_cloud_volume branch from a6af758 to 4202930 Compare January 29, 2018 12:49
@miq-bot
Copy link
Member

miq-bot commented Jan 29, 2018

Checked commits lpichler/manageiq-api@a2a4e1c~...4202930 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@lpichler
Copy link
Contributor Author

ping @imtayadeway

@imtayadeway
Copy link
Contributor

@lpichler you could possibly add it to the new API-only section?

if so, it will cause that the added subtree will be displayed in the tree of product role feature when the role is edited and we don't need it

To me, this is an issue with DRY - the knowledge of how to authorize a user for this endpoint is now duplicated. Any attempt to work around this is only going to add complexity to the system, and it's complexity that we impose upon the user - I would much prefer we figure out a way to DRY this up in our configuration.

@abellotti abellotti added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 1, 2018
@abellotti abellotti merged commit aca3a26 into ManageIQ:master Feb 1, 2018
@lpichler lpichler deleted the allow_multiple_role_identifiers_for_cloud_volume branch February 1, 2018 16:34
simaishi pushed a commit that referenced this pull request Mar 22, 2018
@abellotti @simaishi
Merge pull request #299 from lpichler/allow_multiple_role_identifiers…
1035373 
…_for_cloud_volume

Allow multiple role identifiers for cloud volume
(cherry picked from commit aca3a26)

https://bugzilla.redhat.com/show_bug.cgi?id=1552821
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 1035373aa2488d0cd94c0ca783f42b1b9eab2467
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Thu Feb 1 11:00:41 2018 -0500

    Merge pull request #299 from lpichler/allow_multiple_role_identifiers_for_cloud_volume
    
    Allow multiple role identifiers for cloud volume
    (cherry picked from commit aca3a26078bfe0875327bc989561d34a109f649f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552821

@simaishi simaishi mentioned this pull request Apr 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imtayadeway imtayadeway imtayadeway approved these changes

@abellotti abellotti abellotti approved these changes

Assignees

@abellotti abellotti

Labels
blocker gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lpichler @abellotti @miq-bot @imtayadeway @simaishi