Conversation
|
Skipping CI for Draft Pull Request. |
cadenmarchese
left a comment
cadenmarchese
left a comment
There was a problem hiding this comment.
nice work, things are working for me locally. I think at this point it's mostly just idempotence and DRY needed. thanks!
| vnet, | ||
| vnet_resource_group_name=None) -> list: | ||
|
|
||
| # FIXME: figure out how to do the fancy "in progress" spinner |
There was a problem hiding this comment.
Alternatively, can we have the CLI print what it's doing during each creation? Something like Creating identity "cloud-controller-manager" in resource group "caden-miwi" for version "4.19.20" and etc...
python/az/aro/azext_aro/custom.py
Outdated
|
|
||
| # vnet/subnet role assignments | ||
| for scope in scopes: | ||
| ra = racreate(command_args={ |
There was a problem hiding this comment.
While we can't directly modify the aaz itself, we'll need some kind of "check if this already exists" behavior implemented here before we execute the create so that we can run this command idempotently. In its current state, if I delete a previously created identity and re-run this command, I will get an "already exists" error because there's a leftover orphaned role assignment:
(RoleAssignmentExists) The role assignment already exists.
Code: RoleAssignmentExists
Message: The role assignment already exists.
Fixing this will account for cases where a role assignment or identity failed to create and the customer needs to re-run. It's a bit tedious removing role assignments manually and most customers won't have the option to remove and re-create the whole resource group every time.
python/az/aro/azext_aro/custom.py
Outdated
| racreate = roleassignment_create(cli_ctx=cmd.cli_ctx) | ||
|
|
||
| # cluster top-level identity | ||
| cluster_id = idcreate(command_args={ |
There was a problem hiding this comment.
Before we execute any creation, should we prompt the user Y/n like we do on cluster deletion? (I don't have a strong opinion since we don't do it on cluster create, but it might be nice here)
Required identities and role assignments for version "4.19.20" in resource group "caden-miwi" will be created. Proceed? (Y/n)
| ) | ||
|
|
||
|
|
||
| def aro_identity_create_required(*, |
There was a problem hiding this comment.
Most of this function is duplicated from our previous command get-required. Can we move the duplicated code to a separate function where we can call it both here and in get-required?
| @@ -0,0 +1,43 @@ | |||
| # ARO-RP/python | |||
There was a problem hiding this comment.
Thanks for the readme!
We also have this one: https://github.com/Azure/ARO-RP/blob/master/docs/az-aro-python-development.md should we consolidate them?
There was a problem hiding this comment.
We should. I remembered that az-aro-python-development.md existed after I started this one. I have changes in my working tree to consolidate them.
Azure CLI recommends using `logger.<severity>()` instead of `print()`.
.. between aro_identity_get_required and aro_identity_create_required
2 participants
Which issue this PR addresses:
Fixes https://issues.redhat.com/browse/ARO-6445
What this PR does / why we need it:
az aro identity create-requiredcreates all necessary identities and role assignments to facilitate creation of a Managed-Identity OpenShift Cluster.Test plan for issue:
There are little (or no) tests for our ARO CLI extension at the moment. Building tests are a separate work ticket (ARO-8293).
Is there any documentation that needs to be updated for this PR?
Not that I'm aware.
How do you know this will function as expected in production?
This is new CLI functionality that interacts with existing APIs.