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

Backfill tests for the tpl function #11456

Merged
merged 5 commits into from
Jul 13, 2023
Merged

Backfill tests for the tpl function #11456

merged 5 commits into from
Jul 13, 2023

Conversation

greed42
Copy link
Contributor

@greed42 greed42 commented Oct 20, 2022

What this PR does / why we need it:

This PR contains test-only changes for the tpl function to cover all the sharp edges I encountered creating PR http://github.com/helm/helm/pull/11351 and trying to use http://github.com/helm/helm/pull/8371 instead.

It verifies current behaviour, especially the aspects affected by those PRs. (Or which I found mattered to our charts before composing http://github.com/helm/helm/pull/11351.)

Special notes for your reviewer:

I have run these tests against both v3.9.4 and v3.10.1 tags as well as the HEAD of main.

Both proposed tpl performance improvement PRs will fail after this PR is merged. I have a second set of cherries to pick onto http://github.com/helm/helm/pull/11351; I believe the changes are reasonable. (They involve access to define in the manifest invoking tpl and being able to override defines from partial files.)

However, due to the nature of the (t) ExecuteTemplate(...) golang function, I now think that http://github.com/helm/helm/pull/8371 cannot be used without effectively turning it into something very similar to http://github.com/helm/helm/pull/11351: it must use (t) Execute(...) instead.

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

<helm#8371>

This covers:
  - `tpl` text can `include` a `define` provided in a partial file
  - `tpl` text can `include` a `define` provided in its text
  - `tpl` text can be loaded via `.Files.Get`

Signed-off-by: Graham Reed <greed@7deadly.org>
Signed-off-by: Graham Reed <greed@7deadly.org>
Signed-off-by: Graham Reed <greed@7deadly.org>
Signed-off-by: Graham Reed <greed@7deadly.org>
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 20, 2022
@greed42 greed42 mentioned this pull request Oct 20, 2022
3 tasks
Signed-off-by: Graham Reed <greed@7deadly.org>
@simonhege
Copy link

In order to progress on the tpl performance issue (cf. #8002 and #11351) would it be possible to review and merge this PR?

Copy link
Contributor

@joejulian joejulian left a comment

Choose a reason for hiding this comment

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

This is great!

@joejulian joejulian added this to the 3.13.0 milestone Jul 11, 2023
Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

LGTM

@joejulian joejulian merged commit ec1d0d8 into helm:main Jul 13, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants