Skip to content

{Role} az role assignment create: Use RoleAssignmentExists error code to implement idempotence #32027

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

Merged
jiasli merged 1 commit into from
Sep 5, 2025
Merged

{Role} az role assignment create: Use RoleAssignmentExists error code to implement idempotence #32027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/azure-cli-core/azure/cli/core/commands/arm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,7 +787,7 @@ def assign_identity(cli_ctx, getter, setter, identity_role=None, identity_scope=
parameters=parameters)
break
except HttpResponseError as ex:
if 'role assignment already exists' in ex.message:
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:
Copy link
Copy Markdown

@basdanny basdanny Sep 2, 2025

Choose a reason for hiding this comment

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

similar bad pattern of relying on error message instead of error code

Copy link
Copy Markdown
Member Author

@jiasli jiasli Sep 3, 2025

Choose a reason for hiding this comment

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

The new code is indeed relying on error code.

Copy link
Copy Markdown

@basdanny basdanny Sep 3, 2025

Choose a reason for hiding this comment

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

I meant, this line still relies on message:
if retry_time < retry_times and ' does not exist in the directory ' in ex.message

Copy link
Copy Markdown
Member Author

@jiasli jiasli Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@zhoxing-ms zhoxing-ms Sep 5, 2025

Choose a reason for hiding this comment

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

Okay, I'll take a look when I have time and fix it in other PR

Expand Down
2 changes: 1 addition & 1 deletion src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def add_role_assignment(cmd, role, service_principal_msi_id, is_service_principa
)
break
except HttpResponseError as ex:
if isinstance(ex, ResourceExistsError) or "The role assignment already exists." in ex.message:
if isinstance(ex, ResourceExistsError) or ex.error.code == 'RoleAssignmentExists':
break
logger.info(ex.message)
except Exception as ex: # pylint: disable=broad-except
Expand Down
2 changes: 1 addition & 1 deletion src/azure-cli/azure/cli/command_modules/role/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,7 +1332,7 @@ def _process_certificate(cli_ctx, years, app_start_date, app_end_date, cert, cre


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'
Copy link

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
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'

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@jiasli jiasli Sep 1, 2025

Choose a reason for hiding this comment

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

@zhoxing-ms, is it possible that ex.error is None?

Copy link
Copy Markdown
Contributor

@zhoxing-ms zhoxing-ms Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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



def _validate_app_dates(app_start_date, app_end_date, cert_start_date, cert_end_date):
Expand Down
2 changes: 1 addition & 1 deletion src/azure-cli/azure/cli/command_modules/synapse/manual/operations/accesscontrol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def _get_object_stubs(graph_client, assignees):


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'


def _create_role_assignment(cmd, workspace_name, role, assignee, scope=None, item=None, item_type=None,
Expand Down