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

Add Container Administrator and Operator roles #14137

Merged
merged 1 commit into from
Apr 18, 2017
Merged

Add Container Administrator and Operator roles #14137

merged 1 commit into from
Apr 18, 2017

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Mar 2, 2017 •
edited
Loading

@zakiva
Copy link
Contributor Author

zakiva commented Mar 2, 2017

@miq-bot add_label providers/containers, enhancement, wip

cc @simon3z @zeari

@zakiva zakiva force-pushed the add_roles branch 2 times, most recently from 71682e9 to f3c4503 Compare March 9, 2017 15:40
@simon3z
Copy link
Contributor

simon3z commented Mar 9, 2017

@zakiva what's missing to remove the WIP name/label?

@miq-bot assign zakiva

@zakiva
Copy link
Contributor Author

zakiva commented Mar 9, 2017

@zakiva what's missing to remove the WIP name/label?

@simon3z I added all the required features for the new roles and updated the spec.
We are still missing the dashboards for the new groups, but I will add them in a separte PR.

@miq-bot remove_label wip

@zakiva zakiva changed the title [WIP] Add Container Administrator and Operator roles Add Container Administrator and Operator roles Mar 9, 2017
@miq-bot miq-bot removed the wip label Mar 9, 2017
@Loicavenel
Copy link

Reviewed it.. this can be merges

@chessbyte
Copy link
Member

chessbyte commented Mar 15, 2017 •
edited
Loading

@simon3z waiting on 👍 from you so core team can review

@chessbyte chessbyte requested a review from Fryguy March 15, 2017 16:20
simon3z
simon3z reviewed Mar 16, 2017
View reviewed changes
Copy link
Contributor

@simon3z simon3z left a comment

Choose a reason for hiding this comment

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

@simon3z waiting on from you so core team can review

Thanks @chessbyte I indeed wanted to review this thoroughly.
I added few comments on some things that are different from what @Loicavenel gave as directions in a db dump.

db/fixtures/miq_user_roles.yml
- miq_arbitration_rules
- blueprint
- redhat_access_insights_admin
- ems_container_ad_hoc_metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

ems_container_ad_hoc_metrics is missing from @Loicavenel's description but I think it makes sense here (it was introduced after Euwe).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct (it was also mentioned in the docs)

db/fixtures/miq_user_roles.yml
- container_dashboard
- instance_view
- vm_view
- ems_container_ad_hoc_metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

ems_container_ad_hoc_metrics is missing from @Loicavenel's description but I think it makes sense here (it was introduced after Euwe).

Copy link

Choose a reason for hiding this comment

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

@simon3z I am good here..

db/fixtures/miq_user_roles.yml
- container_control
- container_topology
- container_dashboard
- instance_view
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @Loicavenel also had image_view, should we add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think it was a mistake: he had image_view instead of instance_view

Copy link
Contributor

Choose a reason for hiding this comment

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

@zakiva @Loicavenel had both image_view and instance_view IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Loicavenel IIRC when we discussed it you confirmed that we need instance_view and not image_view, is that right?

Copy link

Choose a reason for hiding this comment

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

that is correct.. but don't get confused, I am talking about image view from Cloud not from container..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Loicavenel sure, for container image we have all the features above

db/fixtures/miq_user_roles.yml
- miq_report_control
- chargeback_reports
- my_settings
- tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

tasks is missing from @Loicavenel's description but I think it makes sense here.

@simon3z
Copy link
Contributor

simon3z commented Mar 23, 2017

@Loicavenel @zakiva are we sure this is final?

@zakiva
Copy link
Contributor Author

zakiva commented Mar 23, 2017

@Loicavenel @zakiva are we sure this is final?

I think we do, the only diff I see that I'm not sure if we discussed already is storage features for Container Administrator. I added them to the list according to the db dump, but I don't see them in the docs screenshots.
@Loicavenel are you OK with that?

stor

@Loicavenel
Copy link

@zakiva Storage is just for OpenStack here, we don't need it.. it should have been removed... Doc is good in this case.

@zakiva
Copy link
Contributor Author

zakiva commented Mar 23, 2017

@zakiva Storage is just for OpenStack here, we don't need it.. it should have been removed... Doc is good in this case.

@Loicavenel Thanks, I removed them. @simon3z It seems that we are good to go now.

@miq-bot
Copy link
Member

miq-bot commented Mar 23, 2017

Checked commit zakiva@ee2d846 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 👍

simon3z
simon3z approved these changes Mar 23, 2017
View reviewed changes
Copy link
Contributor

@simon3z simon3z left a comment

Choose a reason for hiding this comment

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

LGTM 👍
cc @blomquisg @chessbyte

@miq-bot assign blomquisg

@Loicavenel
Copy link

@blomquisg @Fryguy Can you guys review an merge.. thanks

@zakiva
Copy link
Contributor Author

zakiva commented Apr 18, 2017

@blomquisg Can you please take a look?

@simon3z
Copy link
Contributor

simon3z commented Apr 18, 2017

@miq-bot assign blomquisg

@blomquisg I assume we need to check what's going on with the bot 😊

@blomquisg blomquisg merged commit 943dd65 into ManageIQ:master Apr 18, 2017
@blomquisg
Copy link
Member

blomquisg commented Apr 18, 2017 •
edited
Loading

@miq-bot assign blomquisg

@blomquisg I assume we need to check what's going on with the bot 

Yeah, I just saw that! 👎

Thanks for trying 😄

simaishi pushed a commit that referenced this pull request Apr 19, 2017
@blomquisg @simaishi
Merge pull request #14137 from zakiva/add_roles
f1c7258 
Add Container Administrator and Operator roles
(cherry picked from commit 943dd65)

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

Fine backport details:

$ git log -1
commit f1c7258db00cd51268e52a2ebafb763582415ba8
Author: Greg Blomquist <[email protected]>
Date:   Tue Apr 18 10:38:13 2017 -0400

    Merge pull request #14137 from zakiva/add_roles
    
    Add Container Administrator and Operator roles
    (cherry picked from commit 943dd6554db099f55813856a8c2957af9a49c2d4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1443695
    https://bugzilla.redhat.com/show_bug.cgi?id=1443694

@chessbyte chessbyte added this to the Sprint 59 Ending Apr 24, 2017 milestone Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Loicavenel Loicavenel Loicavenel left review comments

@simon3z simon3z simon3z approved these changes

@Fryguy Fryguy Awaiting requested review from Fryguy Fryguy is a code owner

Assignees

@zakiva zakiva

Labels
enhancement fine/backported providers/containers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zakiva @simon3z @Loicavenel @chessbyte @miq-bot @blomquisg @simaishi