Skip to content
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.

Sign up for GitHub

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

SNOW-1621834 Cast version to identifier when creating/dropping app versions #1475

Merged
merged 2 commits into from
Aug 23, 2024
Merged

SNOW-1621834 Cast version to identifier when creating/dropping app versions #1475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8de010e
Cast version to identifier
sfc-gh-fcampbell Aug 22, 2024
d9d50f2
Add to release notes
sfc-gh-fcampbell Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
* Fix the typo in spcs service name argument description. It is the identifier of the **service** instead of the **service pool**.
* Fix error handling and improve messaging when no artifacts provided.
* Improved error message for incompatible parameters.
* Fixed SQL error when running `snow app version create` and `snow app version drop` with a version name that isn't a valid Snowflake unquoted identifier


# v2.8.0
Expand Down
16 changes: 9 additions & 7 deletions src/snowflake/cli/_plugins/nativeapp/version/version_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,9 @@
from snowflake.cli.api.console import cli_console as cc
from snowflake.cli.api.exceptions import SnowflakeSQLExecutionError
from snowflake.cli.api.project.schemas.native_app.native_app import NativeApp
from snowflake.cli.api.project.util import unquote_identifier
from snowflake.cli.api.project.util import to_identifier, unquote_identifier
from snowflake.cli.api.utils.cursor import (
find_all_rows,
find_first_row,
)
from snowflake.connector import ProgrammingError
from snowflake.connector.cursor import DictCursor
Expand Down Expand Up @@ -121,6 +120,8 @@ def add_new_version(self, version: str) -> None:
"""
Defines a new version in an existing application package.
"""
# Make the version a valid identifier, adding quotes if necessary
version = to_identifier(version)
with self.use_role(self.package_role):
cc.step(
f"Defining a new version {version} in application package {self.package_name}"
Expand All @@ -141,6 +142,8 @@ def add_new_patch_to_version(self, version: str, patch: Optional[int] = None):
"""
Add a new patch, optionally a custom one, to an existing version in an application package.
"""
# Make the version a valid identifier, adding quotes if necessary
version = to_identifier(version)
with self.use_role(self.package_role):
cc.step(
f"Adding new patch to version {version} defined in application package {self.package_name}"
Expand All @@ -156,11 +159,7 @@ def add_new_patch_to_version(self, version: str, patch: Optional[int] = None):
add_version_query, cursor_class=DictCursor
)

show_row = find_first_row(
result_cursor,
lambda row: row[VERSION_COL] == unquote_identifier(version),
)
sfc-gh-cgorrie marked this conversation as resolved.
Show resolved Hide resolved

show_row = result_cursor.fetchall()[0]
new_patch = show_row["patch"]
cc.message(
f"Patch {new_patch} created for version {version} defined in application package {self.package_name}."
Expand Down Expand Up @@ -328,6 +327,9 @@ def process(
"Manifest.yml file does not contain a value for the version field."
)

# Make the version a valid identifier, adding quotes if necessary
version = to_identifier(version)
Copy link
Contributor

Choose a reason for hiding this comment

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

any way we can unify this to get_version() in manager or processor so we don't have to remember to do it every time, similar to get_app() or get_package() ?

Copy link
Contributor Author

@sfc-gh-fcampbell sfc-gh-fcampbell Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this also needs to handle the string passed in by the user, it's not part of the project model. I could change this to a CLI callback that casts it at that point though that wouldn't handle the version name in the manifest in case it's missing the quotes


cc.step(
dedent(
f"""\
Expand Down
37 changes: 25 additions & 12 deletions tests/nativeapp/test_version_create_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,11 @@ def test_get_existing_release_direction_info(mock_execute, temp_dir, mock_cursor

# Test add_new_version adds a new version to an app pkg correctly
@mock.patch(NATIVEAPP_MANAGER_EXECUTE)
def test_add_version(mock_execute, temp_dir, mock_cursor):
version = "V1"
@pytest.mark.parametrize(
["version", "version_identifier"],
[("V1", "V1"), ("1.0.0", '"1.0.0"'), ('"1.0.0"', '"1.0.0"')],
)
def test_add_version(mock_execute, temp_dir, mock_cursor, version, version_identifier):
side_effects, expected = mock_execute_helper(
[
(
Expand All @@ -120,7 +123,7 @@ def test_add_version(mock_execute, temp_dir, mock_cursor):
dedent(
f"""\
alter application package app_pkg
add version V1
add version {version_identifier}
using @app_pkg.app_src.stage
"""
),
Expand All @@ -146,8 +149,13 @@ def test_add_version(mock_execute, temp_dir, mock_cursor):

# Test add_new_patch_to_version adds an "auto-increment" patch to an existing version
@mock.patch(NATIVEAPP_MANAGER_EXECUTE)
def test_add_new_patch_auto(mock_execute, temp_dir, mock_cursor):
version = "V1"
@pytest.mark.parametrize(
["version", "version_identifier"],
[("V1", "V1"), ("1.0.0", '"1.0.0"'), ('"1.0.0"', '"1.0.0"')],
)
def test_add_new_patch_auto(
mock_execute, temp_dir, mock_cursor, version, version_identifier
):
side_effects, expected = mock_execute_helper(
[
(
Expand All @@ -161,7 +169,7 @@ def test_add_new_patch_auto(mock_execute, temp_dir, mock_cursor):
dedent(
f"""\
alter application package app_pkg
add patch for version V1
add patch for version {version_identifier}
using @app_pkg.app_src.stage
"""
),
Expand All @@ -187,8 +195,13 @@ def test_add_new_patch_auto(mock_execute, temp_dir, mock_cursor):

# Test add_new_patch_to_version adds a custom patch to an existing version
@mock.patch(NATIVEAPP_MANAGER_EXECUTE)
def test_add_new_patch_custom(mock_execute, temp_dir, mock_cursor):
version = "V1"
@pytest.mark.parametrize(
["version", "version_identifier"],
[("V1", "V1"), ("1.0.0", '"1.0.0"'), ('"1.0.0"', '"1.0.0"')],
)
def test_add_new_patch_custom(
mock_execute, temp_dir, mock_cursor, version, version_identifier
):
side_effects, expected = mock_execute_helper(
[
(
Expand All @@ -202,7 +215,7 @@ def test_add_new_patch_custom(mock_execute, temp_dir, mock_cursor):
dedent(
f"""\
alter application package app_pkg
add patch 12 for version V1
add patch 12 for version {version_identifier}
using @app_pkg.app_src.stage
"""
),
Expand All @@ -222,7 +235,7 @@ def test_add_new_patch_custom(mock_execute, temp_dir, mock_cursor):
)

processor = _get_version_create_processor()
processor.add_new_patch_to_version(version, "12")
processor.add_new_patch_to_version(version, 12)
assert mock_execute.mock_calls == expected


Expand Down Expand Up @@ -282,7 +295,7 @@ def test_process_no_version_exists_throws_bad_option_exception_one(
processor.process(
bundle_map=mock_bundle_map,
version="v1",
patch="12",
patch=12,
policy=policy_param,
git_policy=policy_param,
is_interactive=False,
Expand Down Expand Up @@ -315,7 +328,7 @@ def test_process_no_version_exists_throws_bad_option_exception_two(
processor.process(
bundle_map=mock_bundle_map,
version="v1",
patch="12",
patch=12,
policy=policy_param,
git_policy=policy_param,
is_interactive=False,
Expand Down
83 changes: 76 additions & 7 deletions tests/nativeapp/test_version_drop_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ def test_process_has_no_existing_app_pkg(mock_get_existing, policy_param, temp_d
def test_process_no_version_from_user_no_version_in_manifest(
mock_version_info_in_manifest,
mock_build_bundle,
mock_mismatch,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this was named that

mock_distribution,
mock_get_existing,
policy_param,
temp_dir,
):

mock_mismatch.return_Value = "internal"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this didn't break the test 😅

mock_distribution.return_value = "internal"
current_working_directory = os.getcwd()
create_named_file(
file_name="snowflake.yml",
Expand Down Expand Up @@ -144,15 +144,15 @@ def test_process_drop_cannot_complete(
mock_typer_confirm,
mock_version_info_in_manifest,
mock_build_bundle,
mock_mismatch,
mock_distribution,
mock_get_existing,
policy_param,
is_interactive_param,
expected_code,
temp_dir,
):

mock_mismatch.return_value = "internal"
mock_distribution.return_value = "internal"
current_working_directory = os.getcwd()
create_named_file(
file_name="snowflake.yml",
Expand Down Expand Up @@ -190,20 +190,20 @@ def test_process_drop_cannot_complete(
(ask_always_policy, True),
],
)
def test_process_drop_success(
def test_process_drop_from_manifest(
mock_typer_confirm,
mock_execute,
mock_version_info_in_manifest,
mock_build_bundle,
mock_mismatch,
mock_distribution,
mock_get_existing,
policy_param,
is_interactive_param,
temp_dir,
mock_cursor,
):

mock_mismatch.return_value = "internal"
mock_distribution.return_value = "internal"
side_effects, expected = mock_execute_helper(
[
(
Expand Down Expand Up @@ -234,3 +234,72 @@ def test_process_drop_success(
version=None, policy=policy_param, is_interactive=is_interactive_param
)
assert mock_execute.mock_calls == expected


@mock.patch(
f"{VERSION_MODULE}.{DROP_PROCESSOR}.get_existing_app_pkg_info",
return_value={"owner": "package_role"},
)
@mock_get_app_pkg_distribution_in_sf()
@mock.patch(f"{VERSION_MODULE}.{DROP_PROCESSOR}.build_bundle", return_value=None)
@mock.patch(NATIVEAPP_MANAGER_EXECUTE)
@mock.patch(
f"snowflake.cli._plugins.nativeapp.policy.{TYPER_CONFIRM}", return_value=True
)
@pytest.mark.parametrize(
"policy_param, is_interactive_param",
[
(allow_always_policy, False),
(ask_always_policy, True),
(ask_always_policy, True),
],
)
@pytest.mark.parametrize(
["version", "version_identifier"],
[("V1", "V1"), ("1.0.0", '"1.0.0"'), ('"1.0.0"', '"1.0.0"')],
)
def test_process_drop_specific_version(
mock_typer_confirm,
mock_execute,
mock_build_bundle,
mock_distribution,
mock_get_existing,
policy_param,
is_interactive_param,
temp_dir,
mock_cursor,
version,
version_identifier,
):

mock_distribution.return_value = "internal"
side_effects, expected = mock_execute_helper(
[
(
mock_cursor([{"CURRENT_ROLE()": "old_role"}], []),
mock.call("select current_role()", cursor_class=DictCursor),
),
(None, mock.call("use role package_role")),
(
None,
mock.call(
f"alter application package app_pkg drop version {version_identifier}"
),
),
(None, mock.call("use role old_role")),
]
)
mock_execute.side_effect = side_effects

current_working_directory = os.getcwd()
create_named_file(
file_name="snowflake.yml",
dir_name=current_working_directory,
contents=[mock_snowflake_yml_file],
)

processor = _get_version_drop_processor()
processor.process(
version=version, policy=policy_param, is_interactive=is_interactive_param
)
assert mock_execute.mock_calls == expected
Loading