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.

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-1643030 Append username template for v1.0 PDFs #1556

Merged
merged 4 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
48 changes: 35 additions & 13 deletions src/snowflake/cli/api/project/definition_conversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -206,14 +209,19 @@ 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 <% %>
# Luckily, package scripts only support {{ package_name }}, so let's convert that tag
# 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:
Expand All @@ -225,13 +233,20 @@ 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
and native_app.package.name != PackageV11.model_fields["name"].default
):
package_identifier = native_app.package.name
else:
# 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)"
Copy link
Contributor

@sfc-gh-cgorrie sfc-gh-cgorrie Sep 13, 2024

Choose a reason for hiding this comment

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

Nit: would be helpful to extract this naming logic out. Consider putting it near the original implementation (i.e. in api/project/util)

)
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,
Expand All @@ -254,13 +269,20 @@ 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
and native_app.application.name != ApplicationV11.model_fields["name"].default
):
app_identifier = native_app.application.name
else:
# 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": (
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:
Expand Down
12 changes: 2 additions & 10 deletions tests/workspace/__snapshots__/test_manager.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
+------------------------------------------------------------------------------+

Comment on lines -171 to -177
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just removed this unused snapshot

'''
# ---
# name: test_migrations_with_multiple_entities
'''
definition_version: '2'
Expand Down Expand Up @@ -226,7 +217,8 @@
deploy_root: output/deploy/
distribution: external
generated_root: __generated/
identifier: myapp_pkg
identifier: <% fn.concat_ids('myapp', '_pkg_', fn.sanitize_id(fn.get_username('unknown_user'))
| lower) %>
manifest: app/manifest.yml
meta:
role: pkg_role
Expand Down
28 changes: 28 additions & 0 deletions tests/workspace/test_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,3 +221,31 @@ 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


@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:
old_definition = yaml.safe_load(f)
old_definition["definition_version"] = definition_version
f.seek(0)
yaml.safe_dump(old_definition, f)
f.truncate()

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)
assert (
new_definition["entities"]["app"]["identifier"]
== "<% fn.concat_ids('integration', '_', fn.sanitize_id(fn.get_username('unknown_user')) | lower) %>"
)
assert (
new_definition["entities"]["pkg"]["identifier"]
== "<% fn.concat_ids('integration', '_pkg_', fn.sanitize_id(fn.get_username('unknown_user')) | lower) %>"
)
Loading