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 release directives support to SnowCLI #1938

Merged
merged 5 commits into from
Dec 11, 2024
Merged

Add release directives support to SnowCLI #1938

merged 5 commits into from
Dec 11, 2024

Conversation

sfc-gh-melnacouzi
Copy link
Contributor

@sfc-gh-melnacouzi sfc-gh-melnacouzi commented Dec 9, 2024 •
edited
Loading

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

  • Add support for release directive commands:
  1. snow app release-directive list to list the release directives.
  2. snow app release-directive set to set a release directive
  3. snow app release-directive unset to unset a release directive.

Integ tests will be added after release channels changes are available in prod.

@sfc-gh-melnacouzi sfc-gh-melnacouzi requested review from a team as code owners December 9, 2024 16:35
sfc-gh-gbloom
sfc-gh-gbloom reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-gbloom sfc-gh-gbloom left a comment

Choose a reason for hiding this comment

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

Why not add integration tests for the release directive commands? We can add additional tests for release channels once they are available too

RELEASE-NOTES.md Outdated Show resolved Hide resolved
src/snowflake/cli/_plugins/nativeapp/entities/application_package.py Outdated Show resolved Hide resolved
src/snowflake/cli/_plugins/nativeapp/entities/application_package.py Outdated Show resolved Hide resolved
src/snowflake/cli/_plugins/nativeapp/entities/application_package.py Show resolved Hide resolved
src/snowflake/cli/_plugins/nativeapp/release_directive/commands.py
Comment on lines +51 to +55
channel: Optional[str] = typer.Option(
default=None,
show_default=False,
help="The release channel to use when listing release directives. If not provided, release directives from all release channels are listed.",
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we hide the channel options until release channels are available in prod?

Copy link
Contributor Author

@sfc-gh-melnacouzi sfc-gh-melnacouzi Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

They will be available in the upcoming release, and they are optional anyways.
Snow CLI will not deploy before January, so I believe we should be ok.
Release channel commands (snow app release-channel) will also follow so they will be visible regardless.

@sfc-gh-melnacouzi
Copy link
Contributor Author

sfc-gh-melnacouzi commented Dec 9, 2024 •
edited
Loading

Why not add integration tests for the release directive commands? We can add additional tests for release channels once they are available too

Will submit this as a follow up - Waiting for the UI Parameter FEATURE_RELEASE_CHANNELS to be available (ETA of deployment is tomorrow) before i can write integ tests. Otherwise, some of the commands (show release channels) are not available, and can't check the UI Parameter because the default is true.

sfc-gh-gbloom
sfc-gh-gbloom previously approved these changes Dec 9, 2024
View reviewed changes
@sfc-gh-melnacouzi sfc-gh-melnacouzi enabled auto-merge (squash) December 9, 2024 21:39
sfc-gh-astus
sfc-gh-astus previously approved these changes Dec 10, 2024
View reviewed changes
@sfc-gh-melnacouzi sfc-gh-melnacouzi force-pushed the melnacouzi-add-release-directives-commands branch from 3fab28d to 514fc72 Compare December 11, 2024 13:23
@sfc-gh-melnacouzi sfc-gh-melnacouzi merged commit 8f559e3 into main Dec 11, 2024
20 checks passed
@sfc-gh-melnacouzi sfc-gh-melnacouzi deleted the melnacouzi-add-release-directives-commands branch December 11, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-gbloom sfc-gh-gbloom sfc-gh-gbloom approved these changes

@sfc-gh-astus sfc-gh-astus sfc-gh-astus approved these changes

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-melnacouzi @sfc-gh-astus @sfc-gh-gbloom