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.

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

SNOW-1706990 Update/remove tests that use NativeAppRunProcessor #1726

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

sfc-gh-fcampbell
Copy link
Contributor

@sfc-gh-fcampbell sfc-gh-fcampbell commented Oct 15, 2024

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

Update/remove tests that use NativeAppRunProcessor. Removed tests/nativeapp/test_package_scripts.py because package scripts are now converted to post-deploy hooks for all commands, the package script code isn't used anymore.

return drop_application_before_upgrade(cascade=True)
else:
generic_sql_error_handler(err)
self.drop_application_before_upgrade(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just moved this to a static so it can be called externally (for now)

@sfc-gh-fcampbell sfc-gh-fcampbell force-pushed the frank-move-run-processor-tests branch 2 times, most recently from b21654b to fabe3f9 Compare October 15, 2024 20:49
@sfc-gh-fcampbell sfc-gh-fcampbell marked this pull request as ready for review October 16, 2024 14:08
@sfc-gh-fcampbell sfc-gh-fcampbell requested a review from a team as a code owner October 16, 2024 14:08
@sfc-gh-fcampbell sfc-gh-fcampbell enabled auto-merge (squash) October 16, 2024 14:18
@sfc-gh-pjafari sfc-gh-pjafari mentioned this pull request Oct 16, 2024
8 tasks
Comment on lines 161 to 162
Usage:
Create a pdf dict with definition_version: "1", native_app with faker-generated name and an empty artifacts list and
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still true that we create "1" PDF here? This whole usage seems a bit outdated TBH, since PdfFactory shouldn't really be used directly most (all?) of the time. Should we move the whole usage thing to a file comment at the very top and use it to document the overall usage of the factories defined here? We can use something like V2 as a running example, since that's intended to be the default.

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 yeah, i can move this to PdfV10Factory. Can i do the overall comment update separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the PDF factories are meant to be used externally, in the case where you want a project definition and snowflake.yml file, but you don't need other files on-disk

@sfc-gh-fcampbell sfc-gh-fcampbell merged commit 3479d4b into main Oct 17, 2024
19 checks passed
@sfc-gh-fcampbell sfc-gh-fcampbell deleted the frank-move-run-processor-tests branch October 17, 2024 18:00
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