Skip to content

Commit

Permalink
Cherrypicks v3.3.0 rc1 (#2003)
Browse files Browse the repository at this point in the history
* Fix notebook create on Windows (#1996)

* Add unit test

* test more cases

* fix stage path handling

* update release notes

* SNOW-1875316: Merge current connection context with config to preserve command line params (#2000)

SNOW-1875316: Merge current connection context with config to preserve cmd line params

* Fix issue with list of accounts in release channels and directives (#1997)

* bump version

* snapshot update

---------

Co-authored-by: Marcin Raba <[email protected]>
Co-authored-by: Michel El Nacouzi <[email protected]>
  • Loading branch information
3 people authored Jan 17, 2025
1 parent 88c05f3 commit 428ea09
Show file tree
Hide file tree
Showing 13 changed files with 122 additions and 26 deletions.
2 changes: 2 additions & 0 deletions RELEASE-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@
## Fixes and improvements
* Fixed inability to add patches to lowercase quoted versions.
* Fixes label being set to blank instead of None when not provided.
* Fixes generate-jwt command to preserve command line connection options.
* Fixed stage path handling for notebook commands.


# v3.2.2
Expand Down
2 changes: 1 addition & 1 deletion src/snowflake/cli/__about__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@

from __future__ import annotations

VERSION = "3.3.0rc0"
VERSION = "3.3.0rc1"
Original file line number Diff line number Diff line change
Expand Up @@ -1714,12 +1714,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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`.",
),
Expand All @@ -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.")
Expand All @@ -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`.",
),
Expand All @@ -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.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`",
Expand Down Expand Up @@ -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.")
Expand Down
21 changes: 13 additions & 8 deletions src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,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,
Expand Down Expand Up @@ -733,7 +734,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,
Expand All @@ -750,14 +751,14 @@ def get_app_properties():
with self._use_role_optional(role), self._use_warehouse_optional(warehouse):
try:
using_clause = install_method.using_clause(stage_fqn)
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}",
Expand Down Expand Up @@ -1091,6 +1092,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,
Expand Down
6 changes: 4 additions & 2 deletions src/snowflake/cli/_plugins/notebook/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from snowflake.cli.api.cli_global_context import get_cli_context
from snowflake.cli.api.identifiers import FQN
from snowflake.cli.api.sql_execution import SqlExecutionMixin
from snowflake.cli.api.stage_path import StagePath


class NotebookManager(SqlExecutionMixin):
Expand All @@ -40,8 +41,9 @@ def parse_stage_as_path(notebook_file: str) -> Path:
"""Parses notebook file path to pathlib.Path."""
if not notebook_file.endswith(".ipynb"):
raise NotebookStagePathError(notebook_file)
stage_path = Path(notebook_file)
if len(stage_path.parts) < 2:
# we don't perform any operations on the path, so we don't need to differentiate git repository paths
stage_path = StagePath.from_stage_str(notebook_file)
if len(stage_path.parts) < 1:
raise NotebookStagePathError(notebook_file)

return stage_path
Expand Down
13 changes: 12 additions & 1 deletion src/snowflake/cli/api/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,17 @@ def update(self, **updates):
raise KeyError(f"{key} is not a field of {self.__class__.__name__}")
setattr(self, key, value)

def merge_with_config(self, **updates) -> ConnectionContext:
"""
Updates missing fields from the config, but does not overwrite existing.
"""
field_map = {field.name for field in fields(self)}
for key, value in updates.items():
if key in field_map and getattr(self, key) is None:
setattr(self, key, value)

return self

def update_from_config(self) -> ConnectionContext:
connection_config = get_connection_dict(connection_name=self.connection_name)
if "private_key_path" in connection_config:
Expand All @@ -87,7 +98,7 @@ def update_from_config(self) -> ConnectionContext:
]
del connection_config["private_key_path"]

self.update(**connection_config)
self.merge_with_config(**connection_config)
return self

def __repr__(self) -> str:
Expand Down
1 change: 1 addition & 0 deletions src/snowflake/cli/api/errno.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 10 additions & 1 deletion tests/nativeapp/test_sf_sql_facade.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
):
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand Down
31 changes: 31 additions & 0 deletions tests/notebook/test_notebook_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

from unittest import mock

import pytest
import typer
from snowflake.cli._plugins.notebook.manager import NotebookManager
from snowflake.cli.api.identifiers import FQN
Expand Down Expand Up @@ -65,3 +66,33 @@ def test_create(mock_create, runner):
notebook_name=FQN.from_string("my_notebook"),
notebook_file=notebook_file,
)


@pytest.mark.parametrize(
"stage_path",
["@db.schema.stage", "@stage/dir/subdir", "@git_repo_stage/branch/main"],
)
@mock.patch("snowflake.connector.connect")
@mock.patch("snowflake.cli._plugins.notebook.manager.make_snowsight_url")
def test_create_query(
mock_make_snowsight_url, mock_connector, mock_ctx, runner, stage_path
):
ctx = mock_ctx()
mock_connector.return_value = ctx
mock_make_snowsight_url.return_value = "mocked_snowsight.url"
notebook_name = "my_notebook"
notebook_file = f"{stage_path}/notebook.ipynb"
result = runner.invoke(
["notebook", "create", notebook_name, "--notebook-file", notebook_file]
)
assert result.exit_code == 0, result.output
assert ctx.get_query() == (
"\n"
"CREATE OR REPLACE NOTEBOOK "
f"IDENTIFIER('MockDatabase.MockSchema.{notebook_name}')\n"
f"FROM '{stage_path}'\n"
"QUERY_WAREHOUSE = 'MockWarehouse'\n"
"MAIN_FILE = 'notebook.ipynb';\n"
"// Cannot use IDENTIFIER(...)\n"
f"ALTER NOTEBOOK MockDatabase.MockSchema.{notebook_name} ADD LIVE VERSION FROM LAST;\n"
)
43 changes: 39 additions & 4 deletions tests/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,6 @@ def test_connection_details_are_resolved_using_environment_variables(
mock_connect, env, test_snowcli_config, runner
):
with mock.patch.dict(os.environ, env, clear=True):

result = runner.invoke(["sql", "-q", "select 1", "-c", "empty"])

assert result.exit_code == 0, result.output
Expand Down Expand Up @@ -929,7 +928,6 @@ def test_flags_take_precedence_before_environment_variables(
mock_connect, env, test_snowcli_config, runner
):
with mock.patch.dict(os.environ, env, clear=True):

result = runner.invoke(
[
"sql",
Expand Down Expand Up @@ -1106,7 +1104,6 @@ def _run_connection_add_with_path_provided_as_prompt(
def test_new_connection_is_added_to_connections_toml(
runner, os_agnostic_snapshot, named_temporary_file, snowflake_home
):

connections_toml = Path(snowflake_home) / "connections.toml"
connections_toml.touch()
connections_toml.write_text(
Expand Down Expand Up @@ -1291,6 +1288,44 @@ def test_generate_jwt_uses_config(mocked_get_token, runner, named_temporary_file
)


@mock.patch(
"snowflake.cli._plugins.connection.commands.connector.auth.get_token_from_private_key"
)
@pytest.mark.parametrize(
"cmd_line_params, expected",
(
pytest.param(
("--user", "jdoe2"),
{"user": "jdoe2"},
id="--user flag",
),
pytest.param(
("--account", "account2"),
{"account": "account2"},
id="--account flag",
),
),
)
def test_generate_jwt_honors_params(
mocked_get_token, runner, cmd_line_params, expected
):
mocked_get_token.return_value = "funny token"

result = runner.invoke(
["connection", "generate-jwt", "--connection", "jwt", *cmd_line_params],
)

assert result.exit_code == 0, result.output
assert result.output == "funny token\n"
expected_params = {
"user": "jdoe",
"account": "testing_account",
"privatekey_path": "/private/key",
"key_password": None,
} | expected
mocked_get_token.assert_called_once_with(**expected_params)


@pytest.mark.parametrize("attribute", ["account", "user", "private_key_file"])
@mock.patch(
"snowflake.cli._plugins.connection.commands.connector.auth.get_token_from_private_key"
Expand All @@ -1315,7 +1350,7 @@ def test_generate_jwt_raises_error_if_required_parameter_is_missing(
["connection", "generate-jwt", "-c", "jwt"],
)
assert (
f'{attribute.capitalize().replace("_", " ")} is not set in the connection context'
f"{attribute.capitalize().replace('_', ' ')} is not set in the connection context"
in result.output
)

Expand Down
2 changes: 1 addition & 1 deletion tests_e2e/__snapshots__/test_installation.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

Usage: snow [OPTIONS] COMMAND [ARGS]...

Snowflake CLI tool for developers [v3.3.0rc0]
Snowflake CLI tool for developers [v3.3.0rc1]

+- Options --------------------------------------------------------------------+
| --version Shows version of the Snowflake CLI |
Expand Down

0 comments on commit 428ea09

Please sign in to comment.