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 support for sui product features #501

Merged
merged 2 commits into from
Nov 9, 2018
Merged

Add support for sui product features #501

merged 2 commits into from
Nov 9, 2018

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Oct 25, 2018 •
edited by simaishi
Loading

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

PR 16212 added support for
Service UI permissions.

This PR adds support for the API to honor the SUI permissions.

Chris Hale and I tested on a live appliance.

Depends on: ManageIQ/manageiq#18175

@jvlcek
Copy link
Member Author

jvlcek commented Oct 25, 2018

@chalettu Please review.

@jvlcek
Copy link
Member Author

jvlcek commented Oct 25, 2018

@miq-bot add_label bug
@miq-bot add_label gaprindashvili/yes
@miq-bot add_label hammer/yes

@jvlcek
Copy link
Member Author

jvlcek commented Oct 25, 2018

@miq-bot assign @gtanzillo

@jvlcek
Copy link
Member Author

jvlcek commented Oct 25, 2018

@gtanzillo Please review

chalettu
chalettu approved these changes Oct 25, 2018
View reviewed changes
Copy link

@chalettu chalettu left a comment

Choose a reason for hiding this comment

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

I took a look over this list and these changes are in line with what we would expect from the product features tree that was added that has SUI specific RBAC controls . Great work !!

@JPrause
Copy link
Member

JPrause commented Oct 26, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Oct 29, 2018

@gtanzillo with that approval,..can this be merged.

gtanzillo
gtanzillo suggested changes Oct 30, 2018
View reviewed changes
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

@jvlcek I added some comments / question about the placement of some of the SUI specific product features. I think we should discuss with @chalettu when he returns

config/api.yml Outdated
:identifier: ops_settings
:identifier:
- ops_settings
- sui_services_show
Copy link
Member

Choose a reason for hiding this comment

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

@jvlcek, @chalettu Looking at this, it's not clear why a user should need to have sui_services_show and sui_vm_details_view to be able to request the Category collection. This change would no longer a user to request this collection when he previously could and the reason does not seems obvious.

config/api.yml Outdated
:identifier: ops_settings
:identifier:
- ops_settings
- sui_services_show
Copy link
Member

Choose a reason for hiding this comment

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

Same here

config/api.yml Outdated
:identifier: ops_settings
:identifier:
- ops_settings
- sui_services_show
Copy link
Member

Choose a reason for hiding this comment

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

And here, as well..

@JPrause
Copy link
Member

JPrause commented Nov 8, 2018

@jvlcek @chalettu was there anything left before this can be merged. This is needed for a blocker.

@jvlcek
Copy link
Member Author

jvlcek commented Nov 8, 2018

@JPrause Yes, this is more work left to do. We've discussed with @gtanzillo and have identified a slight change that is in the works. It should be ready soon.

@miq-bot
Copy link
Member

miq-bot commented Nov 8, 2018

Checked commits jvlcek/manageiq-api@e248678~...b0d59c9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

config/api.yml

  • 💣 💥 🔥 🚒 - Line 2827, Col 21 - trailing-spaces - trailing spaces

@jvlcek
Copy link
Member Author

jvlcek commented Nov 8, 2018

@gtanzillo Thank you for the help with this. I believe my latest commit should address the concerns you raised.

This PR is dependent on PR ManageIQ/manageiq#18175 that @chalettu published today.

Please take a look at both and merge if appropriate.

Thank you, JoeV

@gtanzillo
Copy link
Member

Will merge once master is green

@gtanzillo gtanzillo closed this Nov 9, 2018
@gtanzillo gtanzillo reopened this Nov 9, 2018
@gtanzillo gtanzillo added this to the Sprint 99 Ending Nov 19, 2018 milestone Nov 9, 2018
@gtanzillo gtanzillo merged commit b7c13ca into ManageIQ:master Nov 9, 2018
simaishi pushed a commit that referenced this pull request Nov 12, 2018
@gtanzillo @simaishi
Merge pull request #501 from jvlcek/bz_1637552_apiyml
b1591ed 
Add support for sui product features

(cherry picked from commit b7c13ca)

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

Hammer backport details:

$ git log -1
commit b1591eda3bffd89c5bea7da209550a7dc438f0f4
Author: Gregg Tanzillo <[email protected]>
Date:   Fri Nov 9 12:09:19 2018 -0500

    Merge pull request #501 from jvlcek/bz_1637552_apiyml
    
    Add support for sui product features
    
    (cherry picked from commit b7c13ca93d66f44eaf3a6b77ee9ffce046589246)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1637552

@simaishi
Copy link
Contributor

@jvlcek There are multiple conflicts backporting to Gaprindashvili branch. Please create a separate PR. Thanks!

jvlcek pushed a commit to jvlcek/manageiq-api that referenced this pull request Nov 13, 2018
@gtanzillo @jvlcek
Merge pull request ManageIQ#501 from jvlcek/bz_1637552_apiyml
f7883d4 
Add support for sui product features

 (cherry picked from commit b7c13ca)

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

Backported to Gaprindashvili via #510

@jvlcek jvlcek mentioned this pull request Nov 19, 2018
Merged
@gmcculloug gmcculloug mentioned this pull request May 29, 2019
Merged
@jvlcek jvlcek deleted the bz_1637552_apiyml branch June 3, 2019 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtanzillo gtanzillo gtanzillo approved these changes

@chalettu chalettu chalettu approved these changes

Assignees

@gtanzillo gtanzillo

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

Successfully merging this pull request may close these issues.
6 participants
@jvlcek @JPrause @miq-bot @gtanzillo @simaishi @chalettu