Conversation
️✔️AzureCLI-FullTest
|
️✔️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>
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a breaking change in ARM's response message for existing role assignments by updating the error detection logic in az role assignment create command to use error codes instead of message content for implementing idempotence.
- Updates error detection from message-based to error code-based approach
- Maintains backward compatibility while fixing broken idempotence behavior
- Ensures reliable detection of existing role assignments regardless of message format changes
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| def _error_caused_by_role_assignment_exists(ex): | ||
| return getattr(ex, 'status_code', None) == 409 and 'role assignment already exists' in ex.message | ||
| return getattr(ex, 'status_code', None) == 409 and ex.error.code == 'RoleAssignmentExists' |
There was a problem hiding this comment.
The code assumes ex.error exists but doesn't handle the case where ex.error might be None. This could cause an AttributeError if the exception object doesn't have an error attribute. Consider using getattr(ex, 'error', None) and checking if it's not None before accessing the code attribute.
| return getattr(ex, 'status_code', None) == 409 and ex.error.code == 'RoleAssignmentExists' | |
| error = getattr(ex, 'error', None) | |
| return getattr(ex, 'status_code', None) == 409 and error is not None and getattr(error, 'code', None) == 'RoleAssignmentExists' |
There was a problem hiding this comment.
@zhoxing-ms, is it possible that ex.error is None?
There was a problem hiding this comment.
As far as I know, if the type of ex must be HttpResponseError, ex.error is guaranteed to exist, since many old codes are directly used ex.error.code for HttpResponseError. And because if ex has status_code attribute, it should indicate that ex is HttpResponseError, so, I personally speculate that this usage should be safe
|
|
jiasli
force-pushed
the
RoleAssignmentExists
branch
from
665bbb9 to
9de6ad0
Compare
jiasli
requested review from
FumingZhang,
calvinhzy,
jsntcy,
kairu-ms,
necusjz,
yanzhudd and
zhoxing-ms
as code owners
| if ex.error.code == 'RoleAssignmentExists': | ||
| logger.info('Role assignment already exists') | ||
| break | ||
| if retry_time < retry_times and ' does not exist in the directory ' in ex.message: |
There was a problem hiding this comment.
similar bad pattern of relying on error message instead of error code
There was a problem hiding this comment.
The new code is indeed relying on error code.
There was a problem hiding this comment.
I meant, this line still relies on message:
if retry_time < retry_times and ' does not exist in the directory ' in ex.message
There was a problem hiding this comment.
Ah. I see your point. This PR only aims to use RoleAssignmentExists error code. I will inform the ARM owner fix this line. @zhoxing-ms
There was a problem hiding this comment.
Okay, I'll take a look when I have time and fix it in other PR
Related command
az role assignment createDescription
Resolve #31995
az role assignment createcommand relies on the response message ('role assignment already exists' in ex.message) to implement idempotence. The code was introduced by #9108 in 2019 and hasn't been changed since then.However, recently ARM is making a breaking change to the response message on the existing API version
2022-04-01without an API version update. This breaks the idempotence ofaz role assignment createcommand.This PR uses the error code (
ex.error.code == 'RoleAssignmentExists') to detect if there is an existing role assignment.Testing Guide
az role assignment create --assignee a7003d8c-e50f-4371-91a6-ef37bba4ab23 --role reader --scope /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590 # Run the command again and make sure no error is returned az role assignment create --assignee a7003d8c-e50f-4371-91a6-ef37bba4ab23 --role reader --scope /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.