Skip to content

Commit

Permalink
Disable snowflake.local.yml migration by default, add flag to enable …
Browse files Browse the repository at this point in the history
…it (#1636)

Fixes bug with `snow helpers v1-to-v2` if a `snowflake.local.yml` is present. Currently, the local file isn't renamed, so a v1 local can be overlaid onto a v2 main definition file after migration, which breaks subsequent commands. Since local overrides are no longer a desired feature in PDFv2, we don't convert them unless `--migrate-local-overrides` is used.
  • Loading branch information
sfc-gh-fcampbell authored and sfc-gh-jsikorski committed Sep 30, 2024
1 parent be825bb commit 0d126bd
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 8 deletions.
29 changes: 29 additions & 0 deletions src/snowflake/cli/_plugins/helpers/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import typer
import yaml
from click import ClickException
from snowflake.cli.api.commands.snow_typer import SnowTyperFactory
from snowflake.cli.api.output.types import MessageResult
from snowflake.cli.api.project.definition_conversion import (
Expand All @@ -35,10 +36,36 @@ def v1_to_v2(
accept_templates: bool = typer.Option(
False, "-t", "--accept-templates", help="Allows the migration of templates."
),
migrate_local_yml: (bool | None) = typer.Option(
None,
"-l",
"--migrate-local-overrides/--no-migrate-local-overrides",
help=(
"Merge values in snowflake.local.yml into the main project definition. "
"The snowflake.local.yml file will not be migrated, "
"instead its values will be reflected in the output snowflake.yml file. "
"If unset and snowflake.local.yml is present, an error will be raised."
),
show_default=False,
),
**options,
):
"""Migrates the Snowpark, Streamlit, and Native App project definition files from V1 to V2."""
manager = DefinitionManager()
local_yml_path = manager.project_root / "snowflake.local.yml"
has_local_yml = local_yml_path in manager.project_config_paths
if has_local_yml:
if migrate_local_yml is None:
raise ClickException(
"snowflake.local.yml file detected, "
"please specify --migrate-local-overrides to include "
"or --no-migrate-local-overrides to exclude its values."
)
if not migrate_local_yml:
# If we don't want the local file,
# remove it from the list of paths to load
manager.project_config_paths.remove(local_yml_path)

pd = manager.unrendered_project_definition

if pd.meets_version_requirement("2"):
Expand All @@ -49,6 +76,8 @@ def v1_to_v2(
)

SecurePath("snowflake.yml").rename("snowflake_V1.yml")
if has_local_yml:
SecurePath("snowflake.local.yml").rename("snowflake_V1.local.yml")
with open("snowflake.yml", "w") as file:
yaml.dump(
pd_v2.model_dump(
Expand Down
10 changes: 5 additions & 5 deletions src/snowflake/cli/api/project/definition_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class DefinitionManager:
USER_DEFINITION_FILENAME = "snowflake.local.yml"

project_root: Path
_project_config_paths: List[Path]
project_config_paths: List[Path]

def __init__(
self,
Expand All @@ -53,12 +53,12 @@ def __init__(
)

self.project_root = project_root
self._project_config_paths = self._find_definition_files(self.project_root)
self.project_config_paths = self._find_definition_files(self.project_root)
self._context_overrides = context_overrides

@functools.cached_property
def has_definition_file(self):
return len(self._project_config_paths) > 0
return len(self.project_config_paths) > 0

@staticmethod
def _find_definition_files(project_root: Path) -> List[Path]:
Expand Down Expand Up @@ -126,11 +126,11 @@ def _user_definition_file_if_available(project_path: Path) -> Optional[Path]:

@functools.cached_property
def _project_properties(self) -> ProjectProperties:
return load_project(self._project_config_paths, self._context_overrides)
return load_project(self.project_config_paths, self._context_overrides)

@functools.cached_property
def _raw_project_data(self) -> ProjectProperties:
return load_project(self._project_config_paths, {}, False)
return load_project(self.project_config_paths, {}, False)

@functools.cached_property
def project_definition(self) -> ProjectDefinitionV1:
Expand Down
29 changes: 28 additions & 1 deletion tests/helpers/test_v1_to_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def test_if_template_raises_error_during_migrations(

def test_migration_with_only_envs(project_directory, runner):
with project_directory("sql_templating"):
result = runner.invoke(["helpers", "v1-to-v2"])
result = runner.invoke(["helpers", "v1-to-v2", "--no-migrate-local-overrides"])

assert result.exit_code == 0

Expand Down Expand Up @@ -151,3 +151,30 @@ def test_migrating_a_file_with_duplicated_keys_raises_an_error(
result = runner.invoke(["helpers", "v1-to-v2"])
assert result.exit_code == 1, result.output
assert result.output == os_agnostic_snapshot


@pytest.mark.parametrize("migrate_local_yml", [True, False])
def test_migrating_with_local_yml(
runner, project_directory, os_agnostic_snapshot, migrate_local_yml
):
with project_directory("migration_local_yml"):
flag = (
"--migrate-local-overrides"
if migrate_local_yml
else "--no-migrate-local-overrides"
)
result = runner.invoke(["helpers", "v1-to-v2", flag])
assert result.exit_code == 0, result.output
assert Path("snowflake_V1.local.yml").exists()
with Path("snowflake.yml").open() as f:
pdf = yaml.safe_load(f)
assert pdf["env"]["foo"] == "bar_local" if migrate_local_yml else "bar"


def test_migrating_with_local_yml_no_flag(
runner, project_directory, os_agnostic_snapshot
):
with project_directory("migration_local_yml"):
result = runner.invoke(["helpers", "v1-to-v2"])
assert result.exit_code == 1, result.output
assert "please specify --migrate-local-overrides" in result.output
4 changes: 2 additions & 2 deletions tests/project/test_definition_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class DefinitionManagerTest(TestCase):
def test_no_project_parameter_provided(self, mock_getcwd):
with mock_is_file_for("/hello/world/snowflake.yml") as mock_is_file:
definition_manager = DefinitionManager()
assert definition_manager._project_config_paths == [ # noqa: SLF001
assert definition_manager.project_config_paths == [
Path("/hello/world/snowflake.yml")
]

Expand All @@ -48,7 +48,7 @@ def test_finds_local_project_definition(self, mock_getcwd):
"/hello/world/snowflake.yml", "/hello/world/snowflake.local.yml"
) as mock_is_file:
definition_manager = DefinitionManager()
assert definition_manager._project_config_paths == [ # noqa: SLF001
assert definition_manager.project_config_paths == [
Path("/hello/world/snowflake.yml"),
Path("/hello/world/snowflake.local.yml"),
]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
definition_version: 1.1
env:
foo: bar_local
3 changes: 3 additions & 0 deletions tests/test_data/projects/migration_local_yml/snowflake.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
definition_version: 1.1
env:
foo: bar

0 comments on commit 0d126bd

Please sign in to comment.