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

Refactor app package deploy logic from NativeAppManager to ApplicationPackageEntity #1442

Merged
merged 14 commits into from
Aug 26, 2024

Conversation

sfc-gh-gbloom
Copy link
Contributor

@sfc-gh-gbloom sfc-gh-gbloom commented Aug 13, 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

This is a preparation PR to enable snow ws deploy for application package entities in the following PR. The end goal is to reuse the same logic in both NativeAppManager.deploy() and ApplicationPackageEntity/ApplicationEntity actions.

Extracted methods

The first step is to extract the minimal logic out of NativeAppManager to enable ApplicationPackageEntity.action_deploy() (which will be very similar to NativeAppManager.deploy()). The following methods were moved from NativeAppManager and modified to be stateless:

ApplicationPackageEntity

  • get_existing_app_pkg_info()
  • get_app_pkg_distribution_in_snowflake()
  • verify_project_distribution()
  • use_package_warehouse()
  • apply_package_scripts()
  • create_app_package()

utils

  • generic_sql_error_handler()
  • ensure_correct_owner()
  • sync_deploy_root_with_stage()
  • _get_stage_paths_to_sync()
  • execute_post_deploy_hooks()
  • _execute_sql_script()
  • expand_script_templates()

Even though they are now just proxies, the methods in NativeAppManager were kept with the same signatures in order to not break the interface of the class, as other flows call them and were not refactored yet (teardown, version, etc).

SqlExecutor

Added the SqlExecutor utility class to be able to execute SQL without having to inherit directly from SqlExecutionMixin.

Manual tests

snow app deploy
snow app version create v1
snow app version list
snow app version drop v1
snow app teardown
snow app run

@sfc-gh-gbloom sfc-gh-gbloom marked this pull request as ready for review August 16, 2024 14:47
@sfc-gh-gbloom sfc-gh-gbloom requested review from a team as code owners August 16, 2024 14:47
Base automatically changed from ws-app-package-bundle to main August 19, 2024 14:35
@sfc-gh-gbloom sfc-gh-gbloom force-pushed the refactor-app-package-entity-namanager branch from c47fa3d to 9f63585 Compare August 19, 2024 14:54
@sfc-gh-gbloom sfc-gh-gbloom force-pushed the refactor-app-package-entity-namanager branch from 0fcdcbe to 0be7d97 Compare August 20, 2024 15:14
@sfc-gh-gbloom sfc-gh-gbloom enabled auto-merge (squash) August 22, 2024 14:49
sfc-gh-cgorrie
sfc-gh-cgorrie previously approved these changes Aug 22, 2024
@sfc-gh-gbloom sfc-gh-gbloom merged commit a0d87d0 into main Aug 26, 2024
18 checks passed
@sfc-gh-gbloom sfc-gh-gbloom deleted the refactor-app-package-entity-namanager branch August 26, 2024 18:45
sfc-gh-sichen pushed a commit that referenced this pull request Oct 17, 2024
…nPackageEntity (#1442)

* refactor app package deploy logic from NativeAppManager to ApplicationPackageEntity

* remove global sql executor

* get_sql_executor docstring
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.

4 participants