build(images): run the tests of the packages the patches touch - #4326

[MINOR] the gate reports success when its own input command fails

git apply --numstat feeds the loop through a command substitution, so its exit status is dropped. When it errors the list comes back empty, the loop body never runs, and the step passes having tested nothing. The git apply a few lines up fails closed on the same argument, so the two disagree about the same input; in this image and in objectstorage they do not even read the same copy of it, since the gate reads a second COPY --from=source /patches /patches in another stage.

Reproduced with the argument pointed at a path that does not resolve:

$ sh -c 'for dir in $(git apply --numstat /nonexistent/*.diff | cut -f3 | grep "\.go$" | xargs -n1 dirname | sort -u); do
  echo "RAN FOR: $dir"
done
echo "LOOP FINISHED, RUN EXIT WOULD BE 0"'
error: can't open patch '/nonexistent/*.diff': No such file or directory
LOOP FINISHED, RUN EXIT WOULD BE 0
shell exit=0

Capturing the list on its own line under set -e closes it, and I checked both halves behave:

RUN set -e; \
    files=$(git apply --numstat /patches/*.diff); \
    for dir in $(printf '%s\n' "$files" | cut -f3 | grep '\.go$' | xargs -n1 -r dirname | sort -u); do \
      (cd "$dir" && go test .); \
    done

set -e does abort on the assignment (exit=128, the loop is never reached), and an empty .go list still iterates zero times without tripping it. The -r on xargs is worth adding for its own reason: without it GNU xargs runs dirname once with no operands on an empty list, which is where the stray dirname: missing operand in the build log comes from.

The same line appears in the other 11 images.

Fixed in a08ccca, same change as in the kamaji thread. It also removes the stray dirname: missing operand. I used || exit 1 on the assignment instead of set -e: it fails in the same cases and matches the || exit 1 already in the loop body.

[MINOR] the integration arm is hardcoded while the derived loop already visits that directory vacuously

valkey-cli-auth.diff touches test/integration/redisfailover/database_engine_test.go, so the derived list already contains that directory. At v3.3.5 it holds creation_test.go and database_engine_test.go, both behind //go:build integration in package redisfailover_test, plus an untagged doc.go in package redisfailover. So the loop's go test . there compiles only doc.go, reports no test files and asserts nothing; this hardcoded go vet line is the real check.

Two consequences. Unlike everything else in this change the path is not derived from the patches, so a later patch touching build-tagged tests in any other directory is silently uncovered. And the directory's coverage rests on doc.go continuing to exist: if upstream drops it, the loop's own visit fails outright rather than passing empty.

$ mkdir -p /tmp/tagtest/pkg && cd /tmp/tagtest
$ printf 'module tagtest\n\ngo 1.21\n' > go.mod
$ printf '//go:build integration\n\npackage pkg\n\nimport "testing"\n\nfunc TestX(t *testing.T) {}\n' > pkg/only_tagged_test.go
$ cd pkg && go test .
# tagtest/pkg
package tagtest/pkg: build constraints exclude all Go files in /tmp/tagtest/pkg
FAIL	tagtest/pkg [setup failed]
FAIL
exit=1

Deriving the tagged arm from the patches the same way the untagged one is derived would close both.

Fixed in 357f57a, details in the newer thread on this line. The tagged arm is now derived from the patches like the untagged one.

The doc.go dependency is still there. If upstream drops it, go test . in that directory fails the build with "build constraints exclude all Go files". That failure is loud, so I left it as is.

[MINOR] the first real signal from the new runner is answered by deleting the assertion

This hunk drops the errors because initial+delta > maxSize case from TestNodeGroupIncreaseSizeErrors, because the carried patch's other hunk removes the case nreplicas > r.maxSize refusal from SetSize and the test caught the mismatch. The image's behaviour does not change, and the commit message says so plainly, so this is not a regression and it is not why the note is here.

It is here because of what the assertion was holding. At cluster-autoscaler-1.32.0, IncreaseSize in clusterapi_nodegroup.go checks only that the delta is positive and then delegates:

func (ng *nodegroup) IncreaseSize(delta int) error {
	if delta <= 0 {
		return fmt.Errorf("size increase must be positive")
	}
	size, err := ng.scalableResource.Replicas()
	if err != nil {
		return err
	}
	return ng.scalableResource.SetSize(size + delta)
}

So the SetSize guard was the only enforcement of maxSize on this path, and that deleted case was its only assertion across the package's test files. The patch is named fix-downscale, yet the hunk it carries removes the size-increase refusal and leaves the size-decrease one intact, which reads more like the wrong case was deleted in January 2025 than like a decision. That predates this PR by twenty months and is not this PR's to fix.

What is this PR's: the change whose whole purpose is to stop carried patches from drifting away from what upstream asserts resolves its own first finding by removing the assertion, inside a build(...) commit, without a line on whether the guard removal was intended. Suggest keeping the deletion so the build goes green, and opening a separate issue against fix-downscale.diff so the question does not disappear with the test that raised it.

I don't think the wrong case was deleted. DeleteNodes lowers a group through SetSize(replicas - 1), so with the old nreplicas > r.maxSize check a group already above its max size could not shed a node at all. Every step was refused as an increase, and that is the downscale the patch is named for.

On the increase side, the core scale-up orchestrator skips a group that is already at MaxSize() before it calls IncreaseSize (the currentTargetSize >= nodeGroup.MaxSize() check in core/scaleup/orchestrator/orchestrator.go). So the provider check was a second line of defence, not the only one. 70667a8 adds a test for the downscale path, so the patched behaviour now has an assertion, and I don't think a separate issue is needed.

[MAJOR] generic go test . loop drags the envtest controller suite into the build and fails it

The loop runs go test . in every directory the patch touches. The opensearch patch edits controllers/opensearchController.go, so the loop enters opensearch-operator/controllers and compiles its pre-existing suite_test.go, an envtest BeforeSuite that execs /usr/local/kubebuilder/bin/etcd. That binary is absent in the builder, TestAPIs fails, and the RUN exits 1. All 11 other image builds on this head pass; only this one is red, for exactly this step. The curated line just below (go test -race ./opensearch-gateway/... ./pkg/reconcilers/...) already runs the patch's real unit tests, and its comment says both trees run without a cluster. The generic loop is redundant here and additionally pulls in the one tree that needs a cluster. Drop the loop for this image, or exclude cluster-bound suites from it.

$ gh api repos/cozystack/cozystack/actions/jobs/109706697882/logs
unable to start control plane itself: failed to start the controlplane. retried 5 times: fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory
--- FAIL: TestAPIs (0.01s)
FAIL	github.com/Opster/opensearch-k8s-operator/opensearch-operator/controllers	0.023s
#14 ERROR: process for the added test loop did not complete successfully: exit code: 1

Fixed in 0979736. After the stall-tracing patch landed on main, the loop started entering controllers/, and that suite starts an envtest control plane the builder has no binaries for. The step now skips that one directory.

I also moved the -race flag into the loop and dropped the hand-picked go test -race line, so the same packages are not tested twice and a patch in a new directory is still picked up. Locally the builder stage builds and reports ok for opensearch-gateway/services and pkg/reconcilers.

[MINOR] for dir in $(git apply --numstat ...) swallows the substitution's exit status (same pattern in every image)

If the patch glob is missing or git apply --numstat errors, the substitution yields an empty list, the loop body never runs, and the RUN still exits 0. A runner that silently runs zero tests reads as green, which defeats the point of the change. Capture the list first and fail when the apply itself errored or the list is empty when a test was expected.

Fixed in all 12 images, for kamaji in 509ce5d. The list goes into a variable with || exit 1 before the loop, and xargs -r keeps dirname from running with no operand.

Checked under dash in the golang builder image: a patch path that does not resolve exits 1, a patch with no Go file exits 0 and runs nothing, and a normal patch runs the test. I didn't add a "list must not be empty" check. Once git has read the patches, the list depends only on what they contain, so an empty list just means no Go file was touched.

[MINOR] the integration arm is hardcoded while the loop already visits that directory vacuously

The derived loop runs go test . in test/integration/redisfailover, where the tests sit behind a build tag, so it compiles and asserts nothing there; the separate go vet -tags integration ./test/integration/redisfailover then covers the same directory by hand. Keep one path, not both.

Fixed in 357f57a. The loop now runs go vet -tags integration . next to go test . in every directory it visits, and the hand-written go vet line is gone. All 11 touched directories vet clean with the tag.

To check it, I added an undefined call to database_engine_test.go. go vet -tags integration fails on it, while go test . still reports no test files. The tag name is still written in the Dockerfile, so a test behind some other tag would need an edit.

[MINOR] the first real failure from the new runner is answered by deleting the assertion

The new hunk removes the errors because initial+delta > maxSize case rather than updating it to the patched scale-up behavior, so the exact path the production patch changes loses its only assertion.

Fixed in 70667a8. The patch now also carries TestNodeGroupDeleteNodesAboveMaxSize, which deletes a node from a group with 4 replicas and max size 2 and expects 3.

Against the unpatched SetSize the test fails with size increase too large - desired:3 max:2. With the patch the touched package passes. I ran both from the cluster-autoscaler-1.32.0 tag tarball, and the CI build of the image passes the step too. The deleted case still has to go, because it asserts the exact refusal the patch removes.