refactor(operator): package annotation helpers accept any client.Object (#300)

[ayuskauskas]

Second PR of the #223 series (closes #300). Targets feature/package-as-jobs, which now sits on the #311 API-rename base.

What

GetPackage / SetPackages / InvalidatePackage / IsInvalidPackage in operator/internal/controller/annotations.go only ever touch an object's annotations, but were typed to *corev1.Pod. This widens them to client.Object — switching from the .Annotations field to GetAnnotations() / SetAnnotations() — so the same package metadata can ride on batch/v1 Jobs and their pod templates.

This is the first code step of the Jobs migration (design in #307): the Job builders (#301) will set the package annotation on both the Job and its pod template (the in-flight erroring path reads it off the child pod).

No behavior change

Every existing call site passes a *corev1.Pod, which already satisfies client.Object, so the call sites are unchanged and compile as-is. The defensive nil guard stays as an interface-nil check; no caller passes a typed-nil pod.

Tests

New annotations_test.go (TDD — written first, watched fail to compile against the *corev1.Pod signature, then made to pass):

• round-trips SetPackages → GetPackage on a *batchv1.Job and asserts it is identical to the same round-trip on a *corev1.Pod (same annotation key, same decoded value);
• exercises InvalidatePackage / IsInvalidPackage on a *batchv1.Job.

Verification

• go build ./... + go vet ./internal/controller/: clean.
• Controller suite in isolation: 185/185 specs pass (the 2 new specs plus every existing caller-exercising spec — no regressions).
• make lint (golangci-lint + license check): 0 issues; gofmt clean.

Heads-up (unrelated to this PR): the full make unit-tests (ginkgo ./..., shared --timeout=180s) does not complete on this base — the cmd/cli/app/package suite is slow (individual package rerun specs take ~30s each; e.g. should accept multiple --node flags passes in isolation but in ~33s), so the aggregate run trips the ginkgo timeout before the controller suite runs. This branch has zero CLI diff — the slowness is pre-existing on the fix/api-rename base and worth a separate look.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Package annotation helpers now accept metav1.Object instances instead of Pods, using object annotations to read, write, and invalidate serialized package data. Tests cover Pod and Job round trips, typed-nil inputs, missing annotations, and Job invalidation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: lockwobr, rice-riley

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the refactor to make package annotation helpers accept broader object types.
Description check	✅ Passed	The description matches the changes by explaining the object-type refactor, nil handling, and Job-focused tests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jobs-migration/300-annotation-helpers

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

operator/internal/controller/annotations.go (1)

93-125: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Handle objects without a package annotation.

GetPackage returns (nil, nil) for a missing annotation, then both helpers dereference pkg. An unannotated Job panics instead of being treated as invalidation no-op / not-invalid.

• operator/internal/controller/annotations.go#L93-L125: return early when pkg == nil.
• operator/internal/controller/annotations_test.go#L73-L85: cover InvalidatePackage and IsInvalidPackage on an empty Job.

Proposed fix

 pkg, err := GetPackage(obj)
 if err != nil {
     return fmt.Errorf("error getting package: %w", err)
 }
+if pkg == nil {
+    return nil
+}
 
 pkg.Invalid = true

 pkg, err := GetPackage(obj)
 if err != nil {
     return false, fmt.Errorf("error getting package: %w", err)
 }
+if pkg == nil {
+    return false, nil
+}
 return pkg.Invalid, nil

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@operator/internal/controller/annotations.go` around lines 93 - 125, Handle
nil packages returned by GetPackage in InvalidatePackage and IsInvalidPackage:
return nil from InvalidatePackage and false, nil from IsInvalidPackage before
dereferencing pkg. Add coverage in
operator/internal/controller/annotations_test.go:73-85 using an empty Job to
verify both helpers treat a missing package annotation without panic. Update
operator/internal/controller/annotations.go:93-125 for the root fix; the test
site requires corresponding cases only.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@operator/internal/controller/annotations.go`:
- Line 45: Define a named constant for the package annotation key using
v1alpha1.METADATA_PREFIX and the “/package” suffix in the nearest appropriate
file. Update the annotation read and write paths, including the code around
GetAnnotations and the locations at the other referenced ranges, to reuse this
constant instead of reconstructing the key.
- Line 41: Update GetPackage and the related annotation helpers in
operator/internal/controller/annotations.go to accept metav1.Object or a minimal
annotation-only interface instead of client.Object, so *corev1.PodTemplateSpec
is supported. In operator/internal/controller/annotations_test.go, add a
round-trip test using &job.Spec.Template that verifies annotations can be
written and read successfully.

---

Outside diff comments:
In `@operator/internal/controller/annotations.go`:
- Around line 93-125: Handle nil packages returned by GetPackage in
InvalidatePackage and IsInvalidPackage: return nil from InvalidatePackage and
false, nil from IsInvalidPackage before dereferencing pkg. Add coverage in
operator/internal/controller/annotations_test.go:73-85 using an empty Job to
verify both helpers treat a missing package annotation without panic. Update
operator/internal/controller/annotations.go:93-125 for the root fix; the test
site requires corresponding cases only.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 0a3bb469-9bef-4816-a1ff-2daf1c7893b1

📥 Commits

Reviewing files that changed from the base of the PR and between 7ba9f6e and 2a5bfb8.

📒 Files selected for processing (2)

• operator/internal/controller/annotations.go
• operator/internal/controller/annotations_test.go

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@operator/internal/controller/annotations_test.go`:
- Around line 50-78: Extend the round-trip assertions in the “round-trips the
package annotation identically...” test to verify fromPod.Version,
fromPod.Image, and fromPod.ContainerSHA against the expected package values,
alongside the existing Skyhook, Stage, and Name checks. Keep the cross-resource
equality and annotation assertions unchanged.

In `@operator/internal/controller/annotations.go`:
- Around line 47-50: Update the metadata helpers GetPackage, SetPackages,
InvalidatePackage, and IsInvalidPackage to use one shared nil-check helper that
detects both nil interfaces and typed-nil metav1.Object pointers before
accessing annotations or mutating objects. Replace their direct nil checks with
this helper while preserving existing behavior for non-nil objects.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: b85ab0fd-a072-4392-92c7-e32dbee5e89f

📥 Commits

Reviewing files that changed from the base of the PR and between 2a5bfb8 and 61a61b7.

📒 Files selected for processing (2)

• operator/internal/controller/annotations.go
• operator/internal/controller/annotations_test.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

operator/internal/controller/annotations.go (1)

106-116: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle absent package annotations before dereferencing.

GetPackage returns (nil, nil) when the annotation is missing (Lines 63-66). Both InvalidatePackage and IsInvalidPackage then dereference pkg, causing a panic on otherwise valid unannotated objects. Handle pkg == nil explicitly and add a regression test for an unannotated Job or Pod.

Also applies to: 134-143

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@operator/internal/controller/annotations.go` around lines 106 - 116, Handle a
nil package returned by GetPackage in both InvalidatePackage and
IsInvalidPackage before dereferencing pkg. Preserve the existing behavior for
annotated objects, and add a regression test covering an unannotated Job or Pod
to verify no panic and the expected result.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@operator/internal/controller/annotations.go`:
- Around line 106-116: Handle a nil package returned by GetPackage in both
InvalidatePackage and IsInvalidPackage before dereferencing pkg. Preserve the
existing behavior for annotated objects, and add a regression test covering an
unannotated Job or Pod to verify no panic and the expected result.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: dd2f3667-a4c9-489c-8ac7-2bf5de905412

📥 Commits

Reviewing files that changed from the base of the PR and between 61a61b7 and da24d20.

📒 Files selected for processing (2)

• operator/internal/controller/annotations.go
• operator/internal/controller/annotations_test.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

operator/internal/controller/annotations.go (1)

111-116: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle missing package annotations before dereferencing pkg.

GetPackage returns (nil, nil) when the annotation is absent, so both pkg.Invalid = true and return pkg.Invalid can panic for otherwise valid objects without package metadata. Return a no-op/false result when pkg == nil, and add regression tests for unannotated objects.

🛠️ Proposed fix

  pkg, err := GetPackage(obj)
  if err != nil {
    return fmt.Errorf("error getting package: %w", err)
  }
+ if pkg == nil {
+   return nil
+ }
  pkg.Invalid = true

  pkg, err := GetPackage(obj)
  if err != nil {
    return false, fmt.Errorf("error getting package: %w", err)
  }
+ if pkg == nil {
+   return false, nil
+ }
  return pkg.Invalid, nil

Also applies to: 139-143

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@operator/internal/controller/annotations.go` around lines 111 - 116, Update
the function containing GetPackage and pkg.Invalid so it handles a nil package
before dereferencing it: after successful GetPackage, return the existing
no-op/false result when pkg == nil, while preserving the current error path and
annotated-object behavior. Apply the same guard to the corresponding return
pkg.Invalid path, and add regression coverage for objects without package
annotations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@operator/internal/controller/annotations.go`:
- Around line 111-116: Update the function containing GetPackage and pkg.Invalid
so it handles a nil package before dereferencing it: after successful
GetPackage, return the existing no-op/false result when pkg == nil, while
preserving the current error path and annotated-object behavior. Apply the same
guard to the corresponding return pkg.Invalid path, and add regression coverage
for objects without package annotations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 033a0bf0-8084-48a2-963b-6325453eae70

📥 Commits

Reviewing files that changed from the base of the PR and between da24d20 and 8b711e4.

📒 Files selected for processing (1)

• operator/internal/controller/annotations.go

[coveralls]

Coverage Report for CI Build 29611718659

Warning

No base build found for commit e21aaf2 on feature/package-as-jobs.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 78.041%

Details

• Patch coverage: 4 uncovered changes across 1 file (30 of 34 lines covered, 88.24%).

Uncovered Changes

File	Changed	Covered	%
operator/internal/controller/annotations.go	34	30	88.24%

Coverage Regressions

Requires a base build to compare against. How to fix this →

---

Coverage Stats

Relevant Lines:	12619
Covered Lines:	9848
Line Coverage:	78.04%
Coverage Strength:	7.87 hits per line

---

💛 - Coveralls