Skip to content

feat: add snow run command for project scripts#2823

Open
sfc-gh-mborins wants to merge 10 commits intomainfrom
snow-run
Open

feat: add snow run command for project scripts#2823
sfc-gh-mborins wants to merge 10 commits intomainfrom
snow-run

Conversation

@sfc-gh-mborins
Copy link
Copy Markdown
Contributor

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.
  • I've described my changes in the documentation.

Changes description

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


manager = ScriptManager(project_root)

if list_scripts:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this work with --format=json?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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": ""
}

Copy link
Copy Markdown
Contributor Author

@sfc-gh-mborins sfc-gh-mborins Mar 24, 2026

Choose a reason for hiding this comment

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

Fair point on consistency. Scripts aren't Snowflake objects so add_object_command_aliases doesn't fit directly, but we could add a dedicated snow run list subcommand to follow the pattern. Worth discussing with @sfc-gh-jbilek as you suggested. For now, I've updated --list to return CollectionResult so --format=json works properly.

One callout... anything we put after "run" breaks a potential script. That is why I am hesitant on the subcommands

Copy link
Copy Markdown
Contributor

@sfc-gh-jwilkowski sfc-gh-jwilkowski left a comment

Choose a reason for hiding this comment

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

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)

Comment on lines +69 to +75
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"
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking out loud - is that a bad thing really? Maybe we can merge scripts from both sources?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good thought, but I'd prefer to keep single-source here. Having one file own all scripts avoids ambiguity about which file defines what, makes debugging simpler ("where is this script defined?" has exactly one answer), and prevents subtle ordering/shadowing issues if both files define the same name. The error message already guides users to pick one file.


manager = ScriptManager(project_root)

if list_scripts:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines +148 to +149
if not result.success:
raise typer.Exit(result.exit_code)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, we should be more informative. Updated to raise CliError with the script name and exit code: Script 'deploy' failed with exit code 1. This is consistent with how other CLI commands report errors.

Comment on lines +122 to +127
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point on both fronts. The try/except already catches malformed YAML and logs it, so we won't crash on bad manifest files. Extracting shared base fields (manifest_version, project_type, scripts) from DCMManifest into a base class is a solid idea, but I think it's better suited for a follow-up PR to keep scope manageable here. Would that work?


log = logging.getLogger(__name__)

VARIABLE_PATTERN = re.compile(r"\$\{([^}]+)\}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid concern. The distinction is: <% ctx.env.XXX %> is Jinja-style templating resolved at project load time (for entity definitions, identifiers, etc.), while ${var} is resolved at script execution time and supports --var overrides for runtime flexibility. ${var} is also the natural syntax users expect in shell commands (matches bash, Makefile, docker-compose conventions). I think clear documentation of the two scopes would address confusion. Happy to discuss if you'd prefer aligning to one syntax though.

sfc-gh-mborins and others added 7 commits March 24, 2026 13:41
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 <noreply@snowflake.com>
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 <noreply@snowflake.com>
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 <noreply@snowflake.com>
.... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code)

Co-Authored-By: Cortex Code <noreply@snowflake.com>
- 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 <noreply@snowflake.com>
…hars

.... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code)

Co-Authored-By: Cortex Code <noreply@snowflake.com>
… 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 <noreply@snowflake.com>
…ize_for_terminal

Incorporates reviewer feedback from sfc-gh-turbaszek and sfc-gh-jwilkowski:
- Replace ClickException with CliError throughout run plugin
- Use CollectionResult for --list output to support --format=json/csv
- Add sanitize_for_terminal for untrusted script names
- Use dict comprehension for vars_dict
- Apply early-return pattern for no-script-name path
- Raise CliError on unresolved variables instead of logging warning
- Report script name and exit code on failure via CliError
- Update tests to match new output format and error behavior

.... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code)

Co-Authored-By: Cortex Code <noreply@snowflake.com>
sfc-gh-mborins and others added 2 commits March 24, 2026 22:33
- Use `is not None` instead of truthiness check for `script.run` to
  handle empty list edge case that would cause AttributeError
- Add schema validation to reject empty `run` lists early
- For non-shell mode, split command template first then interpolate
  per-arg to preserve argument boundaries when variable values
  contain spaces

.... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code)

Co-Authored-By: Cortex Code <noreply@snowflake.com>
CI uses ruff 0.1.7 which treats snowflake.* as same import group as
third-party packages, so no blank line separator is expected.

.... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code)

Co-Authored-By: Cortex Code <noreply@snowflake.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants