Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions template/.pre-commit-config.yaml.j2
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ repos:

- repo: local
hooks:
- id: check-namespaced-defines
name: check-namespaced-defines
language: system
entry: python3 scripts/check_namespaced_defines.py
files: ^deploy/helm/[^/]+/templates/
Comment thread
Techassi marked this conversation as resolved.

- id: regenerate-charts
name: regenerate-charts
language: system
Expand Down
94 changes: 94 additions & 0 deletions template/scripts/check_namespaced_defines.py
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):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

python3 -m unittest check_namespaced_defines.py

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

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?

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?

I would say add it to prek. For people who have it enabled locally to run on git commit, it will immediately be tested and validates that any potential change still passes. Additionally, we can more easily scope based on file paths.

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)