Defer package script conversion until PDFv2 is loaded into Pydantic #1735
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
| @@ -83,23 +87,31 @@ def _is_field_defined(template_context: Optional[Dict[str, Any]], *path: str) -> | |||
|
|
|||
| def convert_project_definition_to_v2( | |||
| project_root: Path, | |||
| pd: ProjectDefinition, | |||
| definition_v1: ProjectDefinition, | |||
There was a problem hiding this comment.
renamed this since we now also have definition_v2 below
| # don't convert them here, conversion is deferred until | ||
| # the final v2 Pydantic model is available | ||
| # (see _convert_templates_in_files()) | ||
| pass |
There was a problem hiding this comment.
left this in instead of removing the if as a marker that this case is indeed being considered but consciously ignored
| @@ -8,3 +8,4 @@ | |||
|
|
|||
| CREATE OR ALTER VERSIONED SCHEMA <% ctx.env.schema_name %>; | |||
| EXECUTE IMMEDIATE from '/another_script.sql'; | |||
| select 'ctx.native_app.name: <% ctx.native_app.name %>'; | |||
There was a problem hiding this comment.
this tests that even if the PDF is implicitly converted to v2, the template context is still v1
There was a problem hiding this comment.
but shouldn't we check the expansion somewhere then?
There was a problem hiding this comment.
We don't care about the expansion, we're just testing that arbitrary files in v1 projects can continue to use v1 template syntax. This test fails if the template context becomes converted implicitly (which is the original bug that Michel found)
sfc-gh-fcampbell
force-pushed
the
frank-fix-package-script-conversion
branch
from
d6fada8
to
9d15792
Compare
8 tasks
Comment on lines
-296
to
-304
|
|
||
| # Override the template context so that templates refer to the new entities | ||
| # Reuse the old ctx.env and other top-level keys in the template context | ||
| # since they don't change between v1 and v2 | ||
| pdfv2_dump = pdfv2.model_dump( | ||
| exclude_none=True, warnings=False, by_alias=True | ||
| ) | ||
| new_ctx = pdfv2_dump | dict(env=cm.template_context["ctx"]["env"]) | ||
| cm.override_template_context = cm.template_context | dict(ctx=new_ctx) |
There was a problem hiding this comment.
stop overriding the template context, templates should be able to refer to fields as they exist in the PDF on-disk, since that's the one that the user sees
Comment on lines
-272
to
-275
| def _make_template(template: str) -> str: | ||
| return f"{PROJECT_TEMPLATE_VARIABLE_OPENING} {template} {PROJECT_TEMPLATE_VARIABLE_CLOSING}" | ||
|
|
||
| def _convert_package_script_files(package_scripts: list[str]): |
There was a problem hiding this comment.
moved to below nearly verbatim
sfc-gh-turbaszek
approved these changes
Oct 17, 2024
| @@ -8,3 +8,4 @@ | |||
|
|
|||
| CREATE OR ALTER VERSIONED SCHEMA <% ctx.env.schema_name %>; | |||
| EXECUTE IMMEDIATE from '/another_script.sql'; | |||
| select 'ctx.native_app.name: <% ctx.native_app.name %>'; | |||
There was a problem hiding this comment.
but shouldn't we check the expansion somewhere then?
sfc-gh-bdufour
approved these changes
Oct 18, 2024
sfc-gh-turbaszek
pushed a commit
that referenced
this pull request
Oct 21, 2024
…1735) When converting PDFv1 to PDFv2, we need to convert package scripts to post-deploy hooks, so we have to convert the `{{ package_name }}` template into the normal template syntax. When converting using `snow helpers v1-to-v2`, we convert `{{ package_name }}` to `<% ctx.entities.pkg.identifier %>` since we know that the next time the CLI is run, the template context will be v2-based and that `ctx.entities.pkg.identifier` will be a valid reference. When converting in-memory however, the template context is v1-based and since the conversion has to be transparent to the user (they could have other files with templates still using v1 references in it), we can't override the template context to v2, so `ctx.entities.pkg.identifier` is an invalid reference. Fortunately for in-memory conversions, package scripts are converted to post-deploy hooks using temp files (to avoid overwriting the user's files), so we can just insert the package name directly into the converted script instead of having to use a template reference. This requires us to defer conversion of package scripts until after the v2 definition is loaded into Pydantic since there are validators that can change the values of fields (like the test resource suffix being added to the package identifier).
sfc-gh-turbaszek
pushed a commit
that referenced
this pull request
Oct 21, 2024
…1735) When converting PDFv1 to PDFv2, we need to convert package scripts to post-deploy hooks, so we have to convert the `{{ package_name }}` template into the normal template syntax. When converting using `snow helpers v1-to-v2`, we convert `{{ package_name }}` to `<% ctx.entities.pkg.identifier %>` since we know that the next time the CLI is run, the template context will be v2-based and that `ctx.entities.pkg.identifier` will be a valid reference. When converting in-memory however, the template context is v1-based and since the conversion has to be transparent to the user (they could have other files with templates still using v1 references in it), we can't override the template context to v2, so `ctx.entities.pkg.identifier` is an invalid reference. Fortunately for in-memory conversions, package scripts are converted to post-deploy hooks using temp files (to avoid overwriting the user's files), so we can just insert the package name directly into the converted script instead of having to use a template reference. This requires us to defer conversion of package scripts until after the v2 definition is loaded into Pydantic since there are validators that can change the values of fields (like the test resource suffix being added to the package identifier).
sfc-gh-turbaszek
pushed a commit
that referenced
this pull request
Oct 21, 2024
…1735) When converting PDFv1 to PDFv2, we need to convert package scripts to post-deploy hooks, so we have to convert the `{{ package_name }}` template into the normal template syntax. When converting using `snow helpers v1-to-v2`, we convert `{{ package_name }}` to `<% ctx.entities.pkg.identifier %>` since we know that the next time the CLI is run, the template context will be v2-based and that `ctx.entities.pkg.identifier` will be a valid reference. When converting in-memory however, the template context is v1-based and since the conversion has to be transparent to the user (they could have other files with templates still using v1 references in it), we can't override the template context to v2, so `ctx.entities.pkg.identifier` is an invalid reference. Fortunately for in-memory conversions, package scripts are converted to post-deploy hooks using temp files (to avoid overwriting the user's files), so we can just insert the package name directly into the converted script instead of having to use a template reference. This requires us to defer conversion of package scripts until after the v2 definition is loaded into Pydantic since there are validators that can change the values of fields (like the test resource suffix being added to the package identifier).
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
When converting PDFv1 to PDFv2, we need to convert package scripts to post-deploy hooks, so we have to convert the
{{ package_name }}template into the normal template syntax.When converting using
snow helpers v1-to-v2, we convert{{ package_name }}to<% ctx.entities.pkg.identifier %>since we know that the next time the CLI is run, the template context will be v2-based and thatctx.entities.pkg.identifierwill be a valid reference.When converting in-memory however, the template context is v1-based and since the conversion has to be transparent to the user (they could have other files with templates still using v1 references in it), we can't override the template context to v2, so
ctx.entities.pkg.identifieris an invalid reference.Fortunately for in-memory conversions, package scripts are converted to post-deploy hooks using temp files (to avoid overwriting the user's files), so we can just insert the package name directly into the converted script instead of having to use a template reference. This requires us to defer conversion of package scripts until after the v2 definition is loaded into Pydantic since there are validators that can change the values of fields (like the test resource suffix being added to the package identifier).