New Feature: Enable/Disable Roles#9549
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9549 +/- ##
============================================
- Coverage 15.54% 15.54% -0.01%
- Complexity 12001 12003 +2
============================================
Files 5499 5502 +3
Lines 481744 481844 +100
Branches 60074 59309 -765
============================================
+ Hits 74897 74910 +13
- Misses 398560 398646 +86
- Partials 8287 8288 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
nvazquez
changed the title
nvazquez
requested review from
Pearl1594,
harikrishna-patnala,
kiranchavala,
shwstppr,
sureshanaparti,
vishesh92 and
weizhouapache
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
nvazquez
changed the title
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10701 |
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10708 |
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 10711 |
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
Both APIs basically do the same thing, they just toggle the same attribute. We should have a single API for this feature, we could call it |
|
@JoaoJandre thanks you are right, I even thought on including it as part of the updateRole API but kept it separate following the same pattern for accounts and users, but I'm fine with the unified approach as well |
|
[SF] Trillian test result (tid-11119)
|
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM
Before fix
(cmk) > list roles -h
listRoles: Lists dynamic roles in CloudStack
API Params Type Description
========== ==== ===========
id uuid List role by role ID.
keyword string List by keyword
name string List role by role name.
page integer
pagesize integer
type string List role by role type, valid options are
: Admin, ResourceAdmin, DomainAdmin, Use
r.
After fix
(cmk) > list roles -h
listRoles: Lists dynamic roles in CloudStack
API Params Type Description
========== ==== ===========
id uuid List role by role ID.
keyword string List by keyword
name string List role by role name.
page integer
pagesize integer
state string List role by role type status, valid opti
ons are: enabled, disabled
type string List role by role type, valid options are
: Admin, ResourceAdmin, DomainAdmin, Use
r.
(cmk) 🐱 > list roles name=test
{
"count": 1,
"role": [
{
"id": "176ecc6f-6627-4369-b752-f6d9f2d6bc56",
"isdefault": false,
"ispublic": true,
"name": "test",
"state": "disabled",
"type": "User"
}
]
}
(cmk) 🐱 > list roles name=test
{
"count": 1,
"role": [
{
"id": "176ecc6f-6627-4369-b752-f6d9f2d6bc56",
"isdefault": false,
"ispublic": true,
"name": "test",
"state": "enabled",
"type": "User"
}
]
}
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm, one silly question (that might point to a code smell but is by no means serious)
|
|
||
| public RoleVO() { | ||
| this.uuid = UUID.randomUUID().toString(); | ||
| this.state = State.ENABLED; |
There was a problem hiding this comment.
no constructor needed with the actual state (from the DB or UI)?
There was a problem hiding this comment.
@DaanHoogland if I understand your question correctly, this is needed as the state DB column needs a non-null value, so each time a new role is created it is enabled by default.
There was a problem hiding this comment.
well, I'd expect the DAO to instantiate a unmarshalled Role with state disabled if it was so when saved. But maybe accessors are enough for that.
|
@JoaoJandre can we address the unification of the enable/disable APIs for roles along with accounts and users in a separate PR? Possibly there could be other resources following the same pattern that we could investigate and unify |
@nvazquez I'm ok with the idea. However, we still do not have a proper mechanism for deprecating/changing API's names (see #8970). The discussion has died out a bit, but I think it's still worth it to try to create a consensus on how to do this, vote it and establish as a practice on the project. Going back to your idea, currently, as we do not have a clear way of changing API names or removing APIs, I would prefer if we introduced these APIs as one consolidated API from the beginning. |
@JoaoJandre I would prefer we do not introduce a new pattern for related entities unless we are sure we will address all of them. I think it is best to leave this PR as is, as it adheres better to related code/interfaces. I don't want to be strict in this perspect but @nvazquez ' argument makes sense to me. |
* New feature: Enable/Disable Roles * Fixes * Fix unit tests
5 participants
Description
This PR allows enabling/disabling roles. By default all the existing roles are set as 'enabled'.
New APIs created:
The listRoles API is extended to support a new parameter: state.
Fixes: #8460
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Create a role, verify role is enabled
Disable role
Add account, verify the disabled role is not listed
How did you try to break this feature and the system with this change?