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

Don't error on a PDFv2 with multiple entities if it's not required #1606

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Don't error on a PDFv2 with multiple entities if it's not required #1606

merged 1 commit into from
Sep 20, 2024
+24 −6

Conversation

sfc-gh-fcampbell
Copy link
Collaborator

@sfc-gh-fcampbell sfc-gh-fcampbell commented Sep 20, 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

Fixes a bug where a command that doesn't use the app entity would show

More than one application entity exists in the project definition file, specify --app-entity-id to choose which one to operate on.

when there was more than one app entity in snowflake.yml. The new logic just ignores all the app entities since the command doesn't use it anyways.

sfc-gh-fcampbell
sfc-gh-fcampbell commented Sep 20, 2024
View reviewed changes
src/snowflake/cli/_plugins/nativeapp/v2_conversions/v2_to_v1_decorator.py
entity = next(iter(entities.values()))
elif len(entities) > 1:
# If there are multiple package entities, the user must specify which one to use
elif len(entities) > 1 and required:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the change, everything else is just cleaning up comments

sfc-gh-cgorrie
sfc-gh-cgorrie approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-cgorrie sfc-gh-cgorrie left a comment

Choose a reason for hiding this comment

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

LGTM; we now get the right error message without --package-entity-id and are able to proceed with one and no app entity specified for bundle|diff|validate

@sfc-gh-fcampbell sfc-gh-fcampbell marked this pull request as ready for review September 20, 2024 16:08
@sfc-gh-fcampbell sfc-gh-fcampbell requested a review from a team as a code owner September 20, 2024 16:08
@sfc-gh-fcampbell sfc-gh-fcampbell enabled auto-merge (squash) September 20, 2024 16:08
@sfc-gh-fcampbell sfc-gh-fcampbell merged commit 05a6689 into main Sep 20, 2024
19 checks passed
@sfc-gh-fcampbell sfc-gh-fcampbell deleted the frank-dont-error-multiple-apps-not-required branch September 20, 2024 16:39
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

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

Successfully merging this pull request may close these issues.
2 participants
@sfc-gh-fcampbell @sfc-gh-cgorrie