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

Template file extension #40

Merged
merged 7 commits into from
Nov 30, 2023
Merged

Template file extension #40

merged 7 commits into from
Nov 30, 2023

Conversation

ilkka
Copy link
Contributor

@ilkka ilkka commented Aug 28, 2023

This PR changes template rendering such that only template files that end in ".tmpl" get run through the template engine. Other files in the templates directory get rendered as-is.

Not sure if it would be worth it to also rename the "templates" directory to something else since it will now also contain non-templated things?

Closes #39.

@ilkka ilkka self-assigned this Aug 28, 2023
@ilkka ilkka requested a review from majori August 28, 2023 10:29
@ilkka ilkka force-pushed the template-file-extension branch from abee627 to 9c99b12 Compare August 28, 2023 10:30
@majori
Copy link
Member

majori commented Aug 28, 2023

I was wondering the case where for example env.local.tmpl is wanted to be copied to the repo. Should it be then be env.local.tmpl.tmpl in the recipe so the wanted prefix is not removed in project context?

@ilkka
Copy link
Contributor Author

ilkka commented Aug 28, 2023

Oof :D that's a good point, and yeah that should be a viable workaround. It's not very nice though, I wonder if some other sort of "filename escaping" would be better, like ._tmpl -> .tmpl or whatever

pkg/engine/engine.go Outdated Show resolved Hide resolved
@majori
Copy link
Member

majori commented Aug 28, 2023

Not sure if it would be worth it to also rename the "templates" directory to something else

Yeah, I agree that it is then misleading if all of the files are not rendered

@ilkka
Copy link
Contributor Author

ilkka commented Aug 28, 2023

I'm thinking is skipping non-.tmpl files feature of a engine, or should it be same for all engines? So when calling recipe.engine.Render() method, should only the templates be passed as a parameter, or all files

Should there be a "NOP-engine" that gets all the non-template files?

@majori
Copy link
Member

majori commented Aug 28, 2023

Should there be a "NOP-engine" that gets all the non-template files?

Or is it just something what happens before recipe.engine.Render() is called?

@ilkka
Copy link
Contributor Author

ilkka commented Aug 29, 2023

I found another way in which this is a problematic design. This is demonstrated by TestRecipeRenderChecksums: it uses TestRenderEngine which doesn't know anything about this name change, and therefore the test will start failing if the template name gets the .tmpl file extension. So the file extension is kind of part of the render engine API contract now, but in a really implicit way.

That makes me think if the big picture here isn't "skip templating", but actually that recipes should be able to have many different types of files all processed with different engines -- one of which is gotpl and uses the .tmpl filename extension.

@majori
Copy link
Member

majori commented Aug 29, 2023

should be able to have many different types of files all processed with different engines

Interesting idea that there could be multiple engines in use at the same time

@ilkka ilkka force-pushed the template-file-extension branch from 889fc63 to 51d9442 Compare September 11, 2023 08:31
@majori majori force-pushed the main branch 7 times, most recently from 774e0e0 to 516f44f Compare September 21, 2023 10:51
@ilkka ilkka force-pushed the template-file-extension branch from af8c200 to 5ef172a Compare October 13, 2023 11:11
@majori majori force-pushed the main branch 2 times, most recently from dcef748 to cd01f4a Compare October 31, 2023 12:31
@majori majori force-pushed the main branch 3 times, most recently from ac6aa39 to fbb5670 Compare November 12, 2023 12:21
Ilkka Poutanen added 4 commits November 30, 2023 12:16
Setting this to a non-empty value would cause only recipe files that end with this extension get templated (maybe validate that it is either empty or starts with a single period).
The engine was ever explicitly set in testing, otherwise it always used the default `engine.Engine{}`. It also then introduces a possible runtime error if the engine was not set for some reason. This change would make that a compile time error instead.
The default suffix is the empty string so it always matches.
@ilkka ilkka force-pushed the template-file-extension branch from 5ef172a to e5703a8 Compare November 30, 2023 11:31
@ilkka ilkka requested a review from majori November 30, 2023 11:32
@ilkka
Copy link
Contributor Author

ilkka commented Nov 30, 2023

Alright, I exploded everything and did another version :D In this version the determination of whether to use templating is not is not with the engine but with the recipe. This circumvents the problem with TestRenderEngine that I mentioned above. It's also opt-in because if there is no template extension defined on the recipe, everything gets rendered like before.

pkg/recipe/metadata.go Outdated Show resolved Hide resolved
pkg/recipe/metadata.go Show resolved Hide resolved
Ilkka Poutanen and others added 2 commits November 30, 2023 13:54
Co-authored-by: Antti Kivimäki <[email protected]>
fixup: fix new extension validation check
@ilkka ilkka force-pushed the template-file-extension branch from 648e748 to 36823a6 Compare November 30, 2023 11:54
cmd/docs/templates/schema.tmpl Outdated Show resolved Hide resolved
@ilkka
Copy link
Contributor Author

ilkka commented Nov 30, 2023

Eh, here I was rebasing and force pushing and forgot that we always squash anyway 🤦🏻

@ilkka ilkka merged commit a6eb0f7 into main Nov 30, 2023
4 checks passed
@ilkka ilkka deleted the template-file-extension branch November 30, 2023 11:59
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.

Allow skip template rendering for specific files
2 participants