Upgrade operator as part of the test suite

[alicefr]

Exercise the operator upgrade path and validate the the node upgrade still works after we have upgrade the operator from the last release to the latest version

Fixes: #14

[redhat-chai-bot]

Detailed Code Review

Thanks for the upgrade test — the 4-phase architecture (delete → install released → upgrade → verify) is clean and the finalizer-aware teardown ordering is correct. I went through every line; here's what I found.

---

🔴 Issues to Fix

1. Duplicated RELEASED_OPERATOR_IMG env in CI workflow

File: .github/workflows/ci.yaml

The same expression is copy-pasted into two separate step-level env: blocks:

      - name: Deploy to bink cluster
        env:
          RELEASED_OPERATOR_IMG: ghcr.io/${{ github.repository }}:${{ env.RELEASED_OPERATOR_TAG }}
        run: make deploy-bink

      - name: Run e2e tests
        env:
          RELEASED_OPERATOR_IMG: ghcr.io/${{ github.repository }}:${{ env.RELEASED_OPERATOR_TAG }}
        run: make e2e V=1

If the expression ever changes, you'll need to update both, and they can drift silently.

Fix: Move RELEASED_OPERATOR_IMG once to the job-level env: block (under jobs.e2e.env) or to the existing top-level env: next to RELEASED_OPERATOR_TAG. Both steps then inherit it.

---

2. CRD name list repeated twice in deleteCurrentOperator()

File: test/e2e/upgrade_test.go

The slice []string{"bootcnodepools.node.bootc.dev", "bootcnodes.node.bootc.dev"} is hard-coded in two places — the deletion loop and the Eventually verification block:

// First occurrence (line ~307):
for _, crdName := range []string{
    "bootcnodepools.node.bootc.dev",
    "bootcnodes.node.bootc.dev",
} {

// Second occurrence (line ~324):
for _, crdName := range []string{
    "bootcnodepools.node.bootc.dev",
    "bootcnodes.node.bootc.dev",
} {

If a CRD is added later, only one copy might be updated.

Fix: Extract to a package-level variable:

var operatorCRDs = []string{
    "bootcnodepools.node.bootc.dev",
    "bootcnodes.node.bootc.dev",
}

---

3. containerImage() silently returns "" — latent bug

File: test/e2e/upgrade_test.go, line ~482

func containerImage(containers []corev1.Container, name string) string {
    for _, c := range containers {
        if c.Name == name {
            return c.Image
        }
    }
    return ""
}

If the "manager" container is ever renamed or absent from the Deployment spec, containerImage returns "" silently. That empty string flows into currentImg, and t.Cleanup will attempt to restore the operator with an empty image — leaving the cluster broken without any obvious error.

Fix: Make it test-aware and fail fast:

func containerImage(t *testing.T, containers []corev1.Container, name string) string {
    t.Helper()
    for _, c := range containers {
        if c.Name == name {
            return c.Image
        }
    }
    t.Fatalf("container %q not found in pod spec", name)
    return "" // unreachable
}

---

4. Hardcoded push destination vs IMG_BINK_RELEASED — drift risk

File: Makefile

The push target in push-released-operator-image hardcodes the destination:

podman push --tls-verify=false $(RELEASED_OPERATOR_IMG) localhost:5000/bootc-operator-released:latest

Meanwhile, the cluster-internal address is a separate variable:

IMG_BINK_RELEASED ?= registry.cluster.local:5000/bootc-operator-released:latest

The image path bootc-operator-released:latest is now duplicated in two places — if someone changes one, the other silently breaks.

Fix: Introduce an explicit local variable:

IMG_BINK_RELEASED_LOCAL ?= localhost:5000/bootc-operator-released:latest
IMG_BINK_RELEASED       ?= registry.cluster.local:5000/bootc-operator-released:latest

and use $(IMG_BINK_RELEASED_LOCAL) in the recipe. Both values derive from one source of truth.

---

🟡 Repeated Code / DRY Improvements

5. bytesReader wrapper is unnecessary

File: test/e2e/upgrade_test.go, line ~491

func bytesReader(b []byte) io.Reader {
    return bytes.NewReader(b)
}

This is a one-line wrapper around bytes.NewReader that adds indirection without adding value. It forces readers to look up the function to verify it doesn't do something special. Used in patchManifestImage and kubectlApply.

Fix: Remove bytesReader and inline bytes.NewReader(manifest) at the two call sites.

---

6. BootcNode Idle polling block duplicated (Phase 2 & Phase 4)

File: test/e2e/upgrade_test.go

The g.Eventually(func() ... { Get BootcNode → return Status }).Should(...) block is structurally identical in Phase 2 (line ~192) and Phase 4 (line ~225). Same fetch function, nearly the same matchers — Phase 4 adds an ImageDigest check.

Fix: Extract a helper:

func waitForNodeIdle(g Gomega, c client.Client, ctx context.Context,
    nodeName string, timeout time.Duration, extra ...types.GomegaMatcher) {
    matchers := []types.GomegaMatcher{
        HaveField("Booted", Not(BeNil())),
        HaveField("Conditions", ContainElement(And(
            HaveField("Type", bootcv1alpha1.NodeIdle),
            HaveField("Status", metav1.ConditionTrue),
            HaveField("Reason", bootcv1alpha1.NodeReasonIdle),
        ))),
    }
    matchers = append(matchers, extra...)
    g.Eventually(func() (bootcv1alpha1.BootcNodeStatus, error) {
        var bn bootcv1alpha1.BootcNode
        err := c.Get(ctx, client.ObjectKey{Name: nodeName}, &bn)
        return bn.Status, err
    }).WithTimeout(timeout).Should(And(matchers...))
}

Phase 2 calls waitForNodeIdle(g, env.Client, ctx, nodeName, 3*time.Minute).
Phase 4 calls waitForNodeIdle(g, env.Client, ctx, nodeName, 5*time.Minute, HaveField("ImageDigest", ...)).

---

⚪ Style / Nits

7. context.Context parameter ordering

File: test/e2e/upgrade_test.go

Go convention places ctx context.Context as the first parameter (or second, after *testing.T). In waitForOperatorReady, installReleasedOperator, and deleteCurrentOperator, ctx is last:

func waitForOperatorReady(
    t *testing.T,
    g Gomega,
    c client.Client,
    ctx context.Context, // ← Go convention: move after t
)

Not wrong, but unusual — consider (t *testing.T, ctx context.Context, g Gomega, ...) for consistency with the ecosystem.

---

8. Naming inconsistency between Make variables and env vars

File: Makefile

$(if $(RELEASED_OPERATOR_TAG),E2E_OPERATOR_RELEASE_TAG=$(RELEASED_OPERATOR_TAG))
$(if $(RELEASED_OPERATOR_IMG),E2E_OPERATOR_RELEASED_IMG=$(IMG_BINK_RELEASED))

• RELEASED_OPERATOR_TAG → E2E_OPERATOR_RELEASE_TAG (drops the "D")
• RELEASED_OPERATOR_IMG → E2E_OPERATOR_RELEASED_IMG (keeps the "D")

The inconsistent RELEASE vs RELEASED makes it easy to typo. Consider aligning both to either RELEASE_* or RELEASED_*.

---

9. patchManifestImage passes img twice via fmt.Sprintf

File: test/e2e/upgrade_test.go, line ~368

yqExpr := fmt.Sprintf(
    `(select(...)).image = "%s" | `+
        `(select(...)).image = "%s"`,
    img,
    img,
)

Passing the same argument twice is error-prone — if you later change one %s you might forget the other.

Fix: Use positional verbs:

yqExpr := fmt.Sprintf(
    `(select(...)).image = "%[1]s" | `+
        `(select(...)).image = "%[1]s"`,
    img,
)

---

10. downloadReleaseManifest has no HTTP timeout

File: test/e2e/upgrade_test.go, line ~345

resp, err := http.Get(url) //nolint:gosec

http.Get uses the default client with no timeout. If the GitHub CDN hangs, the test blocks until the 40-minute go test -timeout kills it — without a useful error message.

Fix:

httpClient := &http.Client{Timeout: 30 * time.Second}
resp, err := httpClient.Get(url) //nolint:gosec

---

11. yq is an undocumented runtime dependency

File: test/e2e/upgrade_test.go, line ~375

patchManifestImage shells out to yq, but nothing in the PR (or the repo README from what I can see) declares it as a requirement. A missing yq produces an opaque exec error.

Fix: Guard the call:

if _, err := exec.LookPath("yq"); err != nil {
    t.Fatal("yq is required for upgrade tests but not found in PATH")
}

---

✅ What Looks Good

• Finalizer-aware ordering — deleting CRs before the operator ensures CRD deletion doesn't hang. This is a common pitfall done correctly here.
• t.Cleanup for restoration — registering after the released operator is installed means it runs regardless of outcome.
• t.Helper() usage — consistently applied in all helpers for clean backtraces.
• Phase 4 is a real functional test — patches the pool image and waits for the full update lifecycle, proving the upgrade didn't break the control loop.
• env.go scheme registration — adding apiextensionsv1.AddToScheme to the shared client builder is the right place.
• Guard t.Skip on missing env vars — keeps the upgrade test opt-in and the default e2e suite unaffected.
• findRepoRoot — the parent == dir guard correctly handles the filesystem root boundary.
• deleteCurrentOperator CR ordering — deleting BootcNodePool and BootcNode first, then waiting for empty lists before touching the Deployment/DaemonSet/CRDs is the right sequence.
• applyCurrentManifests kustomize fallback — checking bin/kustomize first, falling back to PATH, is pragmatic.

---

Summary

Severity	Count	Key items
🔴 Fix	4	Duplicated CI env, duplicated CRD list, silent "" return bug, hardcoded push path
🟡 DRY	2	Remove bytesReader, extract waitForNodeIdle
⚪ Nit	5	ctx ordering, naming inconsistency, positional %[1]s, HTTP timeout, yq guard

Nothing is a runtime correctness blocker today, but issue #3 (containerImage returning "") could cause a very confusing CI failure if the Deployment spec ever changes. The DRY items are worth addressing to keep the test maintainable as the operator grows.

---

AI-generated. Review for accuracy.

[redhat-chai-bot]

Addendum: Context from #14 — "Also manage the controller and daemonset manifests"

After reading through #14, here's how the team's agreed direction reframes some of the findings above.

Background

The team reached consensus on matching the MCO pattern:

1. Bake all manifests into the container image
2. Add a new cmd/operator binary that syncs its baked manifests
3. Add a deployment manifest for the operator
4. Upgrading = just updating the operator Deployment image — the operator itself handles cascading updates to the DaemonSet

A key motivator (from @alicefr, the same author as this PR): the operator can control when the DaemonSet is updated — avoiding restarting daemon pods while nodes are in staging/booting phase. @cheesesashimi also noted this ties into #160 for garbage collection on operator deletion.

How this reframes the review

The patchManifestImage / yq approach (findings #9, #11) is a stopgap.
This PR shells out to yq to rewrite image references in downloaded YAML manifests. Once #14 lands, upgrading is "update the operator Deployment image, the operator syncs the rest." The test would simplify dramatically — no manifest downloading, no yq patching, no kubectlApply. This makes finding #11 (undocumented yq dependency) less critical long-term, but the test still needs to work now.

---

The manual delete → reinstall dance (finding #2) will eventually simplify.
Right now the test manually deletes CRs, CRDs, the Deployment, and the DaemonSet in careful order. Once the operator manages its own manifests with proper garbage collection (#160), uninstalling should be "delete the operator Deployment" and the operator handles teardown. The duplicated CRD list is a symptom of this manual approach.

---

containerImage() (finding #3) matters even more.
The upgrade path in #14's vision is: update the operator Deployment image → operator syncs everything else. That means containerImage() — which reads the current operator image from the Deployment — sits at the exact hinge point of the upgrade flow. The silent "" return is even more dangerous in that context, since a wrong image means the entire upgrade chain breaks silently. This is the finding I'd most strongly recommend fixing before merge.

---

The polling duplication (finding #6) will grow.
When the operator manages the DaemonSet, the upgrade test will need to verify both the operator Deployment image and the DaemonSet image updated correctly. Extracting waitForNodeIdle now avoids more copy-paste when those assertions are added.

---

Bottom line

This PR is testing the upgrade path as it exists today (external manifest management). All findings still stand — especially #3 (containerImage silent failure) and the DRY items — but the broader direction from #14 means some of this test plumbing will simplify once the operator self-manages its manifests. Getting the upgrade test in place now is good to catch regressions during that transition.

---

AI-generated. Review for accuracy.

[alicefr]

@ptalgulk01 do you feel we can merge this or does it require further reviews?

[ptalgulk01]

this looks good, thankyou for changes
/lgtm