Skip to content

Conversation

@arde0708
Copy link
Contributor

@arde0708 arde0708 commented Oct 2, 2020

The 'az postgres flexible-server create' command fails with an error as reported in the issue. It happens only when CLI is installed through the installer.

This PR just removes that hardcoded API version.
https://github.com/MicrosoftDocs/azure-docs/issues/63318

arde0708 and others added 5 commits September 29, 2020 17:11
…e to exclude servername and update default region for MySQL (Azure#15295)

* hotfix chnages

* Remove - from password if thats the first character

* Style check fix and Mysql sku

* Remove MySQL SKU validator

* Remove MySQL SKu and tier validators
@arde0708 arde0708 changed the title Remove hardcoded API version from network client. [RDBMS] Bugfix: az postgres flexible-server create Remove hardcoded API version from network client. Oct 2, 2020
@yonzhan yonzhan added this to the S177 milestone Oct 2, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Oct 2, 2020

RDBMS

@yungezz yungezz requested a review from mmyyrroonn October 6, 2020 12:49
Comment on lines 326 to +327
def network_client_factory(cli_ctx):
from azure.mgmt.network import NetworkManagementClient
return get_mgmt_service_client(cli_ctx, NetworkManagementClient, api_version="2018-08-01")
return get_mgmt_service_client(cli_ctx, ResourceType.MGMT_NETWORK)
Copy link
Contributor

Choose a reason for hiding this comment

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

actually you even don't need to define it here. network module already has the definition and you could just import it.

def network_client_factory(cli_ctx, **kwargs):
from azure.cli.core.profiles import ResourceType
from azure.cli.core.commands.client_factory import get_mgmt_service_client
return get_mgmt_service_client(cli_ctx, ResourceType.MGMT_NETWORK, **kwargs)

help='The name of the compute SKU. Follows the convention Standard_{VM name}. Examples: Standard_B1ms, Standard_D4s_v3 ')
c.argument('storage_mb', default='10', options_list=['--storage-size'], type=int, validator=mysql_storage_validator,
help='The name of the compute SKU. Follows the convention Standard_{VM name}. Examples: Standard_B1ms, Standard_E16ds_v4 ')
c.argument('storage_mb', default='10', options_list=['--storage-size'], type=int,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
c.argument('storage_mb', default='10', options_list=['--storage-size'], type=int,
c.argument('storage_mb', default=10, options_list=['--storage-size'], type=int,

because it is int type.

c.argument('backup_retention', default=7, type=int, options_list=['--backup-retention'],
help='The number of days a backup is retained. Range of 7 to 35 days. Default is 7 days.',
validator=retention_validator)
c.argument('version', default='12', options_list=['--version'],
Copy link
Contributor

Choose a reason for hiding this comment

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

quick question: how can user know which kind of version is accepted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

quick question: how can user know which kind of version is accepted?

Do you mean how they can find out which are the valid versions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they pass in a wrong value, they will get a Validation error with the set of acceptable values.

if command_group == 'mysql':
c.argument('tier', options_list=['--tier'],
help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ')
help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized')
Copy link
Contributor

Choose a reason for hiding this comment

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

still prefer getting allowed value from SDK

help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized')
c.argument('sku_name', options_list=['--sku-name'],
help='The name of the compute SKU. Follows the convention Standard_{VM name}. Examples: Standard_B1ms, Standard_D4s_v3 ')
help='The name of the compute SKU. Follows the convention Standard_{VM name}. Examples: Standard_B1ms, Standard_E16ds_v4 ')
Copy link
Contributor

Choose a reason for hiding this comment

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

The same

c.argument('storage_mb', options_list=['--storage-size'], type=int,
validator=mysql_storage_validator,
help='The storage capacity of the server. Minimum is 5 GiB and increases in 1 GiB increments. Max is 16 TiB.')
c.argument('backup_retention', type=int, options_list=['--backup-retention'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is to define the retention days but how could I enable/disable delete retention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It cannot be disabled. By default it is 7 days. But user can pass a value and override the default value.

Comment on lines +320 to +327
c.argument('tier', options_list=['--tier'],
help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized')
c.argument('sku_name', options_list=['--sku-name'],
help='The name of the compute SKU. Follows the convention Standard_{VM name}. Examples: Standard_D4s_v3 ')
c.argument('storage_mb', options_list=['--storage-size'], type=int,
validator=pg_storage_validator,
help='The storage capacity of the server. Minimum is 32 GiB and max is 16 TiB.')
c.argument('backup_retention', type=int, options_list=['--backup-retention'],
help='The number of days a backup is retained. Range of 7 to 35 days. Default is 7 days.', validator=retention_validator)
Copy link
Contributor

Choose a reason for hiding this comment

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

you have so many duplicate arguments. could you use CLIArgumentType to abstract them and reuse these type.

Comment on lines +416 to +418
c.ignore('location')
c.ignore('sku_name')
c.ignore('tier')
Copy link
Contributor

Choose a reason for hiding this comment

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

The three arguments are totally removed in existing command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not using these three args for the command currently

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

discussed with @arde0708 online and checked the telemetry for removed arguments (1 sub usage from @arde0708 team).
Approved the PR for catching the release.

Please fix other comments in the following PR and prefer separating to several PRs for different motivation.

@Juliehzl Juliehzl merged commit 47b197d into Azure:dev Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants