Add optional subdirectory for stage in application package #1916

sfc-gh-pjafari · 2024-12-03T20:04:35Z

Pre-review checklist

I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
I've added or updated automated unit tests to verify correctness of my new code.
I've added or updated integration tests to verify correctness of my new code.
I've confirmed that my changes are working by executing CLI's commands manually on MacOS.
I've confirmed that my changes are working by executing CLI's commands manually on Windows.
I've confirmed that my changes are up-to-date with the target branch.
I've described my changes in the release notes.
I've described my changes in the section below.

Changes description

Added an optional stage_subdirectory to the application package.
This change means that the root of the stage is no longer the assumed path to finding the artifacts and the manifest.yml. If specified, the application package will only ever operate on stage/stage_subdirectory. This includes deploy, diff, and version create.

For detailed information on how this affects each command, if specified, refer to the design document.

…kedb/snowflake-cli into pj-subartifacts-subdirs

sfc-gh-pjafari · 2024-12-03T21:32:47Z

src/snowflake/cli/_plugins/nativeapp/commands.py

+    # TODO: get all apps created from this application package from snowflake, compare, confirm and drop.
+    # TODO: add messaging/confirmation here for extra apps found as part of above


Kept current teardown functionality. Next PR, we will get and drop all app objects created from this package, not just what's listed in pdf.

src/snowflake/cli/_plugins/nativeapp/codegen/templates/templates_processor.py

sfc-gh-pjafari · 2024-12-05T17:58:36Z

src/snowflake/cli/_plugins/nativeapp/commands.py

+    diff = ws.perform_action(
        package_id,
-        EntityActions.BUNDLE,
-    )
-    stage_fqn = f"{package.fqn.name}.{package.stage}"
-    diff: DiffResult = compute_stage_diff(
-        local_root=Path(package.deploy_root), stage_fqn=stage_fqn
+        EntityActions.DIFF,
+        print_to_console=cli_context.output_format != OutputFormat.JSON,
    )
    if cli_context.output_format == OutputFormat.JSON:
        return ObjectResult(diff.to_dict())
-    else:
-        print_diff_to_console(diff, bundle_map)
-        return None  # don't print any output
+
+    return None


Made diff its own entity action. action_diff

RELEASE-NOTES.md

src/snowflake/cli/_plugins/nativeapp/entities/application_package.py

sfc-gh-bdufour · 2024-12-05T18:48:27Z

src/snowflake/cli/_plugins/nativeapp/entities/application_package.py

    @property
-    def scratch_stage_fqn(self) -> str:
-        return f"{self.name}.{self._entity_model.scratch_stage}"
+    def stage_path(self) -> DefaultStagePathParts:


nit: should it be cached? I don't expect recreating is too expensive, but ...

sfc-gh-bdufour · 2024-12-05T19:12:37Z

src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py

@@ -592,7 +592,7 @@ def upgrade_application(

        @param name: Name of the application object
        @param install_method: Method of installing the application
-        @param stage_fqn: FQN of the stage housing the application artifacts
+        @param stage_path_to_artifacts: Path to directory in stage housing the application artifacts


nit: it's called version directory in the public docs

ah I debated using that but I didn't. updated.

tests/nativeapp/test_application_package_entity.py

tests/stage/test_diff.py

sfc-gh-melnacouzi · 2024-12-09T17:41:44Z

src/snowflake/cli/_plugins/nativeapp/commands.py

+        for package_entity in project.get_entities_by_type(
+            ApplicationPackageEntityModel.get_type()
+        ).values()
+        if package_entity.identifier == app_package_entity.identifier


Does this work with identifiers (quoted/unquoted)? or is this == overridden to do that, and if so, identifier could be String or Identifier.

alternatively, you can take a look into package_entity.fqn

good point, replaced with same_identifier

sfc-gh-melnacouzi · 2024-12-09T17:59:31Z

src/snowflake/cli/_plugins/nativeapp/entities/application_package.py

@@ -123,6 +128,11 @@ class ApplicationPackageEntityModel(EntityModelBase):
        default="",
    )

+    stage_subdirectory: Optional[str] = Field(
+        title="Subfolder in stage",


Can we elaborate a bit here? something along the line: "Subfolder in stage where artifacts are uploaded"?

sfc-gh-melnacouzi · 2024-12-09T18:01:25Z

src/snowflake/cli/_plugins/nativeapp/entities/application_package.py

+        return (
+            self.project_root
+            / self._entity_model.deploy_root
+            / self._entity_model.stage_subdirectory


why is this changing? Seems like an unnecessary complication

The deploy_root is now deploy_root/subdir. So all the actions (bundle, deploy, etc) that need to refer to deploy root to write/read artifacts look at this subdir.
I don't think I understand what you mean by unnecessary complication.

I mean why is the deploy root now deploy_root/subdir?

sfc-gh-melnacouzi · 2024-12-09T18:04:21Z

src/snowflake/cli/_plugins/nativeapp/entities/application_package.py

-    def scratch_stage_fqn(self) -> str:
-        return f"{self.name}.{self._entity_model.scratch_stage}"
+    @cached_property
+    def stage_path(self) -> DefaultStagePathParts:


minor: I know DefaultStagePathParts existed before - but would have been simpler if named something like "StageLocation" or "StagePath"

sfc-gh-melnacouzi · 2024-12-09T18:07:01Z

src/snowflake/cli/_plugins/nativeapp/entities/application_package.py

+        stage_fqn = f"{self.name}.{self._entity_model.stage}"
+        subdir = self._entity_model.stage_subdirectory
+        full_path = f"{stage_fqn}/{subdir}" if subdir else stage_fqn
+        return DefaultStagePathParts(full_path)


we could also accept different constructors in DefaultStagePathParts, so DefaultStagePathParts to undo the concat.

sfc-gh-melnacouzi · 2024-12-09T18:18:01Z

src/snowflake/cli/_plugins/stage/diff.py

+    """
+    @param path: file path on the stage.
+    @param stage_subirectory: subdirectory of stage.
+    @return: path of file relative to the stage_path


there is no stage_path in input.

sfc-gh-melnacouzi · 2024-12-09T18:19:32Z

src/snowflake/cli/_plugins/stage/diff.py

+    wo_stage_name = StagePathType(*path.split("/")[1:])
+    relative_path = str(wo_stage_name).removeprefix(stage_subirectory).lstrip("/")


can't tell what these are doing. I see the first command removing first part left of /, and the second line doing something similar.

Adding comments might help.

Curious if Path can somehow be leveraged to calculate relative path instead?

Added comment and updated method.
Can't use Path at all since stage name can be a quoted identifier which can have any character including a "/" in its name

sfc-gh-melnacouzi · 2024-12-09T18:21:05Z

src/snowflake/cli/_plugins/stage/diff.py

    """
-    Returns a mapping of relative stage paths to their md5sums.
+    Returns a mapping of file paths to their md5sums. File paths are relative to the stage_path.


ditto about stage_path

done, thank you

sfc-gh-melnacouzi · 2024-12-09T18:21:45Z

src/snowflake/cli/_plugins/stage/diff.py

    """
-    Diffs the files in a stage with a local folder.
+    Diffs the files in the local_root with files in the stage path that is stage_path_parts's full_path.


stage_path_parts?

sfc-gh-melnacouzi · 2024-12-09T18:26:57Z

src/snowflake/cli/_plugins/stage/manager.py

-        stage_name = self.stage.split(".")[-1]
+        match = re.fullmatch(STAGE_PATH_REGEX, stage_path)
+        if match is None:
+            raise ClickException("Invalid stage path")


this validation should be done in Pydantic model. For users, this might not be clear where this exception is coming from.

we have validations in the pydantic model for NA. this is for other uses of this stage object.

sfc-gh-melnacouzi · 2024-12-09T19:32:40Z

tests_integration/test_data/projects/napp_stage_subdirs/snowflake.yml

@@ -0,0 +1,31 @@
+definition_version: 2


can't we use PDF factory instead of these?

you caught me haha
I didn't want to re-write a bunch of our tests or add more fixtures to accommodate them. I understand that is not the greatest pattern but sadly the factories work way better for unit tests and have been harder to adopt for integ tests.

I am not asking to rewrite existing tests. New tests added 2 new directories (napp_stage_subdirs*). We shouldn't add any more of these test directories if the factory can do that.

src/snowflake/cli/_plugins/nativeapp/commands.py

sfc-gh-melnacouzi · 2024-12-13T19:17:19Z

tests_integration/test_data/projects/napp_stage_subdirs/snowflake.yml

@@ -0,0 +1,31 @@
+definition_version: 2


I am not asking to rewrite existing tests. New tests added 2 new directories (napp_stage_subdirs*). We shouldn't add any more of these test directories if the factory can do that.

sfc-gh-melnacouzi · 2024-12-13T19:20:16Z

src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py

@@ -254,15 +255,15 @@ def get_account_event_table(self, role: str | None = None) -> str | None:
    def create_version_in_package(
        self,
        package_name: str,
-        stage_fqn: str,
+        path_to_version_directory: str,


i am confused about the naming here. Why we mention version_directory here (while the rest uses stage_subdirectory), and doesn't this also include stage info? Is stage_fqn not accurate anymore? Or stage_with_subdirectory? or StageParts class?

or stage_full_path

path_to_version_directory is the naming used in the official docs for these two operations, so the facade naming is matching that.

sfc-gh-pjafari added 17 commits November 29, 2024 14:59

teardown with comments

8617252

make version work

5a3e154

update path for version create

8db7a40

catch tempalte processor file read error

d9eb5f4

make diff an entity action

71108d0

remove teardown with all apps logic

a7e3a20

ws change

99abebf

add schema to StagePathParts

ce71ebf

use DefaultStagePathParts for stage in NA

855b413

remmove comments

68904aa

clean up

5fc642e

clean up

49275b9

update comment

04d807e

merge main

be0402b

rename stage

ff369e7

remove trailing slash when no directory

31e1f70

update unit tests

f52c6cb

sfc-gh-pjafari changed the title ~~Pj subartifacts subdirs~~ subdirs Dec 3, 2024

sfc-gh-pjafari added 2 commits December 3, 2024 16:43

update docstring

e95ab86

add unit tests for DefaultStagePathParts

b663798

sfc-gh-pjafari mentioned this pull request Dec 4, 2024

Add schema getter to StagePathParts #1915

Merged

8 tasks

sfc-gh-pjafari added 9 commits December 4, 2024 10:53

Merge branch 'pj-schema-in-stagepathparts' into pj-subartifacts-subdirs

3412b3c

update stage diff command and message wording

08a4110

update snapshot for test_deploy

c7a0a88

Merge branch 'main' into pj-subartifacts-subdirs

8153807

add integration tests

504f692

Merge branch 'main' into pj-subartifacts-subdirs

5cbf941

iMerge branch 'pj-subartifacts-subdirs' of https://github.com/snowfla…

56fc4f6

…kedb/snowflake-cli into pj-subartifacts-subdirs

add unit tests

b81670c

update release notes

889c77e

sfc-gh-pjafari marked this pull request as ready for review December 5, 2024 16:35

sfc-gh-pjafari requested review from a team as code owners December 5, 2024 16:35

sfc-gh-pjafari changed the title ~~subdirs~~ Add optional subdirectory for stage in application package Dec 5, 2024

sfc-gh-pjafari commented Dec 5, 2024

View reviewed changes

sfc-gh-pjafari changed the base branch from main to pj-template-processor-error December 5, 2024 18:15

remove template processor change

44263ce

sfc-gh-pjafari changed the base branch from pj-template-processor-error to main December 5, 2024 18:33

sfc-gh-bdufour reviewed Dec 5, 2024

View reviewed changes

sfc-gh-pjafari added 3 commits December 6, 2024 17:01

make stage paths work for quoted ids

70e3d38

address comments

c0b0826

make for quoted identifiers in diff

4ec1071

sfc-gh-melnacouzi reviewed Dec 9, 2024

View reviewed changes

sfc-gh-pjafari added 5 commits December 12, 2024 09:27

merge main

00db5a8

make diff work with all identifiers, update docstrings

7f30fea

update tests

1921805

merge main

3f9ed94

fix tests

d84a59c

sfc-gh-melnacouzi reviewed Dec 13, 2024

View reviewed changes

sfc-gh-pjafari added 4 commits December 16, 2024 19:20

update commands

d865ac8

merge main

dfb4373

replace test data with factory

aa80ac4

import fixture

a225d44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional subdirectory for stage in application package #1916

Add optional subdirectory for stage in application package #1916

sfc-gh-pjafari commented Dec 3, 2024 •

edited

Loading

sfc-gh-pjafari Dec 3, 2024

sfc-gh-pjafari Dec 5, 2024

sfc-gh-bdufour Dec 5, 2024

sfc-gh-bdufour Dec 5, 2024

sfc-gh-pjafari Dec 6, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-pjafari Dec 13, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-pjafari Dec 12, 2024

sfc-gh-melnacouzi Dec 13, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-pjafari Dec 13, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-pjafari Dec 13, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-pjafari Dec 13, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-pjafari Dec 13, 2024

sfc-gh-melnacouzi Dec 9, 2024

sfc-gh-pjafari Dec 13, 2024

sfc-gh-melnacouzi Dec 13, 2024

sfc-gh-melnacouzi Dec 13, 2024

sfc-gh-melnacouzi Dec 13, 2024

sfc-gh-melnacouzi Dec 13, 2024

sfc-gh-pjafari Dec 17, 2024

		# TODO: get all apps created from this application package from snowflake, compare, confirm and drop.
		# TODO: add messaging/confirmation here for extra apps found as part of above

		wo_stage_name = StagePathType(*path.split("/")[1:])
		relative_path = str(wo_stage_name).removeprefix(stage_subirectory).lstrip("/")

Add optional subdirectory for stage in application package #1916

Are you sure you want to change the base?

Add optional subdirectory for stage in application package #1916

Conversation

sfc-gh-pjafari commented Dec 3, 2024 • edited Loading

Pre-review checklist

Changes description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-pjafari commented Dec 3, 2024 •

edited

Loading