Allow converting PDFv1 to PDFv2 in-memory only #1670
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sfc-gh-fcampbell
commented
Oct 4, 2024
Comment on lines
56
to
+59
| # FIXME: this property only exists to help implement | ||
| # nativeapp_definition_v2_to_v1. Consider changing the way | ||
| # this calculation is provided to commands in order to remove | ||
| # this logic (then make project_definition a non-cloned @property) | ||
| # nativeapp_definition_v2_to_v1 and single_app_and_package. | ||
| # Consider changing the way this calculation is provided to commands | ||
| # in order to remove this logic (then make project_definition a non-cloned @property) |
There was a problem hiding this comment.
I know this is proliferating a self-proclaimed undesirable pattern, but I think it's worth it for the simplicity of the change. We'll have a better opportunity to fix this once this v1-to-v2 refactor is done.
sfc-gh-fcampbell
commented
Oct 4, 2024
Comment on lines
+113
to
+114
| if envs is not None: | ||
| data["env"] = envs |
There was a problem hiding this comment.
None isn't a valid value for env according to the pydantic model. When saving the converted PDF to disk, None is omitted but when converting in-memory, we need to omit it here.
sfc-gh-fcampbell
force-pushed
the
frank-in-memory-pdf-conversion
branch
3 times, most recently
from
d51894d
to
07bf2a7
Compare
sfc-gh-fcampbell
changed the title
sfc-gh-fcampbell
force-pushed
the
frank-in-memory-pdf-conversion
branch
from
07bf2a7
to
8cf6f44
Compare
sfc-gh-bdufour
approved these changes
Oct 4, 2024
sfc-gh-astus
approved these changes
Oct 4, 2024
sfc-gh-sichen
pushed a commit
that referenced
this pull request
Oct 17, 2024
Allows converting a v1 definition to v2 only in-memory, without saving the resultant v2 definition to `snowflake.yml`. This will be used in a future decorator added in #1667 to allow v1 commands to operate natively on v2 entities by implicitly converting the definition. Since the definition conversion process currently updates package script files in-place, we need a way to isolate those changes to a temp directory instead of touching the user's project directory. This is achieved by creating a tempdir to hold these conversions and by replacing the package script filenames in the project definition with absolute paths to those temp files, taking advantage of the (documented) fact that `Path(...) / "/tmp/foo"` ignores the first `Path` since the second is absolute, resulting in just `Path("/tmp/foo")`. Since the definition contents will never be emitted to disk to be loaded in the future, another thing we need to do is evaluate templates in the converted project definition immediately instead of returning a definition with tags in it. In addition, since we're modifying the project definition in-memory, we also need to override the template context to be used for future template expansions in other files, so this ability was added to the CLI global context manager.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pre-review checklist
Changes description
Extracted from #1667 to get separate CLI team codeowner review.
Allows converting a v1 definition to v2 only in-memory, without saving the resultant v2 definition to
snowflake.yml. This will be used in a future decorator added in #1667 to allow v1 commands to operate natively on v2 entities by implicitly converting the definition.Since the definition conversion process currently updates package script files in-place, we need a way to isolate those changes to a temp directory instead of touching the user's project directory. This is achieved by creating a tempdir to hold these conversions and by replacing the package script filenames in the project definition with absolute paths to those temp files, taking advantage of the (documented) fact that
Path(...) / "/tmp/foo"ignores the firstPathsince the second is absolute, resulting in justPath("/tmp/foo").Since the definition contents will never be emitted to disk to be loaded in the future, another thing we need to do is evaluate templates in the converted project definition immediately instead of returning a definition with tags in it.
In addition, since we're modifying the project definition in-memory, we also need to override the template context to be used for future template expansions in other files, so this ability was added to the CLI global context manager.