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

[SNOW-1371918] Make snow app deploy behave more intuitively #1170

Merged
merged 10 commits into from
Jun 14, 2024
Merged

[SNOW-1371918] Make snow app deploy behave more intuitively #1170

merged 10 commits into from
Jun 14, 2024

Conversation

sfc-gh-bdufour
Copy link
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.
  • 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

snow app deploy, when invoked with directories, current a matching artifact deployment rule for each parameter. This is not very intuitive. For example, when a user specifies:

- src: app/*
- dest: ./

in their snowflake.yml, snow app deploy -r app results in an error. This change makes the command deploy all files that have been changed within the specified directory instead.

sfc-gh-bdufour
sfc-gh-bdufour commented Jun 6, 2024
View reviewed changes
RELEASE-NOTES.md Outdated Show resolved Hide resolved
@sfc-gh-bdufour sfc-gh-bdufour marked this pull request as ready for review June 6, 2024 19:00
@sfc-gh-bdufour sfc-gh-bdufour requested a review from a team as a code owner June 6, 2024 19:00
@sfc-gh-bdufour sfc-gh-bdufour requested a review from a team as a code owner June 12, 2024 21:32
@sfc-gh-bdufour sfc-gh-bdufour marked this pull request as draft June 12, 2024 21:51
@sfc-gh-bdufour sfc-gh-bdufour marked this pull request as ready for review June 13, 2024 14:05
sfc-gh-bgoel
sfc-gh-bgoel reviewed Jun 13, 2024
View reviewed changes
src/snowflake/cli/plugins/nativeapp/commands.py
specified, it must match one of the artifacts src pattern entries in snowflake.yml. If a directory is
specified, it will be searched for subfolders or files to deploy based on artifacts src pattern entries. If
unspecified, the command syncs all local changes to the stage."""
).strip(),
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc, the reason why this only says file or directory, and not a glob pattern, is because a user's shell should automatically expand that and pass that in to this arg as a list. Is that correct? Do we still need to clarify that here?

Copy link
Contributor Author

@sfc-gh-bdufour sfc-gh-bdufour Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That message has nothing to do with globbing. Any path specified as argument could be a file or a directory. The behaviour is somewhat different based on what the path corresponds to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me rephrase: My intent was to ask if a user can now do all three:

  1. snow app deploy some/dir/**.py
  2. snow app deploy some/dir
  3. snow app deploy some/file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. Yes they can, but this PR doesn't change that, it was true before as well. We it does change is what happens if some/dir is specified. Basically the path -> artifact rule mapping is just being relaxed here, nothing more.

src/snowflake/cli/plugins/nativeapp/commands.py Show resolved Hide resolved
tests_integration/nativeapp/test_deploy.py Show resolved Hide resolved
sfc-gh-bgoel
sfc-gh-bgoel previously approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-bgoel sfc-gh-bgoel left a comment

Choose a reason for hiding this comment

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

LGTM, replied to open threads.

src/snowflake/cli/plugins/nativeapp/commands.py
specified, it must match one of the artifacts src pattern entries in snowflake.yml. If a directory is
specified, it will be searched for subfolders or files to deploy based on artifacts src pattern entries. If
unspecified, the command syncs all local changes to the stage."""
).strip(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me rephrase: My intent was to ask if a user can now do all three:

  1. snow app deploy some/dir/**.py
  2. snow app deploy some/dir
  3. snow app deploy some/file

tests_integration/nativeapp/test_deploy.py Show resolved Hide resolved
@sfc-gh-bdufour sfc-gh-bdufour force-pushed the bdufour-relax-deploy branch 2 times, most recently from f274ec2 to 5646b1f Compare June 14, 2024 00:22
@sfc-gh-bdufour sfc-gh-bdufour merged commit 0f40108 into main Jun 14, 2024
24 checks passed
@sfc-gh-bdufour sfc-gh-bdufour deleted the bdufour-relax-deploy branch June 14, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-cgorrie sfc-gh-cgorrie sfc-gh-cgorrie approved these changes

@sfc-gh-bgoel sfc-gh-bgoel sfc-gh-bgoel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfc-gh-bdufour @sfc-gh-cgorrie @sfc-gh-bgoel