-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[RDBMS] Hotfix: az postgres flexible-server create : Update VnetName to exclude servername and update default region for MySQL
#15295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
01ea325
191ffec
5c6e0c2
ddec5f4
3f27c00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ | |
| get_three_state_flag) | ||
| from azure.cli.command_modules.rdbms.validators import configuration_value_validator, validate_subnet, \ | ||
| retention_validator, tls_validator, public_access_validator, pg_storage_validator, mysql_storage_validator, tier_validator, \ | ||
| pg_sku_name_validator, mysql_sku_name_validator, pg_version_validator, mysql_version_validator, maintenance_window_validator, ip_address_validator | ||
| pg_sku_name_validator, pg_version_validator, mysql_version_validator, maintenance_window_validator, ip_address_validator | ||
| from azure.cli.core.commands.validators import get_default_location_from_resource_group | ||
| from azure.cli.core.local_context import LocalContextAttribute, LocalContextAction | ||
|
|
||
|
|
@@ -240,9 +240,9 @@ def _flexible_server_params(command_group): | |
| c.argument('zone', options_list=['--zone, -z'], | ||
| help='Availability zone into which to provision the resource.') | ||
| elif command_group == 'mysql': | ||
| c.argument('tier', default='Burstable', validator=tier_validator, | ||
| c.argument('tier', default='Burstable', | ||
| help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ') | ||
| c.argument('sku_name', default='Standard_B1ms', options_list=['--sku-name'], validator=mysql_sku_name_validator, | ||
Juliehzl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| c.argument('sku_name', default='Standard_B1ms', options_list=['--sku-name'], | ||
| help='The name of the compute SKU. Follows the convention Standard_{VM name}. Examples: Standard_B1ms, Standard_D4s_v3 ') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to get allowed sku values through SDK?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a permanent fix for this in S176 where we fetch the SKU's from the SDK. We are currently removing the validators in the PR as there is a bug in the validator which prevents provisioning servers with certain SKU's
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good |
||
| c.argument('storage_mb', default='10', 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.') | ||
|
|
@@ -290,8 +290,6 @@ def _flexible_server_params(command_group): | |
|
|
||
| with self.argument_context('{} flexible-server update'.format(command_group)) as c: | ||
| c.ignore('assign_identity') | ||
| c.argument('tier', options_list=['--tier'], validator=tier_validator, | ||
| help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ') | ||
| 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) | ||
| c.argument('administrator_login_password', options_list=['--admin-password', '-p'], | ||
|
|
@@ -302,7 +300,9 @@ def _flexible_server_params(command_group): | |
| help='Period of time (UTC) designated for maintenance. Examples: "Sun:23:30" to schedule on Sunday, 11:30pm UTC. To set back to default pass in "Disabled".') | ||
| c.argument('tags', tags_type) | ||
| if command_group == 'mysql': | ||
| c.argument('sku_name', options_list=['--sku-name'], validator=mysql_sku_name_validator, | ||
| c.argument('tier', options_list=['--tier'], | ||
| help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For allowed values. prefer using get_enume_type()
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in the SDK comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean all allowed values for tier and sku_name. Could we also get the allowed array from sdk?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we do have validators for tier, sku_name, storage size validated by the array from sdk. We will include that for the next train. |
||
| c.argument('sku_name', options_list=['--sku-name'], | ||
Juliehzl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| help='The name of the compute SKU. Follows the convention Standard_{VM name}. Examples: Standard_B1ms, Standard_D4s_v3 ') | ||
| c.argument('storage_mb', options_list=['--storage-size'], type=int, | ||
| validator=mysql_storage_validator, | ||
|
|
@@ -317,6 +317,8 @@ def _flexible_server_params(command_group): | |
| c.argument('replication_role', options_list=['--replication-role'], | ||
| help='The replication role of the server.') | ||
| elif command_group == 'postgres': | ||
| c.argument('tier', options_list=['--tier'], validator=tier_validator, | ||
| help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ') | ||
| c.argument('sku_name', options_list=['--sku-name'], validator=pg_sku_name_validator, | ||
| 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, | ||
|
|
@@ -410,13 +412,15 @@ def _flexible_server_params(command_group): | |
| with self.argument_context('{} flexible-server replica create'.format(command_group)) as c: | ||
| c.argument('source_server', options_list=['--source-server'], | ||
| help='The name or resource ID of the source server to restore from.') | ||
| c.argument('tier', options_list=['--tier'], validator=tier_validator, | ||
| help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ') | ||
| if command_group == 'mysql': | ||
| 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'], | ||
| validator=mysql_sku_name_validator, | ||
| help='The name of the compute SKU. Follows the convention' | ||
| ' Standard_{VM name}. Examples: Standard_B1ms, Standard_D4s_v3 ') | ||
| if command_group == 'postgres': | ||
| c.argument('tier', options_list=['--tier'], validator=tier_validator, | ||
| help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ') | ||
|
|
||
| with self.argument_context('{} flexible-server replica stop-replication'.format(command_group)) as c: | ||
| c.argument('server_name', options_list=['--name', '-s'], help='Name of the server.') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,7 +59,8 @@ def flexible_server_create(cmd, client, | |
|
|
||
| # Populate desired parameters | ||
| location, resource_group_name, server_name = generate_missing_parameters(cmd, location, resource_group_name, | ||
| server_name) | ||
| server_name, 'postgres') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please avoid positional arguments here |
||
| server_name = server_name.lower() | ||
|
|
||
| # Handle Vnet scenario | ||
| if (subnet_arm_resource_id is not None) or (vnet_resource_id is not None): | ||
|
|
@@ -113,15 +114,15 @@ def flexible_server_create(cmd, client, | |
| sku = server_result.sku.name | ||
| host = server_result.fully_qualified_domain_name | ||
|
|
||
| logger.warning('Make a note of your password. If you forget, you would have to \ | ||
| reset your password with \'az postgres flexible-server update -n %s -g %s -p <new-password>\'.', | ||
| logger.warning('Make a note of your password. If you forget, you would have to ' | ||
| 'reset your password with \'az postgres flexible-server update -n %s -g %s -p <new-password>\'.', | ||
| server_name, resource_group_name) | ||
|
|
||
| _update_local_contexts(cmd, server_name, resource_group_name, location, user) | ||
|
|
||
| return _form_response(user, sku, loc, server_id, host, version, | ||
| administrator_login_password if administrator_login_password is not None else '*****', | ||
| _create_postgresql_connection_string(host, administrator_login_password), firewall_id, | ||
| _create_postgresql_connection_string(host, user, administrator_login_password), firewall_id, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. avoid positional arguments here |
||
| subnet_id) | ||
| except Exception as ex: # pylint: disable=broad-except | ||
| logger.error(ex) | ||
|
|
@@ -329,7 +330,7 @@ def flexible_server_connection_string( | |
|
|
||
| def _create_postgresql_connection_strings(host, user, password, database): | ||
| result = { | ||
| 'psql_cmd': "psql --host={host} --port=5432 --username={user} --dbname=postgres", | ||
| 'psql_cmd': "postgresql://{user}:{password}@{host}/postgres?sslmode=require", | ||
| 'ado.net': "Server={host};Database=postgres;Port=5432;User Id={user};Password={password};", | ||
| 'jdbc': "jdbc:postgresql://{host}:5432/postgres?user={user}&password={password}", | ||
| 'jdbc Spring': "spring.datasource.url=jdbc:postgresql://{host}:5432/postgres " | ||
|
|
@@ -355,12 +356,13 @@ def _create_postgresql_connection_strings(host, user, password, database): | |
| return result | ||
|
|
||
|
|
||
| def _create_postgresql_connection_string(host, password): | ||
| def _create_postgresql_connection_string(host, user, password): | ||
| connection_kwargs = { | ||
| 'user': user, | ||
| 'host': host, | ||
| 'password': password if password is not None else '{password}' | ||
| } | ||
| return 'postgres://postgres:{password}@{host}/postgres?sslmode=require'.format(**connection_kwargs) | ||
| return 'postgresql://{user}:{password}@{host}/postgres?sslmode=require'.format(**connection_kwargs) | ||
|
|
||
|
|
||
| def _form_response(username, sku, location, server_id, host, version, password, connection_string, firewall_id=None, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db_engine.lower()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that we are explicitly passing as lower case from the caller. Line 61 in flexible_server_custom_postgres.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you pls add some test to change the overall change?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this, we are just changing the default location from eastus to westus2 for MySQL. We already have tests for both MySQL and Postgres SQL creation otherwise.