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
CLOUDSTACK-10044: Update role permission #2236
Conversation
Thanks @nvazquez |
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
hello @nvazquez |
@blueorangutan help |
@nvazquez I understand these words: "help", "hello", "thanks", "package", "test" Blessed contributors for kicking Trillian test jobs: ['rhtyd', 'nvazquez', 'PaulAngus', 'borisstoyanov', 'DaanHoogland'] |
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.
LGTM overall. Good work @nvazquez and @borisstoyanov - I've left few minor improvement suggestions.
Explicit trillian tests are not necessary as this is purely business layer changes, Travis should be able to verify that. However, let's run a single round if slots are available.
description = "The parent role permission uuid, use 0 to move this rule at the top of the list") | ||
private List<Long> rulePermissionOrder; | ||
|
||
@Parameter(name = ApiConstants.RULE_ID, type = CommandType.UUID, entityType = RolePermissionResponse.class, description = "Role permission rule id") |
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.
@nvazquez This is a new arg, can you add a since field to say '4.11'?
@Parameter(name = ApiConstants.RULE_ID, type = CommandType.UUID, entityType = RolePermissionResponse.class, description = "Role permission rule id") | ||
private Long ruleId; | ||
|
||
@Parameter(name = ApiConstants.PERMISSION, type = CommandType.STRING, description = "Rule permission, can be: allow or deny") |
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.
@nvazquez please add a since
to 4.11 here as well
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-946 |
95ae814
to
fe1da4d
Compare
Pushed changes @rhtyd. Thanks @borisstoyanov and @rhtyd. |
@blueorangutan package |
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-948 |
@blueorangutan test |
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-1354)
|
The test failures are unrelated to this PR and will be fixed/debugged next week after 4.9 freeze. |
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.
LGTM based on test results, code review and manual verification
JIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-10044
Introduction
This feature allows to change permission for existing role permissions, as those were static and could not be changed once created. It also provides the ability to change these permissions in the UI using a drop down menu for each permission rule, in which admin can select ‘Allow’ or ‘Deny’ permission.
Changes in the API:
This feature modifies behaviour of
updateRolePermission
API method:Changes in the UI:
Drop down menu added for role rule-permissions as seen in attached picture