From 3ee387e6774925695052b0faba69db383283e374 Mon Sep 17 00:00:00 2001 From: Francois Campbell Date: Tue, 10 Sep 2024 13:48:29 -0400 Subject: [PATCH 1/3] Append username template for v1.0 PDFs --- .../cli/api/project/definition_conversion.py | 41 +++++++++++++------ .../workspace/__snapshots__/test_manager.ambr | 11 +---- tests/workspace/test_manager.py | 29 +++++++++++++ 3 files changed, 58 insertions(+), 23 deletions(-) diff --git a/src/snowflake/cli/api/project/definition_conversion.py b/src/snowflake/cli/api/project/definition_conversion.py index 1d36ec34a..db960cbc6 100644 --- a/src/snowflake/cli/api/project/definition_conversion.py +++ b/src/snowflake/cli/api/project/definition_conversion.py @@ -46,7 +46,9 @@ def convert_project_definition_to_v2( snowpark_data = convert_snowpark_to_v2_data(pd.snowpark) if pd.snowpark else {} streamlit_data = convert_streamlit_to_v2_data(pd.streamlit) if pd.streamlit else {} native_app_data = ( - convert_native_app_to_v2_data(project_root, pd.native_app) + convert_native_app_to_v2_data( + project_root, pd.definition_version, pd.native_app + ) if pd.native_app else {} ) @@ -166,7 +168,7 @@ def convert_streamlit_to_v2_data(streamlit: Streamlit) -> Dict[str, Any]: def convert_native_app_to_v2_data( - project_root, native_app: NativeApp + project_root, definition_version: str | int, native_app: NativeApp ) -> Dict[str, Any]: def _make_meta(obj: Application | Package): meta = {} @@ -206,6 +208,9 @@ def _find_manifest(): # which use POSIX paths as default values return manifest_path.as_posix() + def _make_template(template: str) -> str: + return f"{PROJECT_TEMPLATE_VARIABLE_OPENING} {template} {PROJECT_TEMPLATE_VARIABLE_CLOSING}" + def _convert_package_script_files(package_scripts: list[str]): # PDFv2 doesn't support package scripts, only post-deploy scripts, so we # need to convert the Jinja syntax from {{ }} to <% %> @@ -213,7 +218,9 @@ def _convert_package_script_files(package_scripts: list[str]): # to v2 template syntax by running it though the template process with a fake # package name that's actually a valid v2 template, which will be evaluated # when the script is used as a post-deploy script - fake_package_replacement_template = f"{PROJECT_TEMPLATE_VARIABLE_OPENING} ctx.entities.{package_entity_name}.identifier {PROJECT_TEMPLATE_VARIABLE_CLOSING}" + fake_package_replacement_template = _make_template( + f"ctx.entities.{package_entity_name}.identifier" + ) jinja_context = dict(package_name=fake_package_replacement_template) post_deploy_hooks = [] for script_file in package_scripts: @@ -225,13 +232,17 @@ def _convert_package_script_files(package_scripts: list[str]): return post_deploy_hooks package_entity_name = "pkg" + if native_app.package and native_app.package.name: + package_identifier = native_app.package.name + else: + package_identifier = f"{native_app.name}_pkg" + if str(definition_version) == "1": + # PDFv1.0 doesn't have the username append in the definition object, + # it's done in the NativeAppProjectModel, so we have to emulate that here + package_identifier += f"_{_make_template('ctx.env.USERNAME')}" package = { "type": "application package", - "identifier": ( - native_app.package.name - if native_app.package and native_app.package.name - else f"{native_app.name}_pkg" - ), + "identifier": package_identifier, "manifest": _find_manifest(), "artifacts": native_app.artifacts, "bundle_root": native_app.bundle_root, @@ -254,13 +265,17 @@ def _convert_package_script_files(package_scripts: list[str]): package["meta"] = package_meta app_entity_name = "app" + if native_app.application and native_app.application.name: + app_identifier = native_app.application.name + else: + app_identifier = native_app.name + if str(definition_version) == "1": + # PDFv1.0 doesn't have the username append in the definition object, + # it's done in the NativeAppProjectModel, so we have to emulate that here + app_identifier += f"_{_make_template('ctx.env.USERNAME')}" app = { "type": "application", - "identifier": ( - native_app.application.name - if native_app.application and native_app.application.name - else native_app.name - ), + "identifier": app_identifier, "from": {"target": package_entity_name}, } if native_app.application: diff --git a/tests/workspace/__snapshots__/test_manager.ambr b/tests/workspace/__snapshots__/test_manager.ambr index f76fe996d..598345279 100644 --- a/tests/workspace/__snapshots__/test_manager.ambr +++ b/tests/workspace/__snapshots__/test_manager.ambr @@ -168,15 +168,6 @@ ''' # --- -# name: test_migrating_native_app_raises_error - ''' - +- Error ----------------------------------------------------------------------+ - | Your project file contains a native app definition. Conversion of Native | - | apps is not yet supported | - +------------------------------------------------------------------------------+ - - ''' -# --- # name: test_migrations_with_multiple_entities ''' definition_version: '2' @@ -226,7 +217,7 @@ deploy_root: output/deploy/ distribution: external generated_root: __generated/ - identifier: myapp_pkg + identifier: myapp_pkg_<% ctx.env.USERNAME %> manifest: app/manifest.yml meta: role: pkg_role diff --git a/tests/workspace/test_manager.py b/tests/workspace/test_manager.py index 30c1e4683..6980b3f14 100644 --- a/tests/workspace/test_manager.py +++ b/tests/workspace/test_manager.py @@ -22,6 +22,10 @@ import pytest import yaml from snowflake.cli._plugins.workspace.manager import WorkspaceManager +from snowflake.cli.api.constants import ( + PROJECT_TEMPLATE_VARIABLE_CLOSING, + PROJECT_TEMPLATE_VARIABLE_OPENING, +) from snowflake.cli.api.entities.common import EntityActions from snowflake.cli.api.exceptions import InvalidProjectDefinitionVersionError from snowflake.cli.api.project.definition_manager import DefinitionManager @@ -221,3 +225,28 @@ def test_migrating_a_file_with_duplicated_keys_raises_an_error( result = runner.invoke(["ws", "migrate"]) assert result.exit_code == 1 assert result.output == os_agnostic_snapshot + + +def test_migrate_nativeapp_fields_with_username_v1_definition( + runner, project_directory +): + with project_directory("integration") as pd: + definition_path = pd / "snowflake.yml" + with definition_path.open("r") as f: + old_definition = yaml.safe_load(f) + assert old_definition["definition_version"] == 1 + + result = runner.invoke(["ws", "migrate"]) + assert result.exit_code == 0, result.output + + with definition_path.open("r") as f: + new_definition = yaml.safe_load(f) + username_suffix = f"_{PROJECT_TEMPLATE_VARIABLE_OPENING} ctx.env.USERNAME {PROJECT_TEMPLATE_VARIABLE_CLOSING}" + assert ( + new_definition["entities"]["app"]["identifier"] + == f"integration{username_suffix}" + ) + assert ( + new_definition["entities"]["pkg"]["identifier"] + == f"integration_pkg{username_suffix}" + ) From 0afb93e4404cbb303fa7f1d183a8942d2f329183 Mon Sep 17 00:00:00 2001 From: Francois Campbell Date: Tue, 10 Sep 2024 15:57:06 -0400 Subject: [PATCH 2/3] Treat 1.0 and 1.1 equally --- .../cli/api/project/definition_conversion.py | 37 ++++++++++++------- .../workspace/__snapshots__/test_manager.ambr | 3 +- tests/workspace/test_manager.py | 23 ++++++------ 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/snowflake/cli/api/project/definition_conversion.py b/src/snowflake/cli/api/project/definition_conversion.py index db960cbc6..b2675f316 100644 --- a/src/snowflake/cli/api/project/definition_conversion.py +++ b/src/snowflake/cli/api/project/definition_conversion.py @@ -20,9 +20,12 @@ from snowflake.cli.api.project.schemas.entities.common import ( SqlScriptHookType, ) -from snowflake.cli.api.project.schemas.native_app.application import Application +from snowflake.cli.api.project.schemas.native_app.application import ( + Application, + ApplicationV11, +) from snowflake.cli.api.project.schemas.native_app.native_app import NativeApp -from snowflake.cli.api.project.schemas.native_app.package import Package +from snowflake.cli.api.project.schemas.native_app.package import Package, PackageV11 from snowflake.cli.api.project.schemas.project_definition import ( ProjectDefinition, ProjectDefinitionV2, @@ -232,14 +235,17 @@ def _convert_package_script_files(package_scripts: list[str]): return post_deploy_hooks package_entity_name = "pkg" - if native_app.package and native_app.package.name: + if ( + native_app.package + and native_app.package.name + and native_app.package.name != PackageV11.model_fields["name"].default + ): package_identifier = native_app.package.name else: - package_identifier = f"{native_app.name}_pkg" - if str(definition_version) == "1": - # PDFv1.0 doesn't have the username append in the definition object, - # it's done in the NativeAppProjectModel, so we have to emulate that here - package_identifier += f"_{_make_template('ctx.env.USERNAME')}" + # Backport the PackageV11 default name template, updated for PDFv2 + package_identifier = _make_template( + f"fn.concat_ids('{native_app.name}', '_pkg_', fn.sanitize_id(fn.get_username('unknown_user')) | lower)" + ) package = { "type": "application package", "identifier": package_identifier, @@ -265,14 +271,17 @@ def _convert_package_script_files(package_scripts: list[str]): package["meta"] = package_meta app_entity_name = "app" - if native_app.application and native_app.application.name: + if ( + native_app.application + and native_app.application.name + and native_app.application.name != ApplicationV11.model_fields["name"].default + ): app_identifier = native_app.application.name else: - app_identifier = native_app.name - if str(definition_version) == "1": - # PDFv1.0 doesn't have the username append in the definition object, - # it's done in the NativeAppProjectModel, so we have to emulate that here - app_identifier += f"_{_make_template('ctx.env.USERNAME')}" + # Backport the ApplicationV11 default name template, updated for PDFv2 + app_identifier = _make_template( + f"fn.concat_ids('{native_app.name}', '_', fn.sanitize_id(fn.get_username('unknown_user')) | lower)" + ) app = { "type": "application", "identifier": app_identifier, diff --git a/tests/workspace/__snapshots__/test_manager.ambr b/tests/workspace/__snapshots__/test_manager.ambr index 598345279..913229f45 100644 --- a/tests/workspace/__snapshots__/test_manager.ambr +++ b/tests/workspace/__snapshots__/test_manager.ambr @@ -217,7 +217,8 @@ deploy_root: output/deploy/ distribution: external generated_root: __generated/ - identifier: myapp_pkg_<% ctx.env.USERNAME %> + identifier: <% fn.concat_ids('myapp', '_pkg_', fn.sanitize_id(fn.get_username('unknown_user')) + | lower) %> manifest: app/manifest.yml meta: role: pkg_role diff --git a/tests/workspace/test_manager.py b/tests/workspace/test_manager.py index 6980b3f14..a95425f30 100644 --- a/tests/workspace/test_manager.py +++ b/tests/workspace/test_manager.py @@ -22,10 +22,6 @@ import pytest import yaml from snowflake.cli._plugins.workspace.manager import WorkspaceManager -from snowflake.cli.api.constants import ( - PROJECT_TEMPLATE_VARIABLE_CLOSING, - PROJECT_TEMPLATE_VARIABLE_OPENING, -) from snowflake.cli.api.entities.common import EntityActions from snowflake.cli.api.exceptions import InvalidProjectDefinitionVersionError from snowflake.cli.api.project.definition_manager import DefinitionManager @@ -227,26 +223,29 @@ def test_migrating_a_file_with_duplicated_keys_raises_an_error( assert result.output == os_agnostic_snapshot -def test_migrate_nativeapp_fields_with_username_v1_definition( - runner, project_directory +@pytest.mark.parametrize("definition_version", [1, "1.1"]) +def test_migrate_nativeapp_fields_with_username( + runner, project_directory, definition_version ): with project_directory("integration") as pd: definition_path = pd / "snowflake.yml" - with definition_path.open("r") as f: + with definition_path.open("r+") as f: old_definition = yaml.safe_load(f) - assert old_definition["definition_version"] == 1 + old_definition["definition_version"] = definition_version + f.seek(0) + yaml.safe_dump(old_definition, f) + f.truncate() - result = runner.invoke(["ws", "migrate"]) + result = runner.invoke(["ws", "migrate", "--accept-templates"]) assert result.exit_code == 0, result.output with definition_path.open("r") as f: new_definition = yaml.safe_load(f) - username_suffix = f"_{PROJECT_TEMPLATE_VARIABLE_OPENING} ctx.env.USERNAME {PROJECT_TEMPLATE_VARIABLE_CLOSING}" assert ( new_definition["entities"]["app"]["identifier"] - == f"integration{username_suffix}" + == "<% fn.concat_ids('integration', '_', fn.sanitize_id(fn.get_username('unknown_user')) | lower) %>" ) assert ( new_definition["entities"]["pkg"]["identifier"] - == f"integration_pkg{username_suffix}" + == "<% fn.concat_ids('integration', '_pkg_', fn.sanitize_id(fn.get_username('unknown_user')) | lower) %>" ) From 1d7b7575c7828aa184bfebab2eba62bd9e3da932 Mon Sep 17 00:00:00 2001 From: Francois Campbell Date: Fri, 13 Sep 2024 09:53:05 -0400 Subject: [PATCH 3/3] Don't need definition version --- src/snowflake/cli/api/project/definition_conversion.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/snowflake/cli/api/project/definition_conversion.py b/src/snowflake/cli/api/project/definition_conversion.py index b2675f316..af2a98360 100644 --- a/src/snowflake/cli/api/project/definition_conversion.py +++ b/src/snowflake/cli/api/project/definition_conversion.py @@ -49,9 +49,7 @@ def convert_project_definition_to_v2( snowpark_data = convert_snowpark_to_v2_data(pd.snowpark) if pd.snowpark else {} streamlit_data = convert_streamlit_to_v2_data(pd.streamlit) if pd.streamlit else {} native_app_data = ( - convert_native_app_to_v2_data( - project_root, pd.definition_version, pd.native_app - ) + convert_native_app_to_v2_data(project_root, pd.native_app) if pd.native_app else {} ) @@ -171,7 +169,7 @@ def convert_streamlit_to_v2_data(streamlit: Streamlit) -> Dict[str, Any]: def convert_native_app_to_v2_data( - project_root, definition_version: str | int, native_app: NativeApp + project_root, native_app: NativeApp ) -> Dict[str, Any]: def _make_meta(obj: Application | Package): meta = {}