From b38a6dc9f719e0f615359015d63886115b6e4018 Mon Sep 17 00:00:00 2001 From: Michel El Nacouzi Date: Fri, 17 Jan 2025 06:25:07 -0500 Subject: [PATCH] Fix issue with list of accounts in release channels and directives (#1997) --- .../nativeapp/entities/application_package.py | 4 ++-- .../nativeapp/release_channel/commands.py | 8 +++---- .../nativeapp/release_directive/commands.py | 4 ++-- .../cli/_plugins/nativeapp/sf_sql_facade.py | 21 ++++++++++++------- src/snowflake/cli/api/errno.py | 1 + tests/nativeapp/test_sf_sql_facade.py | 11 +++++++++- 6 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/snowflake/cli/_plugins/nativeapp/entities/application_package.py b/src/snowflake/cli/_plugins/nativeapp/entities/application_package.py index e0f92aaa61..326a5d66d6 100644 --- a/src/snowflake/cli/_plugins/nativeapp/entities/application_package.py +++ b/src/snowflake/cli/_plugins/nativeapp/entities/application_package.py @@ -1724,12 +1724,12 @@ def resolve_version_info( if not self.get_existing_version_info(to_identifier(resolved_version)): raise BadOptionUsage( option_name="patch", - message=f"Cannot create patch {resolved_patch} when version {resolved_version} is not defined in the application package {self.name}. Try again without specifying a patch.", + message=f"Version {resolved_version} is not defined in the application package {self.name}. Try again with a patch of 0 or without specifying any patch.", ) except ApplicationPackageDoesNotExistError as app_err: raise BadOptionUsage( option_name="patch", - message=f"Cannot create patch {resolved_patch} when application package {self.name} does not exist. Try again without specifying a patch.", + message=f"Application package {self.name} does not exist yet. Try again with a patch of 0 or without specifying any patch.", ) return VersionInfo( diff --git a/src/snowflake/cli/_plugins/nativeapp/release_channel/commands.py b/src/snowflake/cli/_plugins/nativeapp/release_channel/commands.py index 503e445b75..3c9e364750 100644 --- a/src/snowflake/cli/_plugins/nativeapp/release_channel/commands.py +++ b/src/snowflake/cli/_plugins/nativeapp/release_channel/commands.py @@ -80,7 +80,7 @@ def release_channel_add_accounts( show_default=False, help="The release channel to add accounts to.", ), - target_accounts: list[str] = typer.Option( + target_accounts: str = typer.Option( show_default=False, help="The accounts to add to the release channel. Format has to be `org1.account1,org2.account2`.", ), @@ -100,7 +100,7 @@ def release_channel_add_accounts( package_id, EntityActions.RELEASE_CHANNEL_ADD_ACCOUNTS, release_channel=channel, - target_accounts=target_accounts, + target_accounts=target_accounts.split(","), ) return MessageResult("Successfully added accounts to the release channel.") @@ -114,7 +114,7 @@ def release_channel_remove_accounts( show_default=False, help="The release channel to remove accounts from.", ), - target_accounts: list[str] = typer.Option( + target_accounts: str = typer.Option( show_default=False, help="The accounts to remove from the release channel. Format has to be `org1.account1,org2.account2`.", ), @@ -134,7 +134,7 @@ def release_channel_remove_accounts( package_id, EntityActions.RELEASE_CHANNEL_REMOVE_ACCOUNTS, release_channel=channel, - target_accounts=target_accounts, + target_accounts=target_accounts.split(","), ) return MessageResult("Successfully removed accounts from the release channel.") diff --git a/src/snowflake/cli/_plugins/nativeapp/release_directive/commands.py b/src/snowflake/cli/_plugins/nativeapp/release_directive/commands.py index 17cd351f65..603799ac62 100644 --- a/src/snowflake/cli/_plugins/nativeapp/release_directive/commands.py +++ b/src/snowflake/cli/_plugins/nativeapp/release_directive/commands.py @@ -92,7 +92,7 @@ def release_directive_set( DEFAULT_CHANNEL, help="Name of the release channel to use", ), - target_accounts: Optional[list[str]] = typer.Option( + target_accounts: Optional[str] = typer.Option( None, show_default=False, help="List of the accounts to apply the release directive to. Format has to be `org1.account1,org2.account2`", @@ -126,7 +126,7 @@ def release_directive_set( release_directive=directive, version=version, patch=patch, - target_accounts=target_accounts, + target_accounts=None if target_accounts is None else target_accounts.split(","), release_channel=channel, ) return MessageResult("Successfully set release directive.") diff --git a/src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py b/src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py index 671e43da1d..81bca944d8 100644 --- a/src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py +++ b/src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py @@ -67,6 +67,7 @@ RELEASE_DIRECTIVE_DOES_NOT_EXIST, RELEASE_DIRECTIVES_VERSION_PATCH_NOT_FOUND, SQL_COMPILATION_ERROR, + TARGET_ACCOUNT_USED_BY_OTHER_RELEASE_DIRECTIVE, VERSION_ALREADY_ADDED_TO_RELEASE_CHANNEL, VERSION_DOES_NOT_EXIST, VERSION_NOT_ADDED_TO_RELEASE_CHANNEL, @@ -737,7 +738,7 @@ def upgrade_application( """ name = to_identifier(name) - release_channel = to_identifier(release_channel) if release_channel else None + release_channel = to_identifier(release_channel or DEFAULT_CHANNEL) install_method.ensure_app_usable( app_name=name, @@ -754,14 +755,14 @@ def get_app_properties(): with self._use_role_optional(role), self._use_warehouse_optional(warehouse): try: using_clause = install_method.using_clause(path_to_version_directory) - if release_channel: - current_release_channel = get_app_properties().get( - CHANNEL_COL, DEFAULT_CHANNEL + + current_release_channel = ( + get_app_properties().get(CHANNEL_COL) or DEFAULT_CHANNEL + ) + if unquote_identifier(release_channel) != current_release_channel: + raise UpgradeApplicationRestrictionError( + f"Application {name} is currently on release channel {current_release_channel}. Cannot upgrade to release channel {release_channel}." ) - if not same_identifiers(release_channel, current_release_channel): - raise UpgradeApplicationRestrictionError( - f"Application {name} is currently on release channel {current_release_channel}. Cannot upgrade to release channel {release_channel}." - ) upgrade_cursor = self._sql_executor.execute_query( f"alter application {name} upgrade {using_clause}", @@ -1095,6 +1096,10 @@ def set_release_directive( raise UserInputError( f"Release directive {release_directive} does not exist in application package {package_name}. Please create it first by specifying --target-accounts with the `snow app release-directive set` command." ) from err + if err.errno == TARGET_ACCOUNT_USED_BY_OTHER_RELEASE_DIRECTIVE: + raise UserInputError( + f"Some target accounts are already referenced by other release directives in application package {package_name}.\n{str(err.msg)}" + ) from err _handle_release_directive_version_error( err, package_name=package_name, diff --git a/src/snowflake/cli/api/errno.py b/src/snowflake/cli/api/errno.py index 3e57707a39..3fb175022e 100644 --- a/src/snowflake/cli/api/errno.py +++ b/src/snowflake/cli/api/errno.py @@ -74,6 +74,7 @@ MAX_VERSIONS_IN_RELEASE_CHANNEL_REACHED = 512004 MAX_UNBOUND_VERSIONS_REACHED = 512023 CANNOT_DEREGISTER_VERSION_ASSOCIATED_WITH_CHANNEL = 512021 +TARGET_ACCOUNT_USED_BY_OTHER_RELEASE_DIRECTIVE = 93091 ERR_JAVASCRIPT_EXECUTION = 100132 diff --git a/tests/nativeapp/test_sf_sql_facade.py b/tests/nativeapp/test_sf_sql_facade.py index e521bca48b..0862b441f0 100644 --- a/tests/nativeapp/test_sf_sql_facade.py +++ b/tests/nativeapp/test_sf_sql_facade.py @@ -63,6 +63,7 @@ RELEASE_DIRECTIVE_DOES_NOT_EXIST, RELEASE_DIRECTIVES_VERSION_PATCH_NOT_FOUND, SQL_COMPILATION_ERROR, + TARGET_ACCOUNT_USED_BY_OTHER_RELEASE_DIRECTIVE, VERSION_DOES_NOT_EXIST, VERSION_NOT_ADDED_TO_RELEASE_CHANNEL, VERSION_NOT_IN_RELEASE_CHANNEL, @@ -1847,6 +1848,7 @@ def test_upgrade_application_unversioned( mock_get_existing_app_info, mock_use_warehouse, mock_use_role, + mock_get_app_properties, mock_execute_query, mock_cursor, ): @@ -1985,6 +1987,7 @@ def test_upgrade_application_converts_expected_programmingerrors_to_user_errors( mock_get_existing_app_info, mock_use_warehouse, mock_use_role, + mock_get_app_properties, mock_execute_query, ): app_name = "test_app" @@ -2101,6 +2104,7 @@ def test_upgrade_application_converts_unexpected_programmingerrors_to_unclassifi mock_get_existing_app_info, mock_use_warehouse, mock_use_role, + mock_get_app_properties, mock_execute_query, ): app_name = "test_app" @@ -2159,7 +2163,7 @@ def test_upgrade_application_with_release_channel_same_as_app_properties( mock_get_app_properties.return_value = { COMMENT_COL: SPECIAL_COMMENT, AUTHORIZE_TELEMETRY_COL: "true", - CHANNEL_COL: release_channel, + CHANNEL_COL: release_channel.upper(), } side_effects, expected = mock_execute_helper( @@ -3343,6 +3347,11 @@ def test_set_default_release_directive_no_release_channel( UserInputError, "Release directive test_directive does not exist in application package test_package.", ), + ( + ProgrammingError(errno=TARGET_ACCOUNT_USED_BY_OTHER_RELEASE_DIRECTIVE), + UserInputError, + "Some target accounts are already referenced by other release directives in application package test_package.", + ), ( ProgrammingError(), InvalidSQLError,