Repository navigation
fix(tests): keep two unit tests green after the release image restamp - #5179
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Marian Koreniuk (@themoriarti) The code is right, but the commit history needs one more pass before merge.
The bats test now reads the kube-ovn release from the place the build itself reads it. The kube-ovn Makefile builds the image from appVersion in the vendored chart, and its image target is what writes IMAGE_TAG@digest into .global.images.kubeovn.tag. So the old test was reading build output. Neither Prepare release commit on release-1.7 (rc.3 and v1.7.0) touches a Chart.yaml, and nothing under hack/ or .github/workflows/ writes appVersion.
I ran both tests on this head and on the release-1.7 tip. The bats file is 10/10 here and 9/10 on the base, where test 4 fails exactly as #5178 describes. The vtraces case is 28/28 here and 27/28 on the base. Unit & controller tests is green on this head, so these two were the only tests the restamp broke.
Both tests still go red when they should. Moving the multus marker to v1.15.25, bumping appVersion to 1.15.27, or deleting appVersion each fail test 4, while restamping the values.yaml tag keeps it green. On the helm side, replacing the cozy-lib.image include with the raw value fails the case, and so does stripping the digest from values.yaml. The old regex on main stays green with the helper removed, so the claim in the body holds.
What blocks is the history. The repo merges with merge commits, so both commit bodies land as written. The first one says the vtraces test "now accepts the first-party image name too". The second drops that regex and opens with "The previous assertion listed the image names...", which describes an earlier state of this PR, not the code. contributing.md treats review-iteration wording in a commit message as a blocker. Please fold the second commit into the first, or make it one commit per test, so each message describes what lands.
Two small things, not blocking:
- The bats comment pins "(v1.7.0)". "to the Cozystack version" says the same and won't go stale.
- The body still says "I can open it if wanted" about
main, but #5180 exists now, and the body becomes the merge commit message.
e05c6af to
661c5a2
Compare
The release build rewrites packages/*/values.yaml: the kube-ovn image tag becomes the Cozystack version and the traces collector becomes the first-party otelcol-tracing build. Two unit tests read exactly those fields, so every PR against release-1.7 has failed Unit & controller tests since the first rc, and release-1.8 would repeat it: - hack/cni-plugins-staging-contract.bats compared the multus marker with the values.yaml tag. It now reads appVersion from the vendored kube-ovn chart, which is what packages/system/kubeovn/Makefile builds from and what a release build leaves alone. - vtraces_collector_test.yaml matched the upstream collector image name. It now sets the cluster images-registry and checks that it lands in front of the rendered image and that the image stays digest-pinned: that is the cozy-lib.image helper's behaviour, and it holds on main and on a release branch alike. Removing the helper from the template makes the case fail; a name regex did not notice. Same change as cozystack#5179 on release-1.7. Assisted-by: LLM Signed-off-by: Marian Koreniuk <[email protected]>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Marian Koreniuk (@themoriarti) NOT LGTM: please keep the release-restamp explanation and remove the implementation walkthrough from the commit body. The earlier request to fold the history is fixed; no source change is needed.
The remaining blocker is the combined commit body. In 661c5a205510, the failure explanation is followed by two bullets describing how the tests work:
The release build rewrites packages/*/values.yaml: the kube-ovn image tag
becomes the Cozystack version and the traces collector becomes the
first-party otelcol-tracing build. Two unit tests read exactly those
fields, so every PR against release-1.7 has failed Unit & controller
tests since the first rc:
- hack/cni-plugins-staging-contract.bats compared the multus marker with
the values.yaml tag. It now reads appVersion from the vendored kube-ovn
chart, which is what packages/system/kubeovn/Makefile builds from and
what a release build leaves alone.
- vtraces_collector_test.yaml matched the upstream collector image name.
It now sets the cluster images-registry and checks that it lands in
front of the rendered image and that the image stays digest-pinned:
that is the cozy-lib.image helper's behaviour, and it holds on main
and on a release branch alike. Removing the helper from the template
makes the case fail; a name regex did not notice.
The commit rules require the body to explain why the change is needed. The Review Blockers section explicitly includes bodies that walk through the diff. Please amend this commit to keep the rationale without the implementation bullets.
I reran the tests on a merge with current release-1.7: BATS 10/10 and collector 28/28; the base fails exactly those two cases. The full monitoring and multus Helm suites pass too. The version-mismatch, helper-bypass and digest controls fail the intended assertions.
The release build rewrites packages/*/values.yaml: the kube-ovn image tag becomes the Cozystack version and the traces collector becomes the first-party otelcol-tracing build. Two unit tests read exactly those fields, so every PR against release-1.7 has failed Unit & controller tests since the first rc. The same patch applies to main as cozystack#5180. Assisted-by: LLM Signed-off-by: Marian Koreniuk <[email protected]>
661c5a2 to
fcc1ada
Compare
The release build rewrites packages/*/values.yaml: the kube-ovn image tag becomes the Cozystack version and the traces collector becomes the first-party otelcol-tracing build. Two unit tests read exactly those fields, so every PR against release-1.7 has failed Unit & controller tests since the first rc, and release-1.8 would repeat it. Same change as cozystack#5179 on release-1.7. Assisted-by: LLM Signed-off-by: Marian Koreniuk <[email protected]>
|
Aleksei Sviridkin (@lexfrei) Fixed in fcc1ada: the commit body keeps the release-restamp explanation and drops the two implementation bullets. It is a message-only amend, the tree is identical to 661c5a2, and CI is green on the new head. #5180 got the same amend (e92bc86). |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Marian Koreniuk (@themoriarti) LGTM. The commit-message request is fixed. The release-1.7 base and the complete tested tree are unchanged, so the earlier test and control results still apply. Current unit and fork E2E checks pass.
ec30722
into
cozystack:release-1.7
…#5180) ## What this PR does Same change as #5179, on `main`. Both tests pass here today, because `main` still carries the pre-release image pins; they fail on every release line as soon as the first rc is cut, since the `Prepare release` commit rewrites `packages/*/values.yaml` and two unit tests read exactly the fields it rewrites. On `release-1.7` this has kept `Unit & controller tests` red on every PR since v1.7.0-rc.3 ([job](https://github.com/cozystack/cozystack/actions/runs/41162009892/job/124421449910) on #5172), and `release-1.8` would repeat it. | test | reads | before the release build | after (release-1.7 today) | |---|---|---|---| | `hack/cni-plugins-staging-contract.bats` "the kube-ovn release the CNI plugin pin was checked against is the one vendored" | `.global.images.kubeovn.tag` in `packages/system/kubeovn/values.yaml` | `v1.15.26@sha256:…` | `v1.7.0@sha256:…` | | `packages/system/monitoring/tests/vtraces_collector_test.yaml` "pins the collector image through the cozy-lib.image helper" | `tracingCollector.image` | `docker.io/otel/opentelemetry-collector-contrib:0.136.0@sha256:…` | `ghcr.io/cozystack/cozystack/otelcol-tracing:v0.136.0-cozystack.0@sha256:…` | Changes: - The bats test takes the vendored kube-ovn release from `appVersion` in `packages/system/kubeovn/charts/kube-ovn/Chart.yaml`. That is the value `packages/system/kubeovn/Makefile` builds the image from, and a release build leaves it alone. The hint printed on a mismatch names the new file. - The helm unittest case no longer names the image at all. It sets `_cluster.images-registry` and asserts that the registry lands in front of the rendered image and that the image stays digest-pinned (the `grafana_upstream_image_plugins_test.yaml` pattern). Removing the `cozy-lib.image` call from the template makes it fail; the old regex did not. Same single commit as #5179; the only difference is the unrelated `DOC=` line next to the edit, kept as `main` has it. Verified locally on this branch: `bats hack/cni-plugins-staging-contract.bats` 10/10, `helm unittest packages/system/monitoring -f tests/vtraces_collector_test.yaml` 28/28. Refs #5178 (the release-1.7 report), #5179, #5172. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Tests** * Version validation now compares the checked version with the chart’s application version and reports missing or mismatched versions with the chart path. * Monitoring image checks now verify use of the configured mirror registry and require a 64-character lowercase SHA-256 digest, without requiring a specific image name. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Every PR against
release-1.7has had a redUnit & controller testscheck since v1.7.0-rc.3 (seen on #5172, job). The cause is the release build itself: thePrepare releasecommit (0402103) rewritespackages/*/values.yaml, and two unit tests read exactly the fields it rewrites.hack/cni-plugins-staging-contract.bats"the kube-ovn release the CNI plugin pin was checked against is the one vendored".global.images.kubeovn.taginpackages/system/kubeovn/values.yamlv1.15.26@sha256:…v1.7.0@sha256:…packages/system/monitoring/tests/vtraces_collector_test.yaml"pins the collector image through the cozy-lib.image helper"tracingCollector.imagedocker.io/otel/opentelemetry-collector-contrib:0.136.0@sha256:…ghcr.io/cozystack/cozystack/otelcol-tracing:v0.136.0-cozystack.0@sha256:…Changes:
appVersioninpackages/system/kubeovn/charts/kube-ovn/Chart.yaml. That is the valuepackages/system/kubeovn/Makefilebuilds the image from, and a release build leaves it alone. The hint printed on a mismatch names the new file._cluster.images-registryand asserts that the registry lands in front of the rendered image and that the image stays digest-pinned (thegrafana_upstream_image_plugins_test.yamlpattern). Removing thecozy-lib.imagecall from the template makes it fail; the old regex did not.Verified locally on this branch:
bats hack/cni-plugins-staging-contract.bats10/10,helm unittest packages/system/monitoring -f tests/vtraces_collector_test.yaml28/28.Not a backport:
mainstill carries the pre-release pins, so both tests pass there today. The same patch is open againstmainas #5180, sorelease-1.8does not meet the same surprise.Fixes #5178.
Refs #5172 (blocked on this check).
Downstream repositories
Release note