feat: add snow run command for project scripts#2823
feat: add snow run command for project scripts#2823sfc-gh-mborins wants to merge 7 commits intomainfrom
snow run command for project scripts#2823Conversation
Add a new `snow run` command that allows users to define and execute
project-specific scripts in snowflake.yml or manifest.yml. Features include:
- Load script from either snowflake.yml or manifest.yml with conflict detection
- Track script source file and show in --list output
- Script definitions with cmd, description, shell, cwd, and env options
- Composite scripts using `run:` to sequence multiple scripts
- Variable interpolation with ${env.VAR} syntax
- CLI overrides with -D flag
- Dry-run mode and continue-on-error for composite scripts
- Extensive unit and integration tests covering various scenarios and edge cases
.... Generated with [Cortex Code](https://docs.snowflake.com/user-guide/snowflake-cortex/cortex-agents)
Co-Authored-By: Cortex Code <[email protected]>
6479df9 to
8cd2002
Compare
Fix code quality issues (trailing whitespace, ruff G004 f-string logging, black formatting), Windows test failures (subprocess mock was polluting global scope via shared module reference), and update E2E snapshots to include the new run command in snow --help output. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <[email protected]>
8cd2002 to
7deb60c
Compare
Prevents infinite recursion when scripts reference each other by tracking the call stack during composite script execution. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <[email protected]>
.... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <[email protected]>
- Use shlex.quote() on interpolated variable values when shell=True to prevent command injection via shell metacharacters - Track and return the first non-zero exit code from composite scripts instead of always returning hardcoded exit code 1 .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <[email protected]>
…hars .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <[email protected]>
… preservation - test_run_shell_mode_escapes_interpolated_variables: verifies shlex.quote on interpolated values prevents command injection in shell mode - test_run_extra_args_with_spaces_are_quoted: verifies extra args with spaces are properly quoted - test_run_composite_preserves_first_failure_exit_code: verifies composite scripts return the actual first failure exit code .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <[email protected]>
|
|
||
| manager = ScriptManager(project_root) | ||
|
|
||
| if list_scripts: |
There was a problem hiding this comment.
Will this work with --format=json?
There was a problem hiding this comment.
Would be better to use CollectionResult/ObjectResult to have support for different formatting options. I don't think json/csv would be very useful in this case, but having some response in those formats is better than nothing
There was a problem hiding this comment.
Another issue I see is that introduces an inconsistency with other cli commands. As raised before, we usually have a dedicated command for listing objects
There was a problem hiding this comment.
with a simple script like
scripts:
show_pwd:
cmd: "pwd"
description: "run pwd"
we get this:
snow run show_pwd --format json
/Users/jwilkowski/snowflake/dcm_test/dcm_manifest_v2
{
"message": ""
}
| for v in parse_key_value_variables(var_overrides): | ||
| vars_dict[v.key] = v.value |
There was a problem hiding this comment.
| for v in parse_key_value_variables(var_overrides): | |
| vars_dict[v.key] = v.value | |
| vars_dict = {v.key: v.value for v in parse_key_value_variables(var_overrides)} |
More pythonic
sfc-gh-jwilkowski
left a comment
There was a problem hiding this comment.
I'd like to request some changes. I think that some design decisions require more discussion with @sfc-gh-jbilek (like --list as flag rather than standalone command)
| if not script: | ||
| available = list(manager.list_scripts().keys()) | ||
| if available: | ||
| raise ClickException( |
There was a problem hiding this comment.
Please raise CliError
| raise ClickException( | ||
| "Scripts defined in both manifest.yml and snowflake.yml.\n" | ||
| "Scripts must be defined in only one file per directory.\n\n" | ||
| "Recommendation: Move all scripts to one file.\n" | ||
| "- Use manifest.yml for DCM-focused projects\n" | ||
| "- Use snowflake.yml for app-focused projects" | ||
| ) |
There was a problem hiding this comment.
Thinking out loud - is that a bad thing really? Maybe we can merge scripts from both sources?
| for name, script in scripts.items(): | ||
| desc = script.description or "(no description)" | ||
| cc.message(f" {name:20} {desc}") | ||
| return MessageResult("") |
There was a problem hiding this comment.
You can also use EmptyResult
|
|
||
| manager = ScriptManager(project_root) | ||
|
|
||
| if list_scripts: |
There was a problem hiding this comment.
Would be better to use CollectionResult/ObjectResult to have support for different formatting options. I don't think json/csv would be very useful in this case, but having some response in those formats is better than nothing
|
|
||
| manager = ScriptManager(project_root) | ||
|
|
||
| if list_scripts: |
There was a problem hiding this comment.
Another issue I see is that introduces an inconsistency with other cli commands. As raised before, we usually have a dedicated command for listing objects
| if scripts: | ||
| source = manager.scripts_source or "project" | ||
| cc.message(f"Available scripts from {source} (use --list for details):") | ||
| for name in scripts: | ||
| cc.message(f" {name}") | ||
| return MessageResult("\nSpecify a script name to run, or use --list") | ||
| return MessageResult( | ||
| "No scripts defined. Add a 'scripts' section to snowflake.yml or manifest.yml" | ||
| ) |
There was a problem hiding this comment.
You can first deal with simple case to reduce indentation levels
| if scripts: | |
| source = manager.scripts_source or "project" | |
| cc.message(f"Available scripts from {source} (use --list for details):") | |
| for name in scripts: | |
| cc.message(f" {name}") | |
| return MessageResult("\nSpecify a script name to run, or use --list") | |
| return MessageResult( | |
| "No scripts defined. Add a 'scripts' section to snowflake.yml or manifest.yml" | |
| ) | |
| if not scripts: | |
| return MessageResult( | |
| "No scripts defined. Add a 'scripts' section to snowflake.yml or manifest.yml" | |
| ) | |
| source = manager.scripts_source or "project" | |
| cc.message(f"Available scripts from {source} (use --list for details):") | |
| for name in scripts: | |
| cc.message(f" {name}") | |
| return MessageResult("\nSpecify a script name to run, or use --list") |
| if not result.success: | ||
| raise typer.Exit(result.exit_code) |
There was a problem hiding this comment.
I'm a bit biased here, as on one hand I'd prefer this minimalistic way of reporting failures, but on the other hand we're not telling what failed or why. Also it's not consistent with how we report errors
| for name, script_def in scripts_data.items(): | ||
| if isinstance(script_def, str): | ||
| result[name] = ScriptModel(cmd=script_def) | ||
| else: | ||
| result[name] = ScriptModel(**script_def) | ||
| return result |
There was a problem hiding this comment.
No error handling for malformed manifest.yaml. We've recently added DCMManifest class and afaik that's the only manifest we have right now, so might be better to use that or even extract some base fields such as manifest_version, project_type and scripts to a base class
| log.warning("Variable '${%s}' not found, leaving as-is", var_name) | ||
| return match.group(0) |
There was a problem hiding this comment.
I'd rather raise an error here, as using the var name could lead to dangerous outcomes.
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| VARIABLE_PATTERN = re.compile(r"\$\{([^}]+)\}") |
There was a problem hiding this comment.
I'm worried about introducing another syntax for variable interpolation. Our users might get confused with those different syntax versions living side by side https://github.com/snowflakedb/snowflake-cli-templates/blob/main/streamlit_vnext_multi_page/snowflake.yml#L6-L9
Pre-review checklist
Changes description
Add a new
snow runcommand that allows users to define and execute project-specific scripts in snowflake.yml or manifest.yml. Features include:run:to sequence multiple scripts