[ACR] Updates for new RP data models #1198
Conversation
DavidObando
commented
Nov 1, 2016
DavidObando
commented
- Updated acr client to support adding/switching environment using cloud/context command modules.
- Updated repository return value to support table format.
- Updated SDK.
- Changed storage account update as part of "az acr update" command. Removed storage subgroup.
- Removed client side validation on registry name and storage account. Added a separate "az acr check-name" command to check name availability.
- Updated format to match new models.
- Changed "az acr list" command from ARM list to RP list.
- Added regenerate admin user credental command.
1. Updated acr client to support adding/switching environment using cloud/context command modules. 2. Updated repository return value to support table format.
1. Updated SDK. 2. Changed storage account update as part of "az acr update" command. Removed storage subgroup. 3. Removed client side validation on registry name and storage account. Added a separate "az acr check-name" command to check name availability. 4. Updated format to match new models. 5. Changed "az acr list" command from ARM list to RP list. 6. Added regenerate admin user credental command.
|
Hi @DavidObando, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
|
/cc @sajayantony @sivagms |
|
/cc @amarzavery |
|
/cc @tjprescott |
There was a problem hiding this comment.
Some minor things on consistency with other commands, but more importantly, this module has no test coverage.
https://github.com/Azure/azure-cli/blob/master/doc/recording_vcr_tests.md
| @@ -86,39 +86,39 @@ Update a container registry | |||
| az acr update: Update a container registry. | |||
There was a problem hiding this comment.
For consistency with other CLI modules you may want to wrap this in a generic update (which adds the --add, --set and --remove flags). network nic update is a good example of an update command that exposes the generic parameters and convenience parameters (which are the ones you already expose).
| invalidate assigned access of existing service principals. | ||
| --name -n [Required]: Name of container registry. | ||
| --disable-admin : Disable admin user. | ||
| --enable-admin : Enable admin user. |
There was a problem hiding this comment.
Generally in the CLI we don't use --enable and --disable flags as it leads to flag clutter and depending on the other arguments, could be separated in the argument list.
Instead, for creates we would have a single flag which triggers the non-default case (which would be, I assume, enable-admin). Then for the update, we would have an option --admin which would have the choice list ['enable', 'diasable']
There was a problem hiding this comment.
I've opened #1201 & #1202 to cover these 2 separately.
This PR is for the model update and if we are ok with that we would like to unblock the new RP roll out which is a part of the swagger update which we need to have today. Otherwise our schedule slips by about a week since the connect folks won't have time utilize the new models. Basically this PR was scoped only the model update as per @amarzavery's feedback and we have 2 more weeks to iron out any non-blocking CLI changes or argument collapsing.
We will send out a PR for the other 2 items once @djyou is back.
Basically the new RP cannot be consumed unless the CLI is merged, otherwise we would need folks to update their CLI from our branch.
@tjprescott - does that sound reasonable?
