-
Notifications
You must be signed in to change notification settings - Fork 3.3k
{Role} Remove MultiAPIAdaptor
#31454
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
Conversation
️✔️AzureCLI-FullTest
|
|
Hi @jiasli, |
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
| scope=scope or ('/subscriptions/' + definitions_client._config.subscription_id))) | ||
| role_dics = {i.id: worker.get_role_property(i, 'role_name') for i in role_defs} | ||
| for i in results: | ||
| if not i.get('roleDefinitionName'): |
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.
It is not possible for a roleAssignment object to have roleDefinitionName property: https://learn.microsoft.com/en-us/rest/api/authorization/role-assignments/list-for-scope, so this statement is unnecessary.
| role_dics = {i.id: worker.get_role_property(i, 'role_name') for i in role_defs} | ||
| for i in results: | ||
| if not i.get('roleDefinitionName'): | ||
| if role_dics.get(worker.get_role_property(i, 'roleDefinitionId')): |
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.
A RoleAssignment object must have roleDefinitionId property: https://learn.microsoft.com/en-us/rest/api/authorization/role-assignments/list-for-scope, so this statement is also unnecessary.
| principals = _get_object_stubs(graph_client, principal_ids) | ||
| principal_dics = {i[ID]: _get_displayable_name(i) for i in principals} | ||
|
|
||
| for i in [r for r in results if not r.get('principalName')]: |
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.
It is not possible for a roleAssignment object to have principalName property: https://learn.microsoft.com/en-us/rest/api/authorization/role-assignments/list-for-scope, so this statement is unnecessary.
| return get_member_group_func(identifier, body) | ||
|
|
||
|
|
||
| class RoleApiHelper: |
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.
Why not just rename the MultiAPIAdaptor toRoleApiHelper and update related code? custom.py has already been a bit large
evelyn-ys
left a comment
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.
Generally LGTM
Related command
az roleDescription
As Azure Stack support has been removed (#31307),
rolemodule'sMultiAPIAdaptoris no longer needed.This PR refactors
MultiAPIAdaptorintoRoleApiHelperand removes code for supporting old API versions.