-
-
Notifications
You must be signed in to change notification settings - Fork 2
feat(template): Check that Helm defined templates carry the chart name #647
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| """ | ||
| Fails when a Helm defined template, or a call to one, is not prefixed with the | ||
| name of the chart it lives in. | ||
|
|
||
| To run tests for this script: | ||
| python3 -m unittest check_namespaced_defines.py | ||
| """ | ||
|
|
||
| import re | ||
| import unittest | ||
| from pathlib import Path | ||
|
|
||
| NAME_PATTERN = r'\b(?:define|include|template)\s+"([^"]+)"' | ||
| CHART_PATTERN = r"(?:^|/)deploy/helm/(?P<chart>[^/]+)/templates/" | ||
|
|
||
|
|
||
| def chart_of(path): | ||
| """The chart whose templates directory holds path, or None if it is outside one.""" | ||
| found = re.search(CHART_PATTERN, Path(path).as_posix()) | ||
| return found.group("chart") if found else None | ||
|
|
||
|
|
||
| def unprefixed(text, chart): | ||
| """The defined template names in text, and the calls to them, lacking the chart prefix.""" | ||
| names = re.findall(NAME_PATTERN, text) | ||
| return sorted({name for name in names if not name.startswith(f"{chart}.")}) | ||
|
|
||
|
|
||
| class TestCoreMethods(unittest.TestCase): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is generally nice that there are some tests, but how exactly are they executed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's documented right at the beginning of the file. They are not executed automatically.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But.... it might be relatively easy to add them to CI, I'll see about that.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be possible. Question is if it should go into the prek file and then tested indirectly via github actions or directly into github actions and leave the test out of prek. Opinions?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah I see. I kinda skipped that block of text. Could we maybe leave a simple comment here either stating the command or referring back to the top of the file?
I would say add it to prek. For people who have it enabled locally to run on Only having it in CI as a separate workflow delays the time one sees if the tests still pass - potentially leading to a follow-up fix commit. |
||
| def test_chart_of(self): | ||
| self.assertEqual( | ||
| chart_of("deploy/helm/trino-operator/templates/x.yaml"), "trino-operator" | ||
| ) | ||
| # An absolute path has to give the same answer, or a manual run invents a chart name. | ||
| self.assertEqual( | ||
| chart_of("/tmp/wt/deploy/helm/secret-operator/templates/a/b.yaml"), | ||
| "secret-operator", | ||
| ) | ||
| self.assertIsNone(chart_of("deploy/helm/trino-operator/values.yaml")) | ||
| self.assertIsNone(chart_of("README.md")) | ||
|
|
||
| def test_prefixed_is_accepted(self): | ||
| text = '{{- define "trino-operator.labels" -}}\n{{ include "trino-operator.chart" . }}\n' | ||
| self.assertEqual(unprefixed(text, "trino-operator"), []) | ||
|
|
||
| def test_unprefixed_define_is_reported(self): | ||
| self.assertEqual( | ||
| unprefixed('{{- define "helper.thing" -}}', "trino-operator"), | ||
| ["helper.thing"], | ||
| ) | ||
|
|
||
| def test_unprefixed_call_is_reported(self): | ||
| # A renamed definition whose call sites did not move with it stops the chart rendering. | ||
| text = ( | ||
| '{{ include "operator.fullname" . }}\n{{ template "operator.labels" . }}\n' | ||
| ) | ||
| self.assertEqual( | ||
| unprefixed(text, "trino-operator"), ["operator.fullname", "operator.labels"] | ||
| ) | ||
|
|
||
| def test_keyword_must_stand_alone(self): | ||
| # Without a word boundary any identifier ending in the keyword matches. | ||
| self.assertEqual( | ||
| unprefixed('{{ .Values.xinclude "bar.baz" }}', "foo-operator"), [] | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| import sys | ||
|
|
||
| if not sys.argv[1:]: | ||
| print(f"usage: {sys.argv[0]} deploy/helm/<chart>/templates/*", file=sys.stderr) | ||
| sys.exit(2) | ||
|
|
||
| failed = False | ||
| for path in sys.argv[1:]: | ||
| chart = chart_of(path) | ||
| if chart is None: | ||
| print( | ||
| f"{sys.argv[0]}: {path} is not inside deploy/helm/<chart>/templates/", | ||
| file=sys.stderr, | ||
| ) | ||
| sys.exit(2) | ||
| text = Path(path).read_text(encoding="utf-8") | ||
| unprefixed_names = unprefixed(text, chart) | ||
| if unprefixed_names: | ||
| failed = True | ||
| print( | ||
| f"{path}: defined templates and the calls to them must be prefixed with '{chart}.'" | ||
| ) | ||
| for name in unprefixed_names: | ||
| print(f" {name}") | ||
|
|
||
| sys.exit(1 if failed else 0) | ||
Uh oh!
There was an error while loading. Please reload this page.