Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 0 additions & 3 deletions src/azure-cli/azure/cli/command_modules/role/_help.py
Original file line number Diff line number Diff line change
Expand Up @@ -684,9 +684,6 @@
helps['role assignment create'] = """
type: command
short-summary: Create a new role assignment for a user, group, or service principal.
long-summary: >-
--scope argument will become required for creating a role assignment in the breaking change release of the fall
of 2023. Please explicitly specify --scope.
examples:
- name: Create role assignment to grant the specified assignee the Reader role on an Azure virtual machine.
text: az role assignment create --assignee sp_name --role Reader --scope /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/MyResourceGroup/providers/Microsoft.Compute/virtualMachines/MyVm
Expand Down
10 changes: 6 additions & 4 deletions src/azure-cli/azure/cli/command_modules/role/_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,13 @@ def process_assignment_namespace(cmd, namespace): # pylint: disable=unused-argu
non_empty_msg = "usage error: {} can't be an empty string. Either omit it or provide a non-empty string."

for arg in non_empty_args:
if getattr(namespace, arg) == "":
if getattr(namespace, arg, None) == "":
Copy link
Contributor

@bebound bebound Nov 3, 2023

Choose a reason for hiding this comment

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

Is there any reason to add the default value for getattr, is there any difference between scope and other non_empty_args?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. After removing resource_group_name from az role assignment create, resource_group_name will not exist in namespace. If we don't add a default value here, getattr will fail for resource_group_name under az role assignment create.

# Get option name, like resource_group_name -> --resource-group
option_name = cmd.arguments[arg].type.settings['options_list'][0]
raise CLIError(non_empty_msg.format(option_name))

resource_group = namespace.resource_group_name
if namespace.scope and resource_group and getattr(resource_group, 'is_default', None):
namespace.resource_group_name = None # drop configured defaults
# `az role assignment create` doesn't support resource_group_name
if hasattr(namespace, "resource_group_name"):
Copy link
Member

Choose a reason for hiding this comment

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

We can mention removing --resource-group in history notes. This is also a breaking change customers need to pay attention

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Added to the History Notes section.

resource_group = namespace.resource_group_name
if namespace.scope and resource_group and getattr(resource_group, 'is_default', None):
namespace.resource_group_name = None # drop configured defaults
16 changes: 5 additions & 11 deletions src/azure-cli/azure/cli/command_modules/role/custom.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,6 @@
"The output includes credentials that you must protect. Be sure that you do not include these credentials in "
"your code or check the credentials into your source control. For more information, see https://aka.ms/azadsp-cli")

SCOPE_WARNING = (
"--scope argument will become required for creating a role assignment in the breaking change release of the fall "
"of 2023. Please explicitly specify --scope.")

logger = get_logger(__name__)

# pylint: disable=too-many-lines, protected-access
Expand Down Expand Up @@ -148,13 +144,11 @@ def _search_role_definitions(cli_ctx, definitions_client, name, scopes, custom_r
return []


def create_role_assignment(cmd, role, assignee=None, assignee_object_id=None, resource_group_name=None,
scope=None, assignee_principal_type=None, description=None,
def create_role_assignment(cmd, role, scope,
assignee=None, assignee_object_id=None,
assignee_principal_type=None, description=None,
condition=None, condition_version=None, assignment_name=None):
"""Check parameters are provided correctly, then call _create_role_assignment."""
if not scope:
logger.warning(SCOPE_WARNING)

if bool(assignee) == bool(assignee_object_id):
raise CLIError('usage error: --assignee STRING | --assignee-object-id GUID')

Expand Down Expand Up @@ -183,13 +177,13 @@ def create_role_assignment(cmd, role, assignee=None, assignee_object_id=None, re
principal_type = _get_principal_type_from_object_id(cmd.cli_ctx, assignee_object_id)

try:
return _create_role_assignment(cmd.cli_ctx, role, object_id, resource_group_name, scope, resolve_assignee=False,
return _create_role_assignment(cmd.cli_ctx, role, object_id, scope=scope, resolve_assignee=False,
assignee_principal_type=principal_type, description=description,
condition=condition, condition_version=condition_version,
assignment_name=assignment_name)
except Exception as ex: # pylint: disable=broad-except
if _error_caused_by_role_assignment_exists(ex): # for idempotent
return list_role_assignments(cmd, assignee, role, resource_group_name, scope)[0]
return list_role_assignments(cmd, assignee=assignee, role=role, scope=scope)[0]
raise


Expand Down
Loading