Append SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX value to app and package name #1443

sfc-gh-fcampbell · 2024-08-13T19:15:19Z

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

Prerequisite for #1437. After discussing in-person, we decided to not overwrite the USER environment variable in tests and instead add a new SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX env var that will be appended to app package and app identifiers. In #1437, we'll use this variable to generate unique names for these resources so they don't clash during concurrent tests.

sfc-gh-fcampbell · 2024-08-13T19:16:09Z

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

@@ -129,12 +134,13 @@ def project_identifier(self) -> str:
        # sometimes strip out double quotes, so we try to get them back here.
        return to_identifier(self.definition.name)

-    @cached_property
+    @property


removed caching to avoid caching the suffix in tests, since it will contain an ID that should be unique for each test

sfc-gh-melnacouzi · 2024-08-13T20:10:12Z

RELEASE-NOTES.md

@@ -31,6 +31,7 @@
  * Currently only supports SQL scripts: `post_deploy: [{sql_script: script.sql}]`
 * Added `snow spcs service execute-job` command, which supports creating and executing a job service in the current schema.
 * Added `snow app events` command to fetch logs and traces from local and customer app installations
+* Added support for `SNOWFLAKE_CLI_RESOURCE_SUFFIX` environment variable to be used as a suffix for Native App package and app names


not sure what's the common practice on this. Since this is purely internal for testing purposes, should we skip it from release notes?

i added it cause it could technically be used by end-users. I'm ok leaving it off if we want to consider this an undocumented feature

I think the variable is rather misleading since it seems to imply it will cause all resources to be suffixed, but that's not the case. I'd leave this to be undocumented since we're not guaranteeing a user can build workflows on it.

SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX might convey the intention here a little more clearly? We are not in any way saying this should be used in a production setting.

sfc-gh-melnacouzi · 2024-08-13T20:13:07Z

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

        if self.definition.package and self.definition.package.name:
-            return to_identifier(self.definition.package.name)
+            return append_to_identifier(
+                to_identifier(self.definition.package.name), suffix


this is a bit tricky, because it assumes suffix is a safe identifier.
concat_identifiers() might be what you're looking for.

oh TIL, will update

sfc-gh-bdufour · 2024-08-13T20:55:06Z

src/snowflake/cli/api/project/definition.py

+def resource_suffix() -> str:
+    """A suffix that should be added to account-level resources."""
+    return os.environ.get(SNOWFLAKE_CLI_RESOURCE_SUFFIX_VAR, "")


Should this really be part of the public API? At least we should give it a very descriptive name, I normally worry about making non-prod logic sound so prod-like.

based on our discussion earlier, it seemed to me that this would indeed be part of the public API (but undocumented). Let's PL this for tomorrow morning

sfc-gh-fcampbell · 2024-08-14T15:43:40Z

src/snowflake/cli/api/project/definition.py

-def default_app_package(project_name: str):
-    user = clean_identifier(get_env_username() or DEFAULT_USERNAME)
-    return append_to_identifier(to_identifier(project_name), f"_pkg_{user}")


merged into the body of NativeAppProjectModel.package_name since this was the only usage of this function (and we needed to separate the addition of the _pkg from the addition of the username)

sfc-gh-fcampbell · 2024-08-14T15:44:00Z

src/snowflake/cli/api/project/definition.py

-def default_application(project_name: str):
-    user = clean_identifier(get_env_username() or DEFAULT_USERNAME)
-    return append_to_identifier(to_identifier(project_name), f"_{user}")


merged into the body of NativeAppProjectModel.app_name since this was the only usage of this function (and we needed to separate the addition of the _ from the addition of the username)

sfc-gh-melnacouzi · 2024-08-14T17:18:20Z

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

-        else:
-            return to_identifier(default_app_package(self.project_identifier))
+            return concat_identifiers(
+                [to_identifier(self.definition.package.name), suffix]


to_identifier() is probably not needed.

yeah you're right, it was only needed when i was using append_to_identifier

sfc-gh-melnacouzi · 2024-08-14T17:19:30Z

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

+        # generate a name for the package from the project identifier and
+        # append the resource suffix
+        # If we don't have a resource suffix specified, use the default one
+        return concat_identifiers(


We should just append at the end. We shouldn't replace default_resource_suffix. Also this logic will not be consistent, because for PDFV1.1 we use Pydantic defaults instead.

my original intention was to skip using $USER if $SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX is non-empty, but I can just make it a straight append too

I would vote to append it, that way we keep the rest of the logic intact, and keep this as pure suffix.

sounds good, updated

sfc-gh-melnacouzi · 2024-08-14T17:19:46Z

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

+        # append the resource suffix.
+        # If we don't have a resource suffix specified, use the default one
+        return concat_identifiers(
+            [self.project_identifier, suffix or default_resource_suffix()]


…me (#1443) Prerequisite for #1437. After discussing in-person, we decided to not overwrite the `USER` environment variable in tests and instead add a new `SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX` env var that will be appended to app package and app identifiers. In #1437, we'll use this variable to generate unique names for these resources so they don't clash during concurrent tests.

sfc-gh-fcampbell commented Aug 13, 2024

View reviewed changes

sfc-gh-fcampbell marked this pull request as ready for review August 13, 2024 19:46

sfc-gh-fcampbell requested review from a team as code owners August 13, 2024 19:46

sfc-gh-fcampbell marked this pull request as draft August 13, 2024 19:46

sfc-gh-fcampbell force-pushed the frank-app-resource-name-suffixes branch 2 times, most recently from 7d4c2f3 to 4a0c31a Compare August 13, 2024 20:06

sfc-gh-melnacouzi reviewed Aug 13, 2024

View reviewed changes

sfc-gh-fcampbell force-pushed the frank-app-resource-name-suffixes branch from 4a0c31a to b6c66d3 Compare August 13, 2024 20:42

sfc-gh-bdufour reviewed Aug 13, 2024

View reviewed changes

sfc-gh-fcampbell force-pushed the frank-app-resource-name-suffixes branch from b6c66d3 to 23bd066 Compare August 14, 2024 15:15

sfc-gh-fcampbell changed the title ~~Append SNOWFLAKE_CLI_RESOURCE_SUFFIX value to app and package name~~ Append SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX value to app and package name Aug 14, 2024

sfc-gh-fcampbell force-pushed the frank-app-resource-name-suffixes branch from 23bd066 to 4125642 Compare August 14, 2024 15:41

sfc-gh-fcampbell commented Aug 14, 2024

View reviewed changes

Append SNOWFLAKE_CLI_RESOURCE_SUFFIX value to app and package name

07a7d3f

sfc-gh-fcampbell force-pushed the frank-app-resource-name-suffixes branch from 4125642 to 07a7d3f Compare August 14, 2024 15:45

sfc-gh-fcampbell marked this pull request as ready for review August 14, 2024 17:01

sfc-gh-melnacouzi reviewed Aug 14, 2024

View reviewed changes

sfc-gh-fcampbell removed the request for review from a team August 14, 2024 17:36

Always append suffix

74556ca

sfc-gh-fcampbell force-pushed the frank-app-resource-name-suffixes branch from 0832f02 to 74556ca Compare August 14, 2024 17:37

sfc-gh-melnacouzi approved these changes Aug 14, 2024

View reviewed changes

sfc-gh-fcampbell enabled auto-merge (squash) August 14, 2024 17:48

sfc-gh-fcampbell merged commit 760d5f8 into main Aug 14, 2024
16 checks passed

sfc-gh-fcampbell deleted the frank-app-resource-name-suffixes branch August 14, 2024 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX value to app and package name #1443

Append SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX value to app and package name #1443

sfc-gh-fcampbell commented Aug 13, 2024 •

edited

Loading

sfc-gh-fcampbell Aug 13, 2024 •

edited

Loading

sfc-gh-melnacouzi Aug 13, 2024

sfc-gh-fcampbell Aug 13, 2024

sfc-gh-bdufour Aug 13, 2024

sfc-gh-bdufour Aug 13, 2024

sfc-gh-melnacouzi Aug 13, 2024

sfc-gh-fcampbell Aug 13, 2024

sfc-gh-bdufour Aug 13, 2024

sfc-gh-fcampbell Aug 13, 2024

sfc-gh-fcampbell Aug 14, 2024

sfc-gh-fcampbell Aug 14, 2024

sfc-gh-melnacouzi Aug 14, 2024

sfc-gh-fcampbell Aug 14, 2024

sfc-gh-melnacouzi Aug 14, 2024

sfc-gh-fcampbell Aug 14, 2024

sfc-gh-melnacouzi Aug 14, 2024

sfc-gh-fcampbell Aug 14, 2024 •

edited

Loading

sfc-gh-melnacouzi Aug 14, 2024

Append SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX value to app and package name #1443

Append SNOWFLAKE_CLI_TEST_RESOURCE_SUFFIX value to app and package name #1443

Conversation

sfc-gh-fcampbell commented Aug 13, 2024 • edited Loading

Pre-review checklist

Changes description

sfc-gh-fcampbell Aug 13, 2024 • edited Loading

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-fcampbell Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-fcampbell commented Aug 13, 2024 •

edited

Loading

sfc-gh-fcampbell Aug 13, 2024 •

edited

Loading

sfc-gh-fcampbell Aug 14, 2024 •

edited

Loading