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

Update setup processor according to new result schema #1448

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

sfc-gh-bdufour
Copy link
Contributor

@sfc-gh-bdufour sfc-gh-bdufour commented Aug 16, 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

Updating the setup processor driver to match ToT in the snowflake.app package

Testing

Since the snowflake.app package is still private, this was tested manually using an env var override. Integ and E2E tests will be added when the feature is closer to PrPr.

@sfc-gh-bdufour sfc-gh-bdufour force-pushed the bdufour-sqlgen-m2 branch 3 times, most recently from 69a3096 to 5c6f613 Compare August 20, 2024 21:52
@@ -34,7 +36,7 @@
)

SNOWPARK_PROCESSOR = "snowpark"
NA_SETUP_PROCESSOR = "native-app-setup"
NA_SETUP_PROCESSOR = "native app setup"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow I keep forgetting that natural strings like this are more natural in YAML.

Comment on lines +115 to +116
processor_ctx = copy.copy(self._bundle_ctx)
processor_subdirectory = re.sub(r"[^a-zA-Z0-9_$]", "_", processor_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the logic so that each processor uses its own subdirectory for its artifacts, enforced by the framework. Right now each processor was responsible for doing this, so I just centralized the same logic to facilitate the implementation of new processors.

@sfc-gh-bdufour sfc-gh-bdufour marked this pull request as ready for review August 26, 2024 14:36
@sfc-gh-bdufour sfc-gh-bdufour requested a review from a team as a code owner August 26, 2024 14:36
processor_ctx = copy.copy(self._bundle_ctx)
processor_subdirectory = re.sub(r"[^a-zA-Z0-9_$]", "_", processor_name)
processor_ctx.bundle_root = (
self._bundle_ctx.bundle_root / processor_subdirectory
Copy link
Contributor

Choose a reason for hiding this comment

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

if each processor gets its own directory, how do we chain processors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only made that change for the bundle root (which is the processor's sandbox) and the generated directory, which is similarly processor-specific. The deploy root remains unchanged, as processors should always be reading and writing from/to the deploy root so they can be chained.

sfc-gh-pjafari
sfc-gh-pjafari previously approved these changes Aug 26, 2024
@@ -40,6 +46,16 @@
DEFAULT_TIMEOUT = 30
DRIVER_PATH = Path(__file__).parent / "setup_driver.py.source"

log = logging.getLogger(__name__)
Copy link
Contributor

Choose a reason for hiding this comment

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

wish we had named this logger instead of log everywhere :')

Comment on lines +210 to +215
for inst in mod.get("instructions", []):
if inst.get("type") == "set":
payload = inst.get("payload")
if payload:
key = payload.get("key")
value = payload.get("value")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: too much nesting here, would be nice to factor this out if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll keep this in mind when I rework this logic in the near future.

@sfc-gh-bdufour sfc-gh-bdufour enabled auto-merge (squash) August 27, 2024 14:02
@sfc-gh-bdufour sfc-gh-bdufour merged commit 24c1918 into main Aug 27, 2024
16 checks passed
@sfc-gh-bdufour sfc-gh-bdufour deleted the bdufour-sqlgen-m2 branch August 27, 2024 18:02
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