{App Config} az appconfig: Add premium sku support
#29417
Conversation
️✔️AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| appconfig create | cmd appconfig create added parameter no_replica |
||
| appconfig create | cmd appconfig create added parameter replica_location |
||
| appconfig create | cmd appconfig create added parameter replica_name |
||
| appconfig create | cmd appconfig create update parameter sku: updated property choices from ['Free', 'Standard'] to ['Free', 'Premium', 'Standard'] |
||
| appconfig replica delete | cmd appconfig replica delete removed property confirmation |
||
| appconfig update | cmd appconfig update update parameter sku: updated property choices from ['Free', 'Standard'] to ['Free', 'Premium', 'Standard'] |
|
AppConfig |
microsoft-github-policy-service
bot
requested review from
yanzhudd,
zhoxing-ms,
yonzhan,
jsntcy and
avanigupta
microsoft-github-policy-service
bot
added
App Configuration
Auto-Assign
|
Please fix CI issues |
ChristineWanjau
changed the title
az appconfig create Add premium sku supportaz appconfig create/update: Add premium sku support
ChristineWanjau
changed the title
az appconfig create/update: Add premium sku supportaz appconfig create/update: Add premium sku support
| @@ -5,7 +5,8 @@ | |||
|
|
|||
| # pylint: disable=line-too-long | |||
| from knack.log import get_logger | |||
| from azure.core.exceptions import ResourceNotFoundError | |||
| from azure.core.exceptions import ResourceNotFoundError, HttpResponseError | |||
| from azure.cli.command_modules.appconfig._constants import StatusCodes | |||
There was a problem hiding this comment.
nit: from ._constants import ...
| ) | ||
| # Incase premium sku is not available in a given region yet. Temporary fix will be removed when premium is in all regions | ||
| except HttpResponseError as exception: | ||
| if exception.status_code == StatusCodes.BAD_REQUEST: |
There was a problem hiding this comment.
Can all this be 1 if statement so we would not need multiple else statements doing the same thing?
|
Please note that Azure CLI will have a code freeze on 07/30/2024 10:00 UTC for the upcoming release. If you want to catch this release train, please address all the comments ASAP, otherwise it has to be postponed to next sprint (09-03). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| progress.spinner.step(label="Creating store") | ||
| config_store.wait(3) | ||
|
|
||
| if config_store.status() == ProvisioningStatus.SUCCEEDED and replica_name is not None: |
There was a problem hiding this comment.
What happens when store creation fails? Are we guaranteed an error will be thrown?
There was a problem hiding this comment.
Yeah it still throws an error. wait() function can throw error
| @@ -269,13 +312,29 @@ def create_replica(cmd, client, store_name, name, location, resource_group_name= | |||
| replica_creation_parameters=replica_creation_params) | |||
|
|
|||
|
|
|||
| def delete_replica(cmd, client, store_name, name, resource_group_name=None): | |||
| def delete_replica(cmd, client, store_name, name, yes=False, resource_group_name=None): | |||
There was a problem hiding this comment.
Just to confirm, this command already had a confirmation so this will not break users, right?
There was a problem hiding this comment.
Yes the delete command already had a confirmation.
|
|
||
| if config_store.sku.name.lower() == "premium" and len(list(replicas)) == 1: | ||
| user_confirmation( | ||
| "Deleting the last replica will disable geo-replication. When using the premium tier, it is recommended to have geo-replication enabled to take advantage of an increased SLA of 99.99%. The first replica created for a premium tier store is included. Do you want to continue with this operation?" |
There was a problem hiding this comment.
Not exactly sure but how about this?
Deleting the last replica will disable geo-replication. It is recommended that a premium tier store have geo-replication enabled to take advantage of the 99.99% SLA. The first replica for a premium tier store comes at no additional cost.
There was a problem hiding this comment.
We usually avoid documenting SLA in clients. We have dedicated documentation for SLAs.
There was a problem hiding this comment.
I see, Although these were strings discussed by Jimmy and Joe. We have added them on portal as well. I can confirm with them on this.
There was a problem hiding this comment.
Perhaps, we can remove the numbers?
Deleting the last replica will disable geo-replication. It is recommended that a premium tier store have geo-replication enabled to take advantage of the improved SLA. The first replica for a premium tier store comes at no additional cost.
@avanigupta @ChristineWanjau Thoughts?
There was a problem hiding this comment.
This makes sense to me. Updated
| @@ -144,6 +159,9 @@ def load_arguments(self, _): | |||
| c.argument('disable_local_auth', arg_type=get_three_state_flag(), help='Disable all authentication methods other than AAD authentication.') | |||
| c.argument('retention_days', arg_type=retention_days_arg_type) | |||
| c.argument('enable_purge_protection', options_list=['--enable-purge-protection', '-p'], arg_type=get_three_state_flag(), help='Property specifying whether protection against purge is enabled for this App Configuration store. Setting this property to true activates protection against purge for this App Configuration store and its contents. Enabling this functionality is irreversible.') | |||
| c.argument('replica_name', arg_type=store_creation_replica_name_arg_type, help='Name of the replica of the App Configuration store.') | |||
| c.argument('replica_location', arg_type=replica_location_arg_type, help='Location of the replica of the App Configuration store.') | |||
| c.argument('no-replica', help='Proceed without replica creation for premium tier store.', arg_type=get_three_state_flag()) | |||
| replica_location=None, | ||
| no_replica=False): | ||
|
|
||
| if sku.lower() == 'premium' and not no_replica: |
There was a problem hiding this comment.
Are the new arguments supported for standard tier?
There was a problem hiding this comment.
Yes they are. You can create a replica while creating a standard tier store, although --no-replica argument when provided will be ignored.
| ) | ||
| replicas = list_replica(cmd, client, store_name, resource_group_name) | ||
|
|
||
| if config_store.sku.name.lower() == "premium" and len(list(replicas)) == 1: |
There was a problem hiding this comment.
What if the provided replica name does not match the last replica name? The operation should fail without the need to display this warning.
There was a problem hiding this comment.
Currently when you want to delete a replica and fail to provide the replica name that matches the last replica name/replica name that exists. We still ask for user confirmation and the operation doesn't fail.
I was thinking we should still show the warning if we also ask for confirmation with the current implementation. What do you think
There was a problem hiding this comment.
ok, thanks! we can show the warning even if name does not match
| retention_days = None | ||
| enable_purge_protection = None | ||
|
|
||
| __validate_replication(sku, replica_name, replica_location, no_replica) |
There was a problem hiding this comment.
I think we should move this validation to _validators.py
There was a problem hiding this comment.
We move the whole function to validators.py?
There was a problem hiding this comment.
@avanigupta I initially had a similar thought, but since this method jointly validates 3 arguments, I was wondering which of them the validation should be added to in params.
There was a problem hiding this comment.
I think it should be on the sku.
There was a problem hiding this comment.
The warning about free sku just above this code can also move to validators
There was a problem hiding this comment.
Moving the validation function to validators.py and adding it to the sku was causing tests to fail. I was getting an error - "Attribute "no-replica" or "replica-name" doesn't exist in Namespace. I moved it back to the custom.py.
I am thinking its because the arguments are in a different argument context than the sku argument.
| @@ -18,6 +18,10 @@ | |||
| examples: | |||
| - name: Create an App Configuration store with name, location, sku, tags and resource group. | |||
| text: az appconfig create -g MyResourceGroup -n MyAppConfiguration -l westus --sku Standard --tags key1=value1 key2=value2 | |||
| - name: Create an App Configuration store with name, location, resource group and premium sku with a replica | |||
There was a problem hiding this comment.
| - name: Create an App Configuration store with name, location, resource group and premium sku with a replica | |
| - name: Create a premium sku App Configuration store with a replica |
There was a problem hiding this comment.
This has been updated
| @@ -18,6 +18,10 @@ | |||
| examples: | |||
| - name: Create an App Configuration store with name, location, sku, tags and resource group. | |||
| text: az appconfig create -g MyResourceGroup -n MyAppConfiguration -l westus --sku Standard --tags key1=value1 key2=value2 | |||
| - name: Create an App Configuration store with name, location, resource group and premium sku with a replica | |||
| text: az appconfig create -g MyResourceGroup -n MyAppConfiguration -l westus --sku Premium --replica-name MyReplica --replica-location eastus | |||
| - name: Create an App Configuration store with name, location, resource group and premium sku without a replica | |||
There was a problem hiding this comment.
| - name: Create an App Configuration store with name, location, resource group and premium sku without a replica | |
| - name: Create a premium sku App Configuration store without a replica |
There was a problem hiding this comment.
This has been updated
| if sku.lower() == 'free' and (enable_purge_protection or retention_days): | ||
| logger.warning("Options '--enable-purge-protection' and '--retention-days' will be ignored when creating a free store.") | ||
| if sku.lower() == 'free' and (enable_purge_protection or retention_days or replica_name or replica_location or no_replica): | ||
| logger.warning("Options '--enable-purge-protection', '--replica-name', '--replica-location' , 'no-replica' and '--retention-days' will be ignored when creating a free store.") |
| progress.spinner.step(label="Creating store") | ||
| config_store.wait(3) | ||
| config_store.wait(1) | ||
|
|
||
| if config_store.status() == ProvisioningStatus.SUCCEEDED and replica_name is not None: |
There was a problem hiding this comment.
nit: We do not need to check config_store.status() == ProvisioninsStatus.SUCCEEDED
There was a problem hiding this comment.
Yeah, updated
…ChristineWanjau/azure-cli into cwanjau/addPremiumSkuSupportCLI
yanzhudd
changed the title
az appconfig create/update: Add premium sku supportaz appconfig: Add premium sku support
yanzhudd
changed the title
az appconfig: Add premium sku supportaz appconfig: Add premium sku support
Related command
az appconfig create/update
Description
Adds new option to --sku argument in az appconfig update/create
Adds new arguments to az appconfig create i.e --replica-name, --replica-location and --no-replica
Either of the new arguments will be required when creating a premium tier app configuration store.
A user should either specify --no-replica or create a replica using --replica-name and --replica-location.
Testing Guide
az appconfig create -n <storename> -l <location> --sku Premium --no-replica)az appconfig create -n <storename> -l <location> --sku Premium --replica-name <yourReplica> --replica-location <replicaLocation>)History 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.