mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-30 00:27:04 +09:00
fix(cdi): stop make update from deleting first-party templates (#4097)
## What this PR does `make update` in `packages/system/kubevirt-cdi` began with `rm -rf templates` and then refetched one file, `cdi-cr.yaml`. Nothing in that directory is purely vendored. Two of the three manifests, the upload-proxy Ingress and TLSRoute from the Gateway API work, are ours and no fetch recreates them, so the next CDI bump would have dropped upload-proxy exposure from the chart and still exited 0. The third is `cdi-cr.yaml` itself, which despite its name carries Helm templating, the `ExpandDisks` gate, a toleration and the two RBAC documents that preauthorize cloning from `cozy-public`; the fetch overwrites it, and removing the wipe does not change that. That recipe now opens with `mkdir -p templates` and deletes nothing, which is where `packages/system/kubevirt-operator` landed at the last KubeVirt bump. The wipe bought nothing there anyway: `wget -O` truncates the file it writes. `kubevirt-cdi-operator` has the same recipe shape, and the same edit there would be a regression rather than a fix. It applies two patches, and since #3920 moved them to `-p4` they resolve, so a hunk upstream has moved out from under leaves `templates/cdi-operator.yaml.rej`; its `awk -i inplace` step leaves `templates/cdi-operator.yaml.gawk.XXXXXX` if gawk dies abnormally. Helm renders every file under `templates/`, and I reproduced both: the reject fails the package with a YAML parse error, and the gawk temp renders a duplicate object at exit 0. The wipe is what takes both, so removing it for symmetry would trade a latent loss for a live one, and the quieter of the two is the one that survives. Deleting the wipe there wants the recipe restructured to transform outside `templates/` and move the result in once, which is what `kubevirt-operator` does today and what its own comment gives this exact reason for. That is a larger change than this one and belongs on its own; #4383 makes it. Meanwhile the scan below covers the package: I added a first-party file to its `templates/` and the guard reported it, so the day that package grows one, this fails rather than the file being lost. `hack/package-update-templates.bats` guards this across the tree rather than in these two packages. It scans every package Makefile and fails when a recipe that clears its own `templates/` leaves behind a tracked file it never names. Put the removal back into `kubevirt-cdi` and it goes red naming both upload-proxy files. A file counts as named only outside the Makefile's own `NAME` and `NAMESPACE`, however the assignment is written. The match is on the variable name rather than on a list of spellings: the tree already carries three of them, `export NAME=`, the bare `NAME=`, and `NAME := ` in `packages/core/testing`, and the hole is identical in each. Without that, a package called `foo-bar` satisfies the check for its own `templates/bar.yaml` through its identity alone. Nothing the scan reaches is exempted that way as the tree stands, so the exclusion is there for the first package that would be: rename the asset in `kubevirt-cdi-operator`'s recipe and put the removal back, and the guard reports the orphaned template only with the exclusion in place. The two variables are matched by name rather than by a `[A-Z_]+=` shape, because a shape that broad is one unspaced assignment away from swallowing a variable that does name templates, which is how `etcd-operator-crds` names all five of its CRDs. A control test runs the same helpers over a fixture broken the same way, over one whose name swallows a template stem, then over that fixture rewritten to spell the removal with a trailing slash, to take one file instead of the directory, and to drop the removal entirely. Both cases pass under `hack/cozytest.sh hack/package-update-templates.bats`. Two limits, both written into the file's header. The check fires on the consequence rather than the shape, so `rm -rf templates` in a package that does refetch everything stays green, which is what keeps `kubevirt-cdi-operator` green for as long as its recipe wipes. And it only looks at a recipe clearing its own directory: `cert-manager` clears `../cert-manager-crds/templates/*` across the package boundary, which needs the removal's target resolved rather than the wiping package's own, so that directory is unguarded. The other packages that clear their own `templates/` refetch all of it and are untouched here. Two more limits the header names. The scan reads the text of each package's own `update:` recipe, so a removal made elsewhere is out of range: in a prerequisite target, behind an include, or behind a recursive make. `capi-operator`, `clustersecret-operator`, `openbao` and `reloader` write `update: clean <name>-update` and take both recipes from `hack/package.mk`, which removes nothing under `templates/` today. And the glob stops at `packages/*/*/Makefile`. The package Makefiles off that depth are the group aggregators such as `packages/system/Makefile` and the vendored kamaji subcharts, and none of them declares `update`. One behaviour of the matcher worth naming, and deliberately not changed here. `wipe_command` evaluates `/^update:/` ahead of its terminator, so a second `update:` in the same Makefile re-arms the scan instead of ending it, and what that means depends on what sits between the two: adjacent or separated by a blank line, both recipes are read; separated by another target, only the first is. The direction that matters is a false positive rather than a miss. A Makefile whose first `update:` clears `templates/` and whose second does not would be reported by this scan, while make runs only the second, so the package fails over a recipe that never executes. No package in the tree defines `update:` twice today. Three more notes, all non-blocking and none acted on. The header says what the scan leaves uncovered is a recipe clearing someone else's `templates/`; that is the uncovered set for *deletion*, and `kubevirt-cdi` is green under the scan while still losing `cdi-cr.yaml`'s content to the in-place `wget -O` on every run, which the Makefile comment records but the header does not. The identity strip has a false-positive direction the header does not argue: a wiping package writing `templates/$(NAME).yaml` would be reported even though the recipe writes it, which no package does today. And the fixture stages `.gitkeep` with `git add --all`, so a developer whose global excludes list that name would get a red for environment rather than defect; `--force` would remove the dependency. The stem match is generous, and the helper's comment now names the two live shapes. A sibling name: `templates/crds.yaml` next to the existing `templates/crds-experimental.yaml` on `gateway-api-crds`. A mention from an unrelated context: `templates/sidecar.yaml` on `objectstorage-controller`, whose Makefile says `sidecar` across its image targets while its recipe clears `templates/` and writes only `templates/controller.yaml`. I fed both to the helper and both come back exempted; a stem the Makefile never mentions is still reported. The duplicate `test:` target in this Makefile is #4157 and stays there. It sits outside the hunks this diff touches, so removing it here would be reaching past what this change is about. One more note, not a defect. Where the comment points at `kubevirt-operator` as the shape that closes the transient-file class, `packages/system/kubevirt/Makefile` is the closer analogue for the `cdi-cr.yaml` half specifically: its `update` downloads nothing and re-applies the parameterization onto a hand-maintained CR with a per-marker self-check. And the failure message offers a third option, staging into a tmpdir the way `etcd-operator-crds` does, in a parenthetical that reads as a gloss on the option before it. What this PR does not fix, and what does. `make update` here still overwrites `templates/cdi-cr.yaml`, which the Makefile comment says plainly. A wholesale overwrite is loud: upstream's object carries neither `podResourceRequirements` nor `cloneStrategyOverride`, and `tests/importer-resources_test.yaml` and `tests/clone_strategy_test.yaml` pin both, so `helm unittest` goes red. The silent case is a partial hand reconciliation that keeps those two and drops something else, because nothing here pins the `ExpandDisks` gate, the `CriticalAddonsOnly` toleration, the `uploadProxyURL` override or the two RBAC documents that preauthorize tenant cloning from `cozy-public`. #4107, stacked on this branch, stops `update` writing the file at all and adds `tests/local_templates_test.yaml`, which pins those. One limit the header does not name, left alone here. The comment strip is anchored to the start of a line, so a trailing comment on a recipe line survives into the searched text and counts as naming the file it mentions. I confirmed it by feeding `wget ... -O templates/cdi-cr.yaml # local-extra.yaml is written by hand` through the filter. Nothing in the tree is written that way, and the header only claims comment lines are dropped, which is what it does. `sed 's/#.*//'` in place of the `grep -v` closes it if it ever matters. Closes #4028. ### Screenshots No UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. Two entries come close: the website's "developer tooling (the package `Makefile`s)" line, and ccp's "move or rename anything under `hack/`". Neither fires. `make update` still behaves as the website's development page describes, nothing under `hack/` moved or was renamed, and the two files ccp's skills key on (`hack/package.mk`, `hack/common-envs.mk`) are untouched. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(cdi): `make update` in kubevirt-cdi no longer deletes its first-party templates ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Updated Kubernetes CDI package updates to preserve existing template files while refreshing downloaded manifests. - Prevented update operations from clearing first-party and vendored template content. - **Tests** - Added safeguards to detect package update recipes that could remove tracked templates without restoring them. - Added Helm unit test support for the Kubernetes CDI package. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
2 files changed
+453
-2
No files matched your search
@@ -0,0 +1,443 @@
|
||||
#!/usr/bin/env bats
|
||||
# -----------------------------------------------------------------------------
|
||||
# Invariant for the package `update` recipes that clear templates/ wholesale.
|
||||
#
|
||||
# `update` refetches a package's vendored upstream manifests, and several
|
||||
# recipes open by deleting templates/ to guarantee a clean slate. That is safe
|
||||
# only while every file under templates/ comes back from the recipe. templates/
|
||||
# is an ordinary chart directory, so a package can also keep first-party
|
||||
# manifests there -- kubevirt-cdi keeps the CDI upload-proxy Ingress and
|
||||
# TLSRoute -- and the recipe knows nothing about those. The wipe takes them and
|
||||
# `make update` exits 0, so the deletion lands in the diff with nothing marking
|
||||
# it. Whether anything downstream notices depends on the package carrying a
|
||||
# helm-unittest suite that names the file, and where none does the loss is
|
||||
# silent all the way to a cluster. That is a property of the package, not of
|
||||
# the tree, so it is worth re-deriving rather than reading: at the time of
|
||||
# writing kubevirt-cdi has such a suite and a deletion there fails `make test`
|
||||
# as well, while some packages that clear their own templates/ declare no
|
||||
# `test` target at all (#3631 tracks packages without one).
|
||||
#
|
||||
# The invariant: a recipe that clears templates/ must NAME every file tracked
|
||||
# under it. Both halves are approximations of the shell the recipe would run,
|
||||
# and each is argued at its helper below: the wipe is a text match over the
|
||||
# recipe's lines rather than a parse, and "names" means the file's basename
|
||||
# minus its extension appears somewhere in the Makefile -- a mention, not proof
|
||||
# of a write. What that catches is the shape kubevirt-cdi had, a file the
|
||||
# Makefile does not refer to at all: every recipe that clears its own
|
||||
# templates/ writes each path it puts back there, so a name appearing nowhere
|
||||
# in the file is a name nothing refetches. A recipe that unpacked an archive or
|
||||
# globbed a stream into templates/ would produce files it never names and break
|
||||
# that reading. cert-manager's `mv charts/.../crd-*.yaml` is that shape, and it
|
||||
# is out of range for the separate reason below: the directory it fills belongs
|
||||
# to another package.
|
||||
#
|
||||
# The tree is scanned rather than a list of packages pinned, so a package that
|
||||
# grows its first first-party manifest under a wiping recipe of its own is
|
||||
# caught by the commit that adds it, within the reach the next two paragraphs
|
||||
# mark out. The packages that clear their own templates/ today refetch all of
|
||||
# it, which is what keeps this green.
|
||||
#
|
||||
# What that leaves uncovered is a recipe clearing SOMEONE ELSE'S templates/, and
|
||||
# the tree has one: packages/system/cert-manager/Makefile runs
|
||||
# `rm -rf ../cert-manager-crds/templates/*` and moves the CRDs it pulled into
|
||||
# that directory, while cert-manager-crds declares no update target at all. A
|
||||
# first-party manifest added there is deleted by the next `make update` in
|
||||
# cert-manager and nothing below reports it. Covering that means resolving the
|
||||
# removal's target directory rather than reading the wiping package's own, which
|
||||
# is a second helper; the pattern below is deliberately not widened to accept a
|
||||
# path prefix, because that alone would fire on cert-manager and then list the
|
||||
# templates of a directory it does not have -- the removal names the sibling's
|
||||
# path, so the lookup has to follow it.
|
||||
#
|
||||
# Two more places are out of reach because the scan reads only the text of a
|
||||
# package's own `update:` recipe. One is a removal made elsewhere: in a
|
||||
# prerequisite target, behind an include, or behind a recursive make.
|
||||
# capi-operator, clustersecret-operator, openbao and reloader all write
|
||||
# `update: clean <name>-update` and take both of those recipes from
|
||||
# hack/package.mk, which removes nothing under templates/ today; a removal
|
||||
# added there would go unreported for all four. The other is a Makefile the
|
||||
# glob does not reach: it stops at packages/*/*/, and the two vendored kamaji
|
||||
# subcharts below that level declare no `update` target today.
|
||||
#
|
||||
# The scan alone would also be green against a detector that finds nothing, so
|
||||
# the control below runs the same two helpers over a fixture that does hold an
|
||||
# unrefetched file, and over that fixture with the removal taken out.
|
||||
#
|
||||
# Constraints from cozytest.sh: it rewrites the closing brace of every function
|
||||
# in this file into `return 0`, so a helper cannot report through its exit
|
||||
# status -- both report through stdout -- and it injects `set -e`, so statuses
|
||||
# are captured explicitly.
|
||||
#
|
||||
# Run with: hack/cozytest.sh hack/package-update-templates.bats
|
||||
# -----------------------------------------------------------------------------
|
||||
|
||||
# Prints the recipe line that clears templates/ wholesale, for a package
|
||||
# Makefile whose `update` target has one. Silent otherwise, including for a
|
||||
# Makefile with no `update` target at all.
|
||||
#
|
||||
# The recipe is read the way make reads one: the run of tab-indented lines after
|
||||
# `update:`, with blank lines and comment lines passed over rather than treated
|
||||
# as its end, because make ignores them and carries on. A tab-indented comment
|
||||
# is skipped rather than searched, which matters more here than anywhere else in
|
||||
# the file: a recipe that deliberately does not clear templates/ is likely to
|
||||
# carry a line saying why, and without the skip
|
||||
# `# deliberately no rm -rf templates here` reads as a wipe and fails that very
|
||||
# package. Both helpers drop comments before searching, for mirror-image
|
||||
# reasons. The terminator class accepts a following space or `;` and nothing
|
||||
# else, because `templates &&` already matches through the space and
|
||||
# `templates&` is a shape nobody writes.
|
||||
#
|
||||
# The match accepts `rm -rf`, `rm -fr` and `rm -r` against `templates` and
|
||||
# `./templates`, bare, with a trailing slash, or with a `/*` glob -- the three
|
||||
# spellings that empty the directory. It deliberately does not accept
|
||||
# `rm -rf templates/one-file.yml`, which takes a single file the recipe goes on
|
||||
# to refetch (rabbitmq-operator), nor a narrower glob such as
|
||||
# `rm -rf templates/*.yaml`, which takes a subset the same way.
|
||||
#
|
||||
# It is a text match and not a shell parse, so a quoted `rm -rf "templates"`, a
|
||||
# removal reached through a variable, a `find -delete` and a `cd` into another
|
||||
# directory first are all invisible to it. So is `rm -rf templates&& mkdir`,
|
||||
# where the removal runs straight into `&&` with no space: legal shell, and
|
||||
# outside the terminator class. So is a path-prefixed target -- the
|
||||
# cert-manager case in the header is one, and the prefix is the only thing
|
||||
# keeping it out, since the `/*` it ends with is accepted. The enumeration is
|
||||
# only as complete as the last reader made it, so extend it in the same edit as
|
||||
# the pattern.
|
||||
#
|
||||
# A second `update:` can fool this in either direction: make runs the last
|
||||
# recipe, this reads whichever ones are adjacent. Nothing in the tree writes
|
||||
# one.
|
||||
wipe_command() {
|
||||
awk '/^update:/ { r = 1; next }
|
||||
r && /^\t[[:space:]]*@?#/ { next }
|
||||
r && /^\t/ { print; next }
|
||||
r && /^[[:space:]]*$/ { next }
|
||||
r && /^[[:space:]]*#/ { next }
|
||||
r { exit }' "$1" |
|
||||
grep -E 'rm[[:space:]]+-(rf|fr|r)[[:space:]]+(\./)?templates(/\*?)?([[:space:]]|;|$)' || true
|
||||
}
|
||||
|
||||
# Prints every file tracked under a package's templates/ that its Makefile
|
||||
# never mentions, one path per line. Whether that matters is the caller's call:
|
||||
# it matters for a package whose recipe clears the directory, and for no other.
|
||||
#
|
||||
# The listing is the index, so only a tracked file is at stake -- it is the one
|
||||
# the next `make update` deletes out from under a reviewer who has no reason to
|
||||
# look. Reading the working tree instead would fail a package over the
|
||||
# developer's own litter: an untracked .orig from a hand-applied patch, a
|
||||
# half-written template, a .DS_Store. A guard that goes red on scratch files is
|
||||
# a guard that gets switched off.
|
||||
#
|
||||
# The extension is stripped before matching, because a recipe may assemble the
|
||||
# filename from a variable holding the bare stem -- etcd-operator-crds writes
|
||||
# templates/$$c.yaml from a CRDS list of names without one. A basename whose
|
||||
# only dot is its first character keeps the whole name instead: stripping there
|
||||
# leaves the empty string, which grep matches against every Makefile, and the
|
||||
# file would pass without being looked at. The search covers the whole Makefile
|
||||
# rather than the recipe alone for the same reason as the stem: the variable is
|
||||
# assigned above the target. Both make the match generous, so a stem that
|
||||
# appears for any other reason satisfies it. Two live examples in the tree
|
||||
# today. A sibling's name: gateway-api-crds clears templates/ and writes
|
||||
# templates/crds-experimental.yaml, so a templates/crds.yaml added beside it
|
||||
# passes because `crds` is inside that name. And a word used elsewhere in the
|
||||
# file: objectstorage-controller clears templates/ and writes only
|
||||
# templates/controller.yaml, yet a templates/sidecar.yaml would pass, since its
|
||||
# image targets say `sidecar` several times. This reports the files nothing in
|
||||
# the Makefile refers to, not every file the recipe fails to write.
|
||||
#
|
||||
# Comment lines are dropped before the search, so a filename written into one is
|
||||
# not a mention: a rationale comment above a recipe, naming the very file it is
|
||||
# about, would otherwise exempt that package from its own guard. Comments
|
||||
# naming a template are not rare in the tree, so this is the difference between
|
||||
# a package being read and a package being taken at its word.
|
||||
#
|
||||
# The NAME and NAMESPACE assignments go the same way. They are the package's
|
||||
# identity rather than a reference to any file, and a package called foo-bar
|
||||
# whose recipe writes templates/bar.yaml has that file's stem inside its own
|
||||
# name: left in, they let a package vouch for its own templates without any
|
||||
# recipe touching them. The match is on the variable name and not on the
|
||||
# assignment operator, because the hole does not care how it was spelled: the
|
||||
# `export` prefix is optional and so is whitespace either side. The cases below
|
||||
# pin the export prefix and the `:=` spelling, which with the bare `NAME=` are
|
||||
# every form the tree writes; the pattern is looser than that on purpose, and
|
||||
# the looseness beyond those forms is not something a case here checks.
|
||||
#
|
||||
# Two names are named rather than a `[A-Z_]+=` shape for the opposite reason:
|
||||
# a variable holding content is what the whole-file search is there to find,
|
||||
# and etcd-operator-crds names its templates through nothing but its CRDS
|
||||
# list, so a shape that broad would be one unspaced assignment away from
|
||||
# blinding the scan to five CRDs. That list is written spaced, which is why
|
||||
# the broader pattern would not reach it today and why no case below can pin
|
||||
# this choice; two names cannot reach it at any spacing. No package the scan
|
||||
# reaches is exempted this way today; the exclusion is for the first one that
|
||||
# would be.
|
||||
unnamed_templates() {
|
||||
_mk=$1
|
||||
_dir=$(dirname "$_mk")
|
||||
_code=$(grep -vE '^([[:space:]]*@?#|(export[[:space:]]+)?(NAME|NAMESPACE)[[:space:]]*[:?+]*=)' "$_mk") || true
|
||||
git -C "$_dir" ls-files -- templates | while IFS= read -r _f; do
|
||||
_base=${_f##*/}
|
||||
_stem=${_base%.*}
|
||||
[ -n "$_stem" ] || _stem=$_base
|
||||
printf '%s\n' "$_code" | grep -Fq -- "$_stem" || printf '%s\n' "$_dir/$_f"
|
||||
done
|
||||
}
|
||||
|
||||
@test "the scan reports a template file that a wiping recipe does not name" {
|
||||
# Negative control, and it runs first on purpose: cozytest.sh exits at the
|
||||
# first failing @test, so behind the tree scan a red tree would take this
|
||||
# with it -- and "a package regressed" and "the helpers stopped measuring"
|
||||
# would look identical at exactly the moment the difference matters. The
|
||||
# scan below is green both on a tree with no offender and on a detector
|
||||
# that cannot see one; this separates them by handing the same helpers a
|
||||
# package broken exactly the way kubevirt-cdi was.
|
||||
tmp=$(mktemp -d)
|
||||
mkdir -p "$tmp/pkg/templates"
|
||||
: > "$tmp/pkg/templates/fetched.yaml"
|
||||
: > "$tmp/pkg/templates/local-extra.yaml"
|
||||
# A tracked dotfile. Stripping its extension leaves nothing and an empty
|
||||
# pattern matches every Makefile, so without the guard in the helper this
|
||||
# file is skipped in silence rather than reported.
|
||||
: > "$tmp/pkg/templates/.gitkeep"
|
||||
# Two files whose stems sit inside the package's own identity, the shape
|
||||
# every `foo-bar` package writing templates/bar.yaml has. One is covered by
|
||||
# NAME and one by NAMESPACE, and the Makefile below writes the first as
|
||||
# `export NAME=` and the second as `NAMESPACE := `, so between them the
|
||||
# export prefix, the bare form, spacing around the operator and the `:=`
|
||||
# spelling are all exercised. Left in the search, each of these matches
|
||||
# with nothing writing it.
|
||||
: > "$tmp/pkg/templates/name-echo.yaml"
|
||||
: > "$tmp/pkg/templates/bare-echo.yaml"
|
||||
# The comment names the file the recipe does not write, which is what a
|
||||
# rationale comment above a real recipe looks like. It must not count as the
|
||||
# recipe naming it.
|
||||
printf 'export NAME=demo-name-echo\nNAMESPACE := cozy-bare-echo\n# local-extra.yaml is written by hand\nupdate:\n\trm -rf templates\n\tmkdir templates\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
# An index is what the helper reads, so the fixture needs one. Staged rather
|
||||
# than committed: nothing here needs a commit object, and a commit would want
|
||||
# an identity and a signature from whoever runs the suite.
|
||||
git -C "$tmp" init --quiet
|
||||
git -C "$tmp" add --all
|
||||
|
||||
# Untracked litter, written after the staging so it stays untracked. Neither
|
||||
# file is at stake and neither may be reported.
|
||||
: > "$tmp/pkg/templates/scratch.yaml"
|
||||
: > "$tmp/pkg/templates/.DS_Store"
|
||||
|
||||
if [ -z "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher did not see 'rm -rf templates' in the fixture recipe" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Exactly the tracked files the recipe does not name. Reporting the fetched
|
||||
# one too would fail every wiping package in the tree, reporting none of them
|
||||
# would pass all of them, and reporting the litter would fail a package over
|
||||
# a file nobody is going to miss.
|
||||
expected=$(printf '%s\n%s\n%s\n%s' "$tmp/pkg/templates/.gitkeep" "$tmp/pkg/templates/bare-echo.yaml" "$tmp/pkg/templates/local-extra.yaml" "$tmp/pkg/templates/name-echo.yaml")
|
||||
found=$(unnamed_templates "$tmp/pkg/Makefile")
|
||||
if [ "$found" != "$expected" ]; then
|
||||
echo "expected only the tracked unrefetched files to be reported:" >&2
|
||||
printf '%s\n' "$expected" >&2
|
||||
echo "got:" >&2
|
||||
printf '%s\n' "$found" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# The trailing-slash spelling removes exactly as much, so it has to be seen
|
||||
# as well. Spelled with the leading ./ at the same time, which is the other
|
||||
# optional half of the pattern; the fixture above carries neither.
|
||||
printf 'update:\n\trm -rf ./templates/\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -z "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher missed 'rm -rf ./templates/', which clears the directory" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# And the glob spelling, which empties the directory without naming it as
|
||||
# the argument. It is what cert-manager's recipe writes, so it is the line a
|
||||
# package copies when it wants to clear its own templates.
|
||||
printf 'update:\n\trm -rf templates/*\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -z "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher missed 'rm -rf templates/*', which empties the directory" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# A tab-indented comment inside the recipe is not shell. A recipe that
|
||||
# deliberately does not clear templates/ tends to carry a line saying why,
|
||||
# and searching that line would fail that very package. Both
|
||||
# spellings are pinned because make accepts both and the tree writes both:
|
||||
# `@#` in kubevirt-operator's own update recipe, bare `#` everywhere else.
|
||||
for lead in '#' '@#'; do
|
||||
printf 'update:\n\tmkdir -p templates\n\t%s deliberately no rm -rf templates here: the upload-proxy files are ours\n' \
|
||||
"$lead" > "$tmp/pkg/Makefile"
|
||||
if [ -n "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher read a '$lead' comment inside the recipe as a removal" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
done
|
||||
|
||||
# A comment at column zero is ignored by make the same way a blank line is,
|
||||
# and the recipe continues past it: a recipe with one runs the commands
|
||||
# that follow it.
|
||||
printf 'update:\n\tmkdir -p templates\n# a comment at column zero\n\trm -rf templates\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -z "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher stopped at a column-zero comment and missed the removal after it" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# make ignores a blank line inside a recipe and carries on, so the scan has
|
||||
# to as well or everything after the blank is invisible to it.
|
||||
printf 'update:\n\tmkdir -p templates\n\n\trm -rf templates\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -z "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher stopped at a blank line and missed the removal after it" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# The `;` terminator is load-bearing, not decoration: etcd-operator-crds
|
||||
# writes `rm -rf templates; mkdir templates; \` and is reached through this
|
||||
# branch alone, so dropping it silently takes that package out of the scan.
|
||||
printf 'update:\n\trm -rf templates; mkdir templates\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -z "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher missed 'rm -rf templates;', the shape etcd-operator-crds uses" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# The two flag spellings the header claims to accept. Nothing in the tree
|
||||
# writes either today, so without these cases that part of the header
|
||||
# would go unchecked.
|
||||
for flags in -fr -r; do
|
||||
printf 'update:\n\trm %s templates\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
"$flags" > "$tmp/pkg/Makefile"
|
||||
if [ -z "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher missed 'rm $flags templates', which the header says it accepts" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
done
|
||||
|
||||
# Scoping. The helper claims to read only the `update` recipe, and to stay
|
||||
# silent for a Makefile that has no `update` target at all. The tree cannot
|
||||
# test either, because every wipe in it already sits inside `update`, so
|
||||
# without these two a matcher that searched the whole file would look
|
||||
# identical. The consequence of losing this is a false positive that never
|
||||
# clears: a package growing `clean: rm -rf templates` reported forever.
|
||||
printf 'clean:\n\trm -rf templates\nupdate:\n\tmkdir -p templates\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -n "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher read a removal from a target other than update" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# The other order, and the one this tree actually writes: kubevirt-cdi puts
|
||||
# `test:` below its `update:` recipe. Above, `r` is still 0 and the gate
|
||||
# does the work; here the recipe has already started and only the
|
||||
# terminator can stop it, so without this case that half is unpinned.
|
||||
printf 'update:\n\tmkdir -p templates\nclean:\n\trm -rf templates\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -n "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher ran past the end of the update recipe into a later target" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
printf 'clean:\n\trm -rf templates\n' > "$tmp/pkg/Makefile"
|
||||
if [ -n "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher reported on a Makefile with no update target" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# The other side of that boundary: removing one file inside templates/ is
|
||||
# rabbitmq-operator's shape, it takes only what the recipe refetches, and a
|
||||
# pattern loose enough to call it a wipe would report that package forever.
|
||||
printf 'update:\n\trm -rf templates/fetched.yaml\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -n "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher read a single-file removal as clearing the directory" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# The narrower glob is the other rejection the header claims. It takes a
|
||||
# subset the recipe refetches, so calling it a wipe would report the package forever -- and it
|
||||
# discriminates: widening the tail to accept any glob, the obvious "also
|
||||
# match globs" edit, slips past the single-file case above but not this one.
|
||||
printf 'update:\n\trm -rf templates/*.yaml\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -n "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher read a narrower glob as clearing the directory" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# And the shape kubevirt-cdi moved to switches the check off, so the scan
|
||||
# is measuring the removal rather than the package.
|
||||
printf 'update:\n\tmkdir -p templates\n\twget -O templates/fetched.yaml https://example.invalid/f.yaml\n' \
|
||||
> "$tmp/pkg/Makefile"
|
||||
if [ -n "$(wipe_command "$tmp/pkg/Makefile")" ]; then
|
||||
echo "the matcher fired on a recipe that only creates the directory" >&2
|
||||
rm -rf "$tmp"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
rm -rf "$tmp"
|
||||
}
|
||||
|
||||
@test "no package update recipe clears templates/ over a file it never refetches" {
|
||||
examined=0
|
||||
offenders=""
|
||||
|
||||
for mk in packages/*/*/Makefile; do
|
||||
[ -f "$mk" ] || continue
|
||||
examined=$((examined + 1))
|
||||
[ -n "$(wipe_command "$mk")" ] || continue
|
||||
found=$(unnamed_templates "$mk")
|
||||
if [ -n "$found" ]; then
|
||||
offenders="$offenders
|
||||
$mk clears templates/ but names none of:
|
||||
$found"
|
||||
fi
|
||||
done
|
||||
|
||||
# A glob that matched nothing -- packages/ moved, or the suite run from
|
||||
# somewhere other than the repository root -- would otherwise report success
|
||||
# having looked at no Makefile at all.
|
||||
if [ "$examined" -eq 0 ]; then
|
||||
echo "no packages/*/*/Makefile found: run this from the repository root" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# The index is where the scan reads a package's templates from, so an answer
|
||||
# of nothing here means it could not have reported anything either.
|
||||
if [ -z "$(git ls-files -- packages | head -1)" ]; then
|
||||
echo "git ls-files reported no tracked file under packages/: this test" >&2
|
||||
echo "cannot see a template directory, so it measured nothing" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if [ -n "$offenders" ]; then
|
||||
echo "a package update recipe deletes templates/ over a file it does not refetch:" >&2
|
||||
echo "$offenders" >&2
|
||||
echo "" >&2
|
||||
echo "templates/ holds first-party manifests alongside the vendored ones and" >&2
|
||||
echo "'rm -rf templates' takes both, silently: the next 'make update' drops the" >&2
|
||||
echo "first-party ones and still exits 0. Either keep 'mkdir -p templates'" >&2
|
||||
echo "without the removal -- wget -O and kustomize > overwrite what they fetch" >&2
|
||||
echo "anyway -- or write the local file from the recipe too, where the clean" >&2
|
||||
echo "slate is load-bearing (etcd-operator-crds stages into a tmpdir first)." >&2
|
||||
exit 1
|
||||
fi
|
||||
}
|
||||
@@ -9,9 +9,17 @@ include ../../../hack/package.mk
|
||||
test:
|
||||
helm unittest .
|
||||
|
||||
# Nothing under templates/ here is purely vendored. The upload-proxy Ingress
|
||||
# and TLSRoute are ours and no fetch recreates them, so clearing the directory
|
||||
# would lose them outright. It is not needed either: wget -O truncates the one
|
||||
# file it writes and leaves the rest.
|
||||
#
|
||||
# cdi-cr.yaml is ours too, despite being named after an upstream asset: it
|
||||
# carries Helm templating and RBAC that no fetch reproduces. The fetch below
|
||||
# still overwrites it, so a CDI bump means diffing it against the upstream
|
||||
# asset and re-applying whatever is local by hand.
|
||||
update:
|
||||
rm -rf templates
|
||||
mkdir templates
|
||||
mkdir -p templates
|
||||
export VERSION=$$(basename $$(curl -s -w %{redirect_url} https://github.com/kubevirt/containerized-data-importer/releases/latest)) && \
|
||||
wget https://github.com/kubevirt/containerized-data-importer/releases/download/$$VERSION/cdi-cr.yaml -O templates/cdi-cr.yaml
|
||||
|
||||
|
||||
Reference in new issue
Block a user