Enable Event Sharing V2 in SnowCLI #1823

sfc-gh-melnacouzi · 2024-10-31T07:13:46Z

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

Enable Event Sharing V2 in SnowCLI.
Example:

  app:
    type: application
    from:
      target: pkg
    telemetry:
      authorize_event_sharing: true
      optional_shared_events:
        - DEBUG_LOGS
        - ERRORS_AND_WARNINGS

Above example authorize telemetry to be shared to provider. In addition to required telemetry by the provider, consumer can specify a list of extra events to share with the provider.
There are 2 flags introduced to control the BCR in the backend. One to enable telemetry on consumer for same-account applications, and another to enforce that the consumer authorize event sharing when the provider has mandatory events.

Added unit tests.
Tested manually - optional events not recognized - reached out to corresponding team.
Integ tests will be added as a follow up PR.

src/snowflake/cli/_plugins/nativeapp/entities/models/telemetry.py

src/snowflake/cli/_plugins/connection/util.py

sfc-gh-fcampbell · 2024-10-31T14:15:36Z

src/snowflake/cli/_plugins/nativeapp/entities/models/event_sharing_telemetry.py

+
+    @field_validator("optional_shared_events")
+    @classmethod
+    def transform_artifacts(


Suggested change

def transform_artifacts(

def validate_optional_shared_events(

?

sfc-gh-fcampbell · 2024-10-31T14:16:27Z

src/snowflake/cli/_plugins/nativeapp/entities/models/event_sharing_telemetry.py

+    authorize_event_sharing: Optional[bool] = Field(
+        title="Whether to authorize Snowflake to share application usage data with application package provider. This automatically enables the sharing of required telemetry events.",
+        default=None,
+    )
+    optional_shared_events: Optional[List[str]] = Field(


do these need to be optional to have a default? do we handle these fields differently if they're None vs False/[]?

i am not sure if optional [] as default will have any side effect (I know it's not recommended in function params)

sfc-gh-fcampbell · 2024-10-31T14:18:11Z

src/snowflake/cli/_plugins/nativeapp/entities/models/event_sharing_telemetry.py

+
+        # make sure that each event is made of letters and underscores:
+        for event in original_shared_events:
+            if not event.isalpha() and not event.replace("_", "").isalpha():


Suggested change

if not event.isalpha() and not event.replace("_", "").isalpha():

if not re.fullmatch(r"[a-zA-Z_]", event):

?

if you don't want the regex, then you only need

Suggested change

if not event.isalpha() and not event.replace("_", "").isalpha():

if not event.replace("_", "").isalpha():

sfc-gh-fcampbell · 2024-10-31T14:26:26Z

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

+        workspace_ctx = self._workspace_ctx
+        console = workspace_ctx.console
+        project_root = workspace_ctx.project_root


Suggested change

workspace_ctx = self._workspace_ctx

console = workspace_ctx.console

project_root = workspace_ctx.project_root

console = self._workspace_ctx.console

sfc-gh-fcampbell · 2024-10-31T14:26:30Z

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

+        is_dev_mode = install_method.is_dev_mode
+
+        mandatory_events_found = (
+            len(find_mandatory_events_in_manifest_file(project_root / deploy_root)) > 0


Suggested change

len(find_mandatory_events_in_manifest_file(project_root / deploy_root)) > 0

len(find_mandatory_events_in_manifest_file(self.project_root / deploy_root)) > 0

sfc-gh-fcampbell · 2024-10-31T14:32:57Z

tests/nativeapp/test_event_sharing.py

+        manifest_contents=manifest_contents,
+        authorize_event_sharing=authorize_event_sharing,
+    )
+    assert not mock_diff_result.has_changes()


mock_diff_result is never passed anywhere, how can this assertion fail? (also found in other tests)

yea it doesn't make sense - i'll remove it from this new file at least.

sfc-gh-fcampbell · 2024-10-31T14:34:26Z

tests/nativeapp/test_event_sharing.py

+allow_always_policy = AllowAlwaysPolicy()
+ask_always_policy = AskAlwaysPolicy()
+deny_always_policy = DenyAlwaysPolicy()


these are unused in this file. your _create_or_upgrade_app takes in a policy param, did you mean to use them there?

no it's a left-over copy from another file.

sfc-gh-fcampbell · 2024-10-31T14:36:40Z

tests/nativeapp/test_event_sharing.py

+        mock_console.warning.assert_has_calls(
+            [
+                mock.call(
+                    "WARNING: Mandatory events are present in the manifest file, but event sharing is not authorized in the application telemetry field. This will soon be required to set in order to deploy applications."
+                )
+            ]
+        )


Suggested change

mock_console.warning.assert_has_calls(

[

mock.call(

"WARNING: Mandatory events are present in the manifest file, but event sharing is not authorized in the application telemetry field. This will soon be required to set in order to deploy applications."

)

]

)

mock_console.warning.assert_called_with(

"WARNING: Mandatory events are present in the manifest file, but event sharing is not authorized in the application telemetry field. This will soon be required to set in order to deploy applications."

)

sfc-gh-fcampbell · 2024-10-31T14:38:22Z

tests/nativeapp/test_run_processor.py

+)
+
+
+def setup_project_file(current_working_directory: str, pdf=None):


let's use project factories in here

I would like to keep this for a separate PR if possible.

sfc-gh-fcampbell · 2024-10-31T14:38:53Z

tests/nativeapp/test_run_processor.py

+        print(mock_execute.mock_calls)
+        raise e


sfc-gh-melnacouzi requested review from a team as code owners October 31, 2024 07:13

sfc-gh-turbaszek reviewed Oct 31, 2024

View reviewed changes

src/snowflake/cli/_plugins/nativeapp/entities/models/telemetry.py Outdated Show resolved Hide resolved

sfc-gh-turbaszek reviewed Oct 31, 2024

View reviewed changes

src/snowflake/cli/_plugins/connection/util.py Show resolved Hide resolved

sfc-gh-turbaszek previously approved these changes Oct 31, 2024

View reviewed changes

sfc-gh-melnacouzi added 2 commits October 31, 2024 09:21

Enable Event Sharing V2 in SnowCLI

2b80439

Rename telemetry to eventsharingtelemetry

7380050

sfc-gh-melnacouzi dismissed sfc-gh-turbaszek’s stale review via 7380050 October 31, 2024 13:21

sfc-gh-melnacouzi force-pushed the melnacouzi-event-sharing-mvp branch from 340bf4e to 7380050 Compare October 31, 2024 13:21

sfc-gh-fcampbell reviewed Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Event Sharing V2 in SnowCLI #1823

Enable Event Sharing V2 in SnowCLI #1823

sfc-gh-melnacouzi commented Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-melnacouzi Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-melnacouzi Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-melnacouzi Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

sfc-gh-melnacouzi Oct 31, 2024

sfc-gh-fcampbell Oct 31, 2024

	def transform_artifacts(
	def validate_optional_shared_events(

	if not event.isalpha() and not event.replace("_", "").isalpha():
	if not re.fullmatch(r"[a-zA-Z_]", event):

	if not event.isalpha() and not event.replace("_", "").isalpha():
	if not event.replace("_", "").isalpha():

	len(find_mandatory_events_in_manifest_file(project_root / deploy_root)) > 0
	len(find_mandatory_events_in_manifest_file(self.project_root / deploy_root)) > 0

		)


		def setup_project_file(current_working_directory: str, pdf=None):

Enable Event Sharing V2 in SnowCLI #1823

Are you sure you want to change the base?

Enable Event Sharing V2 in SnowCLI #1823

Conversation

sfc-gh-melnacouzi commented Oct 31, 2024

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