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.

Sign up for GitHub

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

Jump to bottom

Add package scripts warning back #1764

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Add package scripts warning back #1764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
156c155
Add package scripts warning back
sfc-gh-melnacouzi Oct 22, 2024
a096047
Fix integration tests for emitting package scripts metric
sfc-gh-melnacouzi Oct 22, 2024
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
60 changes: 0 additions & 60 deletions src/snowflake/cli/_plugins/nativeapp/entities/application_package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
DenyAlwaysPolicy,
PolicyBase,
)
from snowflake.cli._plugins.nativeapp.sf_facade import get_snowflake_facade
from snowflake.cli._plugins.nativeapp.utils import needs_confirmation
from snowflake.cli._plugins.stage.diff import DiffResult
from snowflake.cli._plugins.stage.manager import StageManager
Expand All @@ -52,7 +51,6 @@
drop_generic_object,
execute_post_deploy_hooks,
generic_sql_error_handler,
render_script_templates,
sync_deploy_root_with_stage,
validation_item_to_str,
)
Expand All @@ -77,7 +75,6 @@
to_identifier,
unquote_identifier,
)
from snowflake.cli.api.rendering.jinja import get_basic_jinja_env
from snowflake.cli.api.utils.cursor import find_all_rows
from snowflake.connector import DictCursor, ProgrammingError
from snowflake.connector.cursor import SnowflakeCursor
Expand Down Expand Up @@ -213,7 +210,6 @@ def action_deploy(
(model.meta and model.meta.warehouse) or workspace_ctx.default_warehouse
),
post_deploy_hooks=model.meta and model.meta.post_deploy,
package_scripts=[], # Package scripts are not supported in PDFv2
policy=policy,
)

Expand Down Expand Up @@ -493,7 +489,6 @@ def action_version_create(
(model.meta and model.meta.warehouse) or workspace_ctx.default_warehouse
),
post_deploy_hooks=(model.meta and model.meta.post_deploy),
package_scripts=[], # Package scripts are not supported in PDFv2
policy=policy,
)

Expand Down Expand Up @@ -623,7 +618,6 @@ def deploy(
validate: bool,
stage_fqn: str,
post_deploy_hooks: list[PostDeployHook] | None,
package_scripts: List[str],
policy: PolicyBase,
) -> DiffResult:
# 1. Create a bundle if one wasn't passed in
Expand All @@ -649,14 +643,6 @@ def deploy(
if not policy.should_proceed("Proceed with using this package?"):
raise typer.Abort() from e

cls.apply_package_scripts(
console=console,
package_scripts=package_scripts,
package_warehouse=package_warehouse,
project_root=project_root,
package_role=package_role,
package_name=package_name,
)
with get_sql_executor().use_role(package_role):
# 3. Upload files from deploy root local folder to the above stage
stage_schema = extract_schema(stage_fqn)
Expand Down Expand Up @@ -1088,51 +1074,6 @@ def use_package_warehouse(
)
)

@classmethod
def apply_package_scripts(
cls,
console: AbstractConsole,
package_scripts: List[str],
package_warehouse: Optional[str],
project_root: Path,
package_role: str,
package_name: str,
) -> None:
"""
Assuming the application package exists and we are using the correct role,
applies all package scripts in-order to the application package.
"""

metrics = get_cli_context().metrics
metrics.set_counter_default(CLICounterField.PACKAGE_SCRIPTS, 0)

if not package_scripts:
return

metrics.set_counter(CLICounterField.PACKAGE_SCRIPTS, 1)

console.warning(
"WARNING: native_app.package.scripts is deprecated. Please migrate to using native_app.package.post_deploy."
)

queued_queries = render_script_templates(
project_root,
dict(package_name=package_name),
package_scripts,
get_basic_jinja_env(),
)

# once we're sure all the templates expanded correctly, execute all of them
for i, queries in enumerate(queued_queries):
script_name = package_scripts[i]
console.step(f"Applying package script: {script_name}")
get_snowflake_facade().execute_user_script(
queries=queries,
script_name=script_name,
role=package_role,
warehouse=package_warehouse,
)

@classmethod
def create_app_package(
cls,
Expand Down Expand Up @@ -1342,7 +1283,6 @@ def get_validation_result_static(
stage_fqn=stage_fqn,
package_warehouse=package_warehouse,
post_deploy_hooks=[],
package_scripts=[],
policy=policy,
)
prefixed_stage_fqn = StageManager.get_standard_stage_prefix(stage_fqn)
Expand Down
9 changes: 9 additions & 0 deletions src/snowflake/cli/api/project/definition_conversion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
ApplicationPackageEntityModel,
)
from snowflake.cli._plugins.snowpark.common import is_name_a_templated_one
from snowflake.cli.api.cli_global_context import get_cli_context
from snowflake.cli.api.console import cli_console
from snowflake.cli.api.constants import (
DEFAULT_ENV_FILE,
DEFAULT_PAGES_DIR,
Expand All @@ -21,6 +23,7 @@
SNOWPARK_SHARED_MIXIN,
)
from snowflake.cli.api.entities.utils import render_script_template
from snowflake.cli.api.metrics import CLICounterField
from snowflake.cli.api.project.schemas.entities.common import (
MetaField,
SqlScriptHookType,
Expand Down Expand Up @@ -381,8 +384,14 @@ def _convert_templates_in_files(
"""Converts templates in other files to the new format"""
# TODO handle artifacts using the "templates" processor
# For now this only handles Native App package scripts
metrics = get_cli_context().metrics
metrics.set_counter_default(CLICounterField.PACKAGE_SCRIPTS, 0)

if (na := definition_v1.native_app) and (pkg := na.package) and pkg.scripts:
metrics.set_counter(CLICounterField.PACKAGE_SCRIPTS, 1)
cli_console.warning(
"WARNING: native_app.package.scripts is deprecated. Please migrate to using native_app.package.post_deploy."
)
# If the v1 definition has a Native App with a package, we know
# that the v2 definition will have exactly one application package entity
pkg_entity: ApplicationPackageEntityModel = list(
Expand Down
2 changes: 0 additions & 2 deletions tests/nativeapp/test_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,6 @@ def test_validate_use_scratch_stage(mock_execute, mock_deploy, temp_dir, mock_cu
stage_fqn=f"{pkg_model.fqn.name}.{pkg_model.scratch_stage}",
package_warehouse=pkg_model.meta.warehouse,
post_deploy_hooks=[],
package_scripts=[],
policy=AllowAlwaysPolicy(),
)
assert mock_execute.mock_calls == expected
Expand Down Expand Up @@ -1464,7 +1463,6 @@ def test_validate_failing_drops_scratch_stage(
stage_fqn=f"{pkg_model.fqn.name}.{pkg_model.scratch_stage}",
package_warehouse=pkg_model.meta.warehouse,
post_deploy_hooks=[],
package_scripts=[],
policy=AllowAlwaysPolicy(),
)
assert mock_execute.mock_calls == expected
Expand Down
3 changes: 1 addition & 2 deletions tests_integration/nativeapp/test_feature_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def test_sql_templating_emits_counter(
CLICounterField.TEMPLATES_PROCESSOR: 0,
CLICounterField.PDF_TEMPLATES: 1,
CLICounterField.POST_DEPLOY_SCRIPTS: 1,
CLICounterField.PACKAGE_SCRIPTS: 0,
CLICounterField.PACKAGE_SCRIPTS: 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test files have package scripts, so this should be 1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to make the distinction that since the pdf is v1, then POST_DEPLOY_SCRIPTS should count as 0 then?

Copy link
Contributor Author

@sfc-gh-melnacouzi sfc-gh-melnacouzi Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i think it's something we can distinguish in the graphs themselves (like exclude package_scripts = 1). I do not want to touch the post_deploy_scripts part because the goal is to have the bulk part of the code operate in pdf v2 mode, and would make sense to keep the counters for this one with the post_deploy code itself.
It's basically a compromise.

},
),
# ensure post deploy scripts are picked up for v2
Expand All @@ -115,7 +115,6 @@ def test_sql_templating_emits_counter(
CLICounterField.TEMPLATES_PROCESSOR: 0,
CLICounterField.PDF_TEMPLATES: 1,
CLICounterField.POST_DEPLOY_SCRIPTS: 1,
CLICounterField.PACKAGE_SCRIPTS: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is PDF v2, so no need to emit metric for this one.

},
),
],
Expand Down
Loading