feat: initial catalog - #363
Conversation
📝 WalkthroughWalkthroughThe change introduces the ScheduledImageBuild API and controller, integrates scheduled catalog publishing, updates catalog API and CLI output, adds CatalogImage verification failure tracking, and adds ImageBuild target-default resolution with persisted export formats and ownership-aware secret cleanup. ChangesScheduled image build
Catalog API and publishing
ImageBuild controller
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
api/v1alpha1/zz_generated.deepcopy.go (1)
716-732: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the generated deepcopy artifact from this PR.
These methods are generated by
make generate manifests; committing them creates unnecessary generated-file churn.Based on learnings,
api/v1alpha1/zz_generated.deepcopy.gois regenerated during deployment/build and should not be included in PRs.Also applies to: 1372-1396, 1441-1468, 1471-1607
🤖 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 `@api/v1alpha1/zz_generated.deepcopy.go` around lines 716 - 732, Remove the generated ImageBuildTemplateSpec deepcopy methods from zz_generated.deepcopy.go, including the other generated blocks identified by the review, and leave generated-file regeneration to the existing make generate manifests workflow.Source: Learnings
🤖 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 `@api/v1alpha1/scheduledimagebuild_types.go`:
- Around line 53-56: Update the StartingDeadlineSeconds field validation in
ScheduledImageBuild to add a kubebuilder minimum of 0, rejecting negative
deadlines while preserving its optional pointer semantics. Add a failing
validation test first, then regenerate the generated manifests with make
generate manifests.
In `@internal/controller/scheduledimagebuild/controller.go`:
- Around line 118-136: The reconciliation flow currently applies
checkConcurrency before confirming that a scheduled run is due. Move
checkConcurrency after parseScheduleUTC and getMissedAndNext, and invoke it only
when missedRun is non-nil, preserving the existing error handling and creation
flow. Add failing tests covering Replace before the next tick and Replace with
an invalid schedule before implementing the change.
- Around line 97-99: Change the suspended handling in the controller reconcile
flow so isSuspended(sib) prevents only new scheduling, while existing children
continue through active-status updates, completion handling, history cleanup,
and catalog publication. Remove or relocate the early return around isSuspended
and preserve suspended behavior for unscheduled work. Add a failing test
covering completion of an existing child while suspended before implementing the
change.
- Around line 482-484: Update the create flow around the ImageBuild creation
call to add a test first covering create success followed by status-update
failure and deterministic retry. Treat an AlreadyExists result as recoverable by
fetching the existing child, verifying its owner and scheduled identity, then
continuing the status-update path; preserve errors for ownership or identity
mismatches and other create failures.
- Around line 127-129: The reconciliation paths around Status().Update should
avoid writing unchanged status and causing repeated watch events. Add a failing
no-op reconciliation test first, then update each affected status-update block
in the controller’s Reconcile flow to retain the original status, compare it
after mutation, and patch only when changed; preserve existing error handling
for actual status changes.
- Around line 109-113: First add failing tests covering publication errors and
annotation-patch errors, including retention of the source build and prevention
of repeated successful publication. Update handleCompletedBuilds to return an
explicit publication result and propagate both catalog publication and
annotation-patch failures to the caller; make the catalog write idempotent so
retries do not republish successfully completed work. In the reconciliation
flow, call cleanupHistory only when the returned result confirms successful
publication, while retaining unpublished builds.
- Around line 419-445: The missed-run calculation in the scheduled build
controller must return the actual latest missed tick even when more than 100
entries are encountered. First add a failing test covering over 100 missed
ticks, then update the loop around missedRuns to track the newest tick
separately or otherwise handle the exceeded limit, while preserving deadline
filtering and nextRun behavior.
- Around line 451-455: Update the label construction in the scheduled image
build reconciliation flow so template metadata cannot override
LabelScheduledImageBuildName: copy sib.Spec.ImageBuildTemplate.Metadata.Labels
first, then assign the ownership label from sib.Name last. Add a regression test
that fails when a template supplies the reserved label and verifies the
resulting child retains the ScheduledImageBuild ownership label.
---
Nitpick comments:
In `@api/v1alpha1/zz_generated.deepcopy.go`:
- Around line 716-732: Remove the generated ImageBuildTemplateSpec deepcopy
methods from zz_generated.deepcopy.go, including the other generated blocks
identified by the review, and leave generated-file regeneration to the existing
make generate manifests workflow.
🪄 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: CHILL
Plan: Pro
Run ID: 808c4c77-2b21-4c4e-99f3-33f2e56fab63
⛔ Files ignored due to path filters (18)
config/crd/bases/automotive.sdv.cloud.redhat.com_scheduledimagebuilds.yamlis excluded by!config/crd/bases/**config/rbac/role.yamlis excluded by!config/rbac/role.yamlgo.sumis excluded by!**/*.sum,!**go.sumvendor/github.com/robfig/cron/v3/.gitignoreis excluded by!vendor/**vendor/github.com/robfig/cron/v3/.travis.ymlis excluded by!vendor/**vendor/github.com/robfig/cron/v3/LICENSEis excluded by!vendor/**vendor/github.com/robfig/cron/v3/README.mdis excluded by!vendor/**vendor/github.com/robfig/cron/v3/chain.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/constantdelay.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/cron.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/doc.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/logger.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/option.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/parser.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/spec.gois excluded by!vendor/**vendor/k8s.io/utils/clock/testing/fake_clock.gois excluded by!vendor/**vendor/k8s.io/utils/clock/testing/simple_interval_clock.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (19)
PROJECTapi/v1alpha1/labels.goapi/v1alpha1/scheduledimagebuild_types.goapi/v1alpha1/zz_generated.deepcopy.gocmd/caib/catalog/list.gocmd/main.goconfig/crd/kustomization.yamlconfig/manager/kustomization.yamlconfig/manifests/bases/automotive-dev-operator.clusterserviceversion.yamlconfig/samples/automotive_v1alpha1_scheduledimagebuild.yamlconfig/samples/scheduledimagebuild_qa_ebbr.yamlgo.modinternal/buildapi/catalog/handlers.gointernal/buildapi/catalog/models.gointernal/buildapi/catalog/routes.gointernal/buildapi/server.gointernal/controller/catalogimage/publisher.gointernal/controller/scheduledimagebuild/controller.gointernal/controller/scheduledimagebuild/controller_test.go
| r.handleCompletedBuilds(ctx, sib, finished) | ||
|
|
||
| if err := r.cleanupHistory(ctx, sib, finished); err != nil { | ||
| log.Error(err, "Failed to cleanup history") | ||
| return ctrl.Result{}, err |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not clean up builds whose catalog publication failed.
handleCompletedBuilds discards publication and annotation-patch errors, after which cleanupHistory may delete the source build. An annotation failure can also repeat an already-successful external publication. Return an explicit publication result, retain unpublished builds, and make the catalog write idempotent.
As per coding guidelines, add failing publication-error and annotation-patch-error tests before starting implementation.
Also applies to: 267-355
🤖 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 `@internal/controller/scheduledimagebuild/controller.go` around lines 109 -
113, First add failing tests covering publication errors and annotation-patch
errors, including retention of the source build and prevention of repeated
successful publication. Update handleCompletedBuilds to return an explicit
publication result and propagate both catalog publication and annotation-patch
failures to the caller; make the catalog write idempotent so retries do not
republish successfully completed work. In the reconciliation flow, call
cleanupHistory only when the returned result confirms successful publication,
while retaining unpublished builds.
Source: Coding guidelines
32f5e58 to
d977d58
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/buildapi/catalog/handlers_test.go (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the new default-namespace path.
Both requests explicitly specify the namespace, so they cannot detect a regression in
h.defaultNamespace. Omit the query parameter in at least one test, preferably using a non-default configured namespace.Minimal coverage improvement
- req := httptest.NewRequest(http.MethodGet, "/catalog/images/test-image?namespace=default", nil) + req := httptest.NewRequest(http.MethodGet, "/catalog/images/test-image", nil)As per coding guidelines, “Add failing tests before starting implementation.”
Also applies to: 93-93
🤖 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 `@internal/buildapi/catalog/handlers_test.go` at line 59, Update the relevant catalog handler tests around the request construction to omit the namespace query parameter in at least one request, configure the handler with a non-default namespace, and assert that the request uses h.defaultNamespace. Keep another explicitly namespaced request to preserve coverage of the override path.Source: Coding guidelines
internal/controller/scheduledimagebuild/controller_test.go (1)
157-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full owner reference on the created build. Check
Name,UID, andController; otherwise this test still passes if the child build is owned by the wrongScheduledImageBuildor isn’t marked as the controller.🤖 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 `@internal/controller/scheduledimagebuild/controller_test.go` around lines 157 - 166, Expand the owner reference assertions in the scheduled image build test after retrieving ownerRef to validate Name and UID against the parent ScheduledImageBuild and ensure Controller is set as expected, while retaining the existing APIVersion and Kind checks.Source: Learnings
🤖 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 `@internal/controller/imagebuild/controller.go`:
- Around line 2171-2202: Update deleteSecret to determine ownership exclusively
through metav1.GetControllerOf(secret), requiring the controller reference UID
to match ownerUID rather than accepting any owner reference. Capture the fetched
Secret UID and pass it via client.Preconditions{UID: &secretUID} when deleting,
so a replacement Secret is not removed. Add regressions covering non-controller
ownership and replacement between Get and Delete.
In `@internal/controller/scheduledimagebuild/controller_test.go`:
- Around line 766-770: Make the OCI export fixture in the scheduled image build
test admission-valid by adding either SecretRef or UseServiceAccountAuth to the
existing ExportSpec.Disk configuration. Keep the OCI value unchanged and update
only this fixture so it satisfies the CRD requirement.
---
Nitpick comments:
In `@internal/buildapi/catalog/handlers_test.go`:
- Line 59: Update the relevant catalog handler tests around the request
construction to omit the namespace query parameter in at least one request,
configure the handler with a non-default namespace, and assert that the request
uses h.defaultNamespace. Keep another explicitly namespaced request to preserve
coverage of the override path.
In `@internal/controller/scheduledimagebuild/controller_test.go`:
- Around line 157-166: Expand the owner reference assertions in the scheduled
image build test after retrieving ownerRef to validate Name and UID against the
parent ScheduledImageBuild and ensure Controller is set as expected, while
retaining the existing APIVersion and Kind checks.
🪄 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: CHILL
Plan: Pro
Run ID: 98d7f7b4-ad23-4294-88e4-8b98a348a9ad
⛔ Files ignored due to path filters (19)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_scheduledimagebuilds.yamlis excluded by!config/crd/bases/**config/rbac/role.yamlis excluded by!config/rbac/role.yamlgo.sumis excluded by!**/*.sum,!**go.sumvendor/github.com/robfig/cron/v3/.gitignoreis excluded by!vendor/**vendor/github.com/robfig/cron/v3/.travis.ymlis excluded by!vendor/**vendor/github.com/robfig/cron/v3/LICENSEis excluded by!vendor/**vendor/github.com/robfig/cron/v3/README.mdis excluded by!vendor/**vendor/github.com/robfig/cron/v3/chain.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/constantdelay.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/cron.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/doc.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/logger.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/option.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/parser.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/spec.gois excluded by!vendor/**vendor/k8s.io/utils/clock/testing/fake_clock.gois excluded by!vendor/**vendor/k8s.io/utils/clock/testing/simple_interval_clock.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (23)
PROJECTapi/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.goapi/v1alpha1/scheduledimagebuild_types.goapi/v1alpha1/scheduledimagebuild_types_test.goapi/v1alpha1/zz_generated.deepcopy.gocmd/caib/catalog/list.gocmd/main.goconfig/crd/kustomization.yamlconfig/manager/kustomization.yamlconfig/manifests/bases/automotive-dev-operator.clusterserviceversion.yamlconfig/samples/automotive_v1alpha1_scheduledimagebuild.yamlconfig/samples/scheduledimagebuild_qa_ebbr.yamlgo.modinternal/buildapi/catalog/handlers.gointernal/buildapi/catalog/handlers_test.gointernal/buildapi/catalog/models.gointernal/buildapi/catalog/routes.gointernal/buildapi/server.gointernal/controller/catalogimage/publisher.gointernal/controller/imagebuild/controller.gointernal/controller/scheduledimagebuild/controller.gointernal/controller/scheduledimagebuild/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- internal/buildapi/catalog/routes.go
- config/manifests/bases/automotive-dev-operator.clusterserviceversion.yaml
- PROJECT
- api/v1alpha1/labels.go
- config/samples/automotive_v1alpha1_scheduledimagebuild.yaml
- config/crd/kustomization.yaml
- internal/buildapi/server.go
- go.mod
- internal/controller/catalogimage/publisher.go
- internal/buildapi/catalog/models.go
- cmd/main.go
- cmd/caib/catalog/list.go
- config/samples/scheduledimagebuild_qa_ebbr.yaml
- api/v1alpha1/zz_generated.deepcopy.go
- internal/controller/scheduledimagebuild/controller.go
52b1e3e to
db130b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/controller/imagebuild/controller.go (1)
2617-2649: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated target-defaults.yaml parsing logic.
This re-implements a smaller version of the YAML parsing already done in
internal/buildapi/server.go'sloadTargetDefaultsFn(targets map withdefaultFormat), without reusingvalidateTargetDefaultsor the sharedTargetDefaultstype. Errors fromr.Getandyaml.Unmarshalare also silently swallowed here, unlike the buildapi version which wraps and returns them.Based on learnings from
internal/buildapi/server.go:151-197, which loads and validates the same ConfigMap/YAML structure with proper error propagation.🤖 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 `@internal/controller/imagebuild/controller.go` around lines 2617 - 2649, The resolveExportFormat method duplicates target-defaults.yaml parsing and suppresses ConfigMap and YAML errors. Reuse the shared TargetDefaults type and the existing loadTargetDefaultsFn/validateTargetDefaults logic from buildapi, preserving the user-specified format priority and qcow2 fallback while propagating or handling load and validation errors consistently.
🤖 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 `@go.mod`:
- Line 128: Update the github.com/robfig/cron/v3 dependency declaration in
go.mod to remove the // indirect annotation, marking it as a direct dependency
because internal/controller/scheduledimagebuild/controller.go imports it
directly.
In `@internal/controller/imagebuild/controller.go`:
- Around line 2617-2649: Resolve the export format once during build task
creation and persist it in imageBuild.Status.ResolvedExportFormat. Update
createPushTaskRun to reuse that status value instead of calling
resolveExportFormat, ensuring both tasks use the same format even if
aib-target-defaults changes.
- Around line 2617-2649: Update resolveExportFormat to distinguish a
user-specified ExportSpec.Format from the schema-applied "qcow2" default. Allow
target-defaults.yaml to supply a non-empty target-specific default when the
format is absent or only equals the CRD default, while preserving explicitly
requested non-default formats and the final qcow2 fallback.
---
Nitpick comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 2617-2649: The resolveExportFormat method duplicates
target-defaults.yaml parsing and suppresses ConfigMap and YAML errors. Reuse the
shared TargetDefaults type and the existing
loadTargetDefaultsFn/validateTargetDefaults logic from buildapi, preserving the
user-specified format priority and qcow2 fallback while propagating or handling
load and validation errors consistently.
🪄 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: CHILL
Plan: Pro
Run ID: fbe9f788-7fa1-4080-a5e6-41977757cf43
⛔ Files ignored due to path filters (16)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_scheduledimagebuilds.yamlis excluded by!config/crd/bases/**config/rbac/role.yamlis excluded by!config/rbac/role.yamlgo.sumis excluded by!**/*.sum,!**go.sumvendor/github.com/robfig/cron/v3/.gitignoreis excluded by!vendor/**vendor/github.com/robfig/cron/v3/.travis.ymlis excluded by!vendor/**vendor/github.com/robfig/cron/v3/LICENSEis excluded by!vendor/**vendor/github.com/robfig/cron/v3/README.mdis excluded by!vendor/**vendor/github.com/robfig/cron/v3/chain.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/constantdelay.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/cron.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/doc.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/logger.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/option.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/parser.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/spec.gois excluded by!vendor/**
📒 Files selected for processing (26)
PROJECTapi/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.goapi/v1alpha1/scheduledimagebuild_types.goapi/v1alpha1/scheduledimagebuild_types_test.goapi/v1alpha1/zz_generated.deepcopy.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gocmd/main.goconfig/crd/kustomization.yamlconfig/manager/kustomization.yamlconfig/manifests/bases/automotive-dev-operator.clusterserviceversion.yamlconfig/samples/automotive_v1alpha1_scheduledimagebuild.yamlconfig/samples/scheduledimagebuild_qa_ebbr.yamlgo.modinternal/buildapi/catalog/handlers.gointernal/buildapi/catalog/handlers_test.gointernal/buildapi/catalog/models.gointernal/buildapi/catalog/routes.gointernal/buildapi/server.gointernal/common/tasks/scripts/common.shinternal/controller/catalogimage/publisher.gointernal/controller/imagebuild/controller.gointernal/controller/imagebuild/controller_test.gointernal/controller/scheduledimagebuild/controller.gointernal/controller/scheduledimagebuild/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (18)
- config/crd/kustomization.yaml
- config/manager/kustomization.yaml
- config/manifests/bases/automotive-dev-operator.clusterserviceversion.yaml
- config/samples/automotive_v1alpha1_scheduledimagebuild.yaml
- internal/buildapi/catalog/routes.go
- api/v1alpha1/scheduledimagebuild_types_test.go
- PROJECT
- api/v1alpha1/labels.go
- api/v1alpha1/imagebuild_types.go
- internal/buildapi/server.go
- cmd/main.go
- internal/buildapi/catalog/handlers.go
- internal/controller/catalogimage/publisher.go
- config/samples/scheduledimagebuild_qa_ebbr.yaml
- api/v1alpha1/zz_generated.deepcopy.go
- api/v1alpha1/scheduledimagebuild_types.go
- internal/controller/scheduledimagebuild/controller_test.go
- internal/controller/scheduledimagebuild/controller.go
|
@ambient-code please review |
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: feat: initial catalog (ScheduledImageBuild)
Overall this is a well-structured PR that introduces the ScheduledImageBuild CRD following the established CronJob pattern, with good test coverage (~883 lines). The CRD design, matrix builds, concurrency policies, and catalog publishing integration are solid.
I've left inline comments on the items worth addressing. Key themes:
Correctness:
- Template labels can silently override the ownership label
LabelScheduledImageBuildName— this should be set last to prevent it - The
PublishFromImageBuildsignature change uses a variadicsource ...PublishSourcewhich is an unusual Go pattern for a required-in-context parameter — consider making it explicit - The
deleteSecretownership-check improvement is good, but the owner-check uses any OwnerReference UID match rather than specifically checking the controller reference
Robustness:
handleCompletedBuildsswallows publish/annotation-patch errors silently — if publishing fails and the annotation isn't set, the next reconcile will retry publishing, which is fine, but the overall reconcile still returns success potentially causing the status update to commit partial state- When suspended, completed builds are not processed at all (no status updates, no cleanup, no publishing) — this is a design choice worth documenting since it differs from how CronJob handles suspension
Minor:
go.modmarksrobfig/cron/v3as// indirectbut it's directly importedconfig/manager/kustomization.yamlimage tag changed fromv0.2.0-devtov0.0.0— intentional?
There was a problem hiding this comment.
Style: Variadic source ...PublishSource is an unusual pattern.
Using a variadic parameter for a single optional override makes the API less clear — callers can pass 0 or N values and only the first is used. Since there are only two call sites, consider making source an explicit PublishSource parameter to clarify the contract.
There was a problem hiding this comment.
Design consideration: Suspended schedule skips completion handling.
When isSuspended(sib) is true, the controller returns early without processing completed builds, updating status timestamps, cleaning up history, or publishing to catalog. This means:
- A build that completes while the schedule is suspended won't have its success/failure reflected in
lastSuccessfulTime/lastFailedTime - Catalog publishing is deferred until the schedule is unsuspended
- History cleanup doesn't run
This differs from the Kubernetes CronJob controller which continues to track active/completed jobs while suspended. The current behavior may surprise users — consider documenting this, or restructuring so that suspension only prevents new build creation while still processing completions:
// Process completions regardless of suspension
r.handleCompletedBuilds(ctx, sib, finished)
if err := r.cleanupHistory(ctx, sib, finished); err != nil { ... }
r.updateActiveStatus(sib, active)
if isSuspended(sib) {
return r.handleSuspended(ctx, sib, savedStatus)
}
// ... scheduling logic below
There was a problem hiding this comment.
Nit: robfig/cron/v3 is marked // indirect but is directly imported.
internal/controller/scheduledimagebuild/controller.go imports github.com/robfig/cron/v3 directly. Running go mod tidy should fix this annotation.
There was a problem hiding this comment.
Correctness: The >100 missed-run cap with recovery loop is clever but could be simplified.
The current approach iterates up to 100, breaks, then runs a second loop to fast-forward to the actual latest missed tick. This works correctly but the two-loop pattern adds complexity. A simpler approach:
var mostRecent *time.Time
for t := schedule.Next(refTime); !t.After(now); t = schedule.Next(t) {
ts := t
mostRecent = &ts
}The concern is presumably performance if refTime is very old (e.g., a schedule created months ago with */1 * * * *). In that case, the current approach is correct. However, if this is a real concern, consider logging a warning when >100 runs are missed so operators are aware of the catch-up, and perhaps considering the startingDeadlineSeconds window before entering the loop at all (start from max(refTime, now - deadline) when deadline is set).
There was a problem hiding this comment.
Good improvement: ownership-gated secret deletion.
The change from blind-delete to get-check-owner-delete is a solid safety improvement. Two small notes:
-
The
client.Preconditions{UID: &secret.UID}is a nice touch to prevent TOCTOU races between the Get and Delete. -
Consider using
metav1.IsControlledBy(secret, imageBuild)(or checkingmetav1.GetControllerOf(secret)) instead of matching by UID alone viaowner.UID != ownerUID. The current check viaGetControllerOfis correct — just confirming it matches the controller ref specifically, not just any owner ref.
There was a problem hiding this comment.
Question: Intentional image tag change from v0.2.0-dev to v0.0.0?
This looks like it may have been accidentally included. If intentional (e.g., the version is set during CI/CD), it's fine — just want to confirm it's not a stale local change.
There was a problem hiding this comment.
Good fix: Updated to TOML v2 registry config format.
The old [registries.insecure] format is deprecated. The new [[registry]] format with insecure = true is the correct modern syntax.
The .dockerconfigjson fallback and merged auth credentials logic also looks correct for supporting standard Kubernetes docker-registry secrets alongside the custom auth format.
3890a5c to
b0a5795
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/controller/imagebuild/controller_test.go (1)
843-907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest reimplements push logic instead of exercising the real code path.
TestPushUsesPersistedExportFormatcomputesgotby manually repeating the same two lines (ib.Status.ResolvedExportFormatfallback toresolveExportFormat) rather than calling the actual push-time code in the reconciler. This matches today's implementation (confirmed by the downstreamcontroller.gosnippet at lines 1606-1626), but if that logic ever changes or is duplicated elsewhere, this test won't catch the divergence since it only checks its own copy of the logic against itself.Consider extracting this fallback into a small reconciler method (e.g.
effectiveExportFormat(ctx, imageBuild)) used by both the real push path and this test, so the test actually exercises production code.♻️ Suggested refactor
+// effectiveExportFormat returns the format to use for push: the persisted +// Status.ResolvedExportFormat if set, otherwise resolveExportFormat. +func (r *ImageBuildReconciler) effectiveExportFormat(ctx context.Context, imageBuild *automotivev1alpha1.ImageBuild) string { + if imageBuild.Status.ResolvedExportFormat != "" { + return imageBuild.Status.ResolvedExportFormat + } + return r.resolveExportFormat(ctx, imageBuild) +}Then update
controller.go's push path and the test to callr.effectiveExportFormat(ctx, imageBuild)instead of duplicating the two lines.Also applies to: 897-900
🤖 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 `@internal/controller/imagebuild/controller_test.go` around lines 843 - 907, Extract the persisted-format fallback into an ImageBuildReconciler method such as effectiveExportFormat, returning Status.ResolvedExportFormat when set and otherwise resolving via resolveExportFormat. Update the controller’s push path and TestPushUsesPersistedExportFormat to call this method so the test exercises production behavior instead of duplicating the logic.internal/controller/imagebuild/controller.go (1)
2620-2650: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
jsontags for target defaults
sigs.k8s.io/yaml.Unmarshalignoresyamlstruct tags here, so this config currently relies on field-name fallback. SwitchtargetDefaultsandTargetstojson:"..."to make the mapping explicit and avoid silent breakage on future renames.🤖 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 `@internal/controller/imagebuild/controller.go` around lines 2620 - 2650, Update the struct tags used by getTargetDefaults: change targetDefaults.DefaultFormat and targetDefaults.ExtraArgs to json tags, and change the local parsed.Targets field to json:"targets". Keep the existing YAML unmarshalling and lookup behavior unchanged.
🤖 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 `@api/v1alpha1/imagebuild_types.go`:
- Around line 51-52: Update the XValidation rules on ImageBuildSpec for
export.container and export.disk.oci to require pushSecretRef, not secretRef,
unless useServiceAccountAuth is enabled. Keep the existing export presence
checks and exception behavior, and align the validation with the credential
consumed by ImageBuildSpec.GetPushSecretRef().
In `@internal/controller/imagebuild/controller.go`:
- Around line 2625-2650: Update getTargetDefaults to distinguish a missing
ConfigMap from failures: preserve the nil fallback for apierrors.IsNotFound, but
surface other r.Get errors and yaml.Unmarshal failures through the reconciler’s
established logging/event and error-propagation path so reconciliation can
requeue instead of silently using built-in defaults. Adjust the method’s return
contract and callers as needed while keeping successful target lookup behavior
unchanged.
- Around line 2620-2681: Add helper test coverage for malformed
target-defaults.yaml passed to getTargetDefaults, verifying invalid YAML returns
nil and resolveExportFormat/resolveExtraArgs preserve their existing fallback
behavior. Reuse the existing target-defaults ConfigMap test setup and
targetDefaults-related test symbols.
---
Nitpick comments:
In `@internal/controller/imagebuild/controller_test.go`:
- Around line 843-907: Extract the persisted-format fallback into an
ImageBuildReconciler method such as effectiveExportFormat, returning
Status.ResolvedExportFormat when set and otherwise resolving via
resolveExportFormat. Update the controller’s push path and
TestPushUsesPersistedExportFormat to call this method so the test exercises
production behavior instead of duplicating the logic.
In `@internal/controller/imagebuild/controller.go`:
- Around line 2620-2650: Update the struct tags used by getTargetDefaults:
change targetDefaults.DefaultFormat and targetDefaults.ExtraArgs to json tags,
and change the local parsed.Targets field to json:"targets". Keep the existing
YAML unmarshalling and lookup behavior unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 9e863ae6-642c-469e-9fd4-c8bc74028e3e
⛔ Files ignored due to path filters (19)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_scheduledimagebuilds.yamlis excluded by!config/crd/bases/**config/rbac/role.yamlis excluded by!config/rbac/role.yamlgo.sumis excluded by!**/*.sum,!**go.sumvendor/github.com/robfig/cron/v3/.gitignoreis excluded by!vendor/**vendor/github.com/robfig/cron/v3/.travis.ymlis excluded by!vendor/**vendor/github.com/robfig/cron/v3/LICENSEis excluded by!vendor/**vendor/github.com/robfig/cron/v3/README.mdis excluded by!vendor/**vendor/github.com/robfig/cron/v3/chain.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/constantdelay.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/cron.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/doc.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/logger.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/option.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/parser.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/spec.gois excluded by!vendor/**vendor/k8s.io/utils/clock/testing/fake_clock.gois excluded by!vendor/**vendor/k8s.io/utils/clock/testing/simple_interval_clock.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (26)
PROJECTapi/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.goapi/v1alpha1/scheduledimagebuild_types.goapi/v1alpha1/scheduledimagebuild_types_test.goapi/v1alpha1/zz_generated.deepcopy.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gocmd/main.goconfig/crd/kustomization.yamlconfig/manager/kustomization.yamlconfig/manifests/bases/automotive-dev-operator.clusterserviceversion.yamlconfig/samples/automotive_v1alpha1_scheduledimagebuild.yamlconfig/samples/scheduledimagebuild_qa_ebbr.yamlgo.modinternal/buildapi/catalog/handlers.gointernal/buildapi/catalog/handlers_test.gointernal/buildapi/catalog/models.gointernal/buildapi/catalog/routes.gointernal/buildapi/server.gointernal/common/tasks/scripts/common.shinternal/controller/catalogimage/publisher.gointernal/controller/imagebuild/controller.gointernal/controller/imagebuild/controller_test.gointernal/controller/scheduledimagebuild/controller.gointernal/controller/scheduledimagebuild/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
- config/crd/kustomization.yaml
- internal/buildapi/catalog/routes.go
- api/v1alpha1/scheduledimagebuild_types_test.go
- internal/buildapi/server.go
- config/manifests/bases/automotive-dev-operator.clusterserviceversion.yaml
- PROJECT
- api/v1alpha1/labels.go
- config/samples/automotive_v1alpha1_scheduledimagebuild.yaml
- cmd/caib/catalog/get.go
- internal/buildapi/catalog/handlers_test.go
- internal/buildapi/catalog/models.go
- internal/controller/catalogimage/publisher.go
- internal/common/tasks/scripts/common.sh
- internal/buildapi/catalog/handlers.go
- config/samples/scheduledimagebuild_qa_ebbr.yaml
- cmd/main.go
- api/v1alpha1/scheduledimagebuild_types.go
- api/v1alpha1/zz_generated.deepcopy.go
- internal/controller/scheduledimagebuild/controller.go
- internal/controller/scheduledimagebuild/controller_test.go
| // +kubebuilder:validation:XValidation:rule="!(has(self.export) && has(self.export.disk) && has(self.export.disk.oci) && size(self.export.disk.oci) > 0) || size(self.secretRef) > 0 || (has(self.export) && has(self.export.useServiceAccountAuth) && self.export.useServiceAccountAuth)",message="secretRef is required when export.disk.oci is set (unless useServiceAccountAuth is true)" | ||
| // +kubebuilder:validation:XValidation:rule="!(has(self.export) && has(self.export.container) && size(self.export.container) > 0) || size(self.secretRef) > 0 || (has(self.export) && has(self.export.useServiceAccountAuth) && self.export.useServiceAccountAuth)",message="secretRef is required when export.container is set (unless useServiceAccountAuth is true)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'PushSecretRef|SecretRef' internal/controller/imagebuild/controller.go -C2Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 3360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- api/v1alpha1/imagebuild_types.go (relevant section) ---'
sed -n '1,220p' api/v1alpha1/imagebuild_types.go | cat -n | sed -n '1,220p'
echo
echo '--- controller push/auth paths ---'
sed -n '1580,1705p' internal/controller/imagebuild/controller.go | cat -n | sed -n '1,180p'
echo
echo '--- all PushSecretRef/SecretRef references in api and controller ---'
rg -n 'PushSecretRef|SecretRef|useServiceAccountAuth' api/v1alpha1 internal/controller/imagebuild -C 2Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 35606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- export-related controller paths ---'
rg -n 'export\.container|export\.disk\.oci|HasDiskExport|GetLegacyExportURL|GetPushSecretRef|SecretRef' internal/controller/imagebuild/controller.go -C 3
echo
echo '--- export-related helpers in api/v1alpha1/imagebuild_types.go ---'
rg -n 'HasDiskExport|GetLegacyExportURL|GetPushSecretRef|UseServiceAccountAuth|Export' api/v1alpha1/imagebuild_types.go -C 3Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 10818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- controller references to container export and push secret wiring ---'
rg -n 'GetContainerPush|PushSecretRef|SecretRef|export\.container|export\.disk\.oci|secret-ref|registry-auth' internal/controller/imagebuild/controller.go -C 4
echo
echo '--- relevant helper methods around export/container/push ---'
sed -n '450,540p' api/v1alpha1/imagebuild_types.go | cat -nRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 9784
Use pushSecretRef here, not secretRef. The push path passes ImageBuildSpec.GetPushSecretRef() into the registry task, while secretRef is wired separately for registry-auth/flash OCI auth. These rules will reject specs that set only pushSecretRef for export.container or export.disk.oci, and they don’t enforce the credential the push flow actually uses.
🤖 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 `@api/v1alpha1/imagebuild_types.go` around lines 51 - 52, Update the
XValidation rules on ImageBuildSpec for export.container and export.disk.oci to
require pushSecretRef, not secretRef, unless useServiceAccountAuth is enabled.
Keep the existing export presence checks and exception behavior, and align the
validation with the credential consumed by ImageBuildSpec.GetPushSecretRef().
| func (r *ImageBuildReconciler) getTargetDefaults(ctx context.Context, target string) *targetDefaults { | ||
| if target == "" { | ||
| return nil | ||
| } | ||
| cm := &corev1.ConfigMap{} | ||
| if err := r.Get(ctx, types.NamespacedName{ | ||
| Name: "aib-target-defaults", | ||
| Namespace: controllerutils.OperatorNamespace(), | ||
| }, cm); err != nil { | ||
| return nil | ||
| } | ||
| data, ok := cm.Data["target-defaults.yaml"] | ||
| if !ok { | ||
| return nil | ||
| } | ||
| var parsed struct { | ||
| Targets map[string]targetDefaults `yaml:"targets"` | ||
| } | ||
| if err := yaml.Unmarshal([]byte(data), &parsed); err != nil { | ||
| return nil | ||
| } | ||
| if t, ok := parsed.Targets[target]; ok { | ||
| return &t | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
getTargetDefaults silently swallows all errors, including transient ones, not just NotFound.
Any error from r.Get (e.g. a transient API-server error) or yaml.Unmarshal (a malformed target-defaults.yaml) is treated identically to "no defaults configured": the function returns nil with no log, and callers silently fall back to "qcow2" / no extra args. A misconfigured ConfigMap or a momentary API hiccup can therefore cause a build to silently use the wrong export format or extra args for a target, with no signal to operators and no reconcile retry.
Distinguish IsNotFound (expected, no-op fallback) from other errors (log/emit event, and consider returning the error so the reconciler can requeue rather than silently building with wrong defaults).
🤖 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 `@internal/controller/imagebuild/controller.go` around lines 2625 - 2650,
Update getTargetDefaults to distinguish a missing ConfigMap from failures:
preserve the nil fallback for apierrors.IsNotFound, but surface other r.Get
errors and yaml.Unmarshal failures through the reconciler’s established
logging/event and error-propagation path so reconciliation can requeue instead
of silently using built-in defaults. Adjust the method’s return contract and
callers as needed while keeping successful target lookup behavior unchanged.
|
@ambient-code please review |
There was a problem hiding this comment.
Code Review: feat: initial catalog (ScheduledImageBuild) — re-review
Nice work on this PR. The ScheduledImageBuild CRD is well-designed, following the CronJob pattern closely. The export-format resolution from ConfigMap, ownership-gated secret deletion, and updated registries.conf format are all solid improvements. Test coverage is thorough.
Key findings
1. Suspended schedule still skips completed-build processing
(Previously flagged — appears unresolved)
When isSuspended(sib) returns true, the controller returns early (line ~120 of controller.go) before handleCompletedBuilds, cleanupHistory, or updateActiveStatus run. This means:
status.activecan reference builds that already finishedlastSuccessfulTime/lastFailedTimewon't update- Catalog publishing for builds that complete while suspended is skipped
- History cleanup won't run, potentially retaining more builds than the limit
Consider moving the suspension check after the build lifecycle handling.
2. getTargetDefaults swallows all errors including transient ones
(Previously flagged — appears unresolved)
At controller.go:2650, any r.Get error (including network timeouts, RBAC issues) returns nil, causing the controller to silently fall back to qcow2. This could lead to builds using the wrong export format without any indication. Consider distinguishing NotFound (safe to ignore) from other errors (should be logged at minimum).
3. PublishFromImageBuild variadic source parameter
(Previously flagged — appears unresolved)
The variadic source ...PublishSource pattern on PublishFromImageBuild is unusual for a single optional override. A more idiomatic approach would be to use a pointer (*PublishSource) or always require the parameter. The variadic allows callers to accidentally pass multiple values, of which only the first is used.
4. Template labels can override ownership label
The ownership label LabelScheduledImageBuildName is set before template labels are copied (line ~537), so a user template with automotive.sdv.cloud.redhat.com/scheduledimagebuild-name: something-else would overwrite the controller's label, breaking the listing query in listOwnedBuilds. Set the ownership label after copying template labels.
5. Missed-run cap loop can be simplified
The two-stage loop at getMissedAndNext (first loop up to 100, then fast-forward loop) works correctly but could be replaced with a simpler reverse scan from now: walk backward from the current time to find the most recent scheduled time, rather than forward from the last run. This avoids the 100-iteration cap entirely.
Minor / Nits
- The
go.modshould markgithub.com/robfig/cron/v3as a direct dependency (not// indirect) since it's imported directly in the controller. appendOCITagSuffixusesLastIndex(ref, ":")which will match the port separator in references likeregistry:5000/repo(no tag). The test at line 4445 confirms this —"registry.example.com/repo"+"-qemu"="registry.example.com/repo-qemu"which works by accident since there's no colon, butmyregistry:5000/image+"-arm"would producemyregistry:5000/image-arm(correct) whilemyregistry:5000/ns/image:tag+"-arm"works correctly. Looks OK for now but worth noting in a comment that this assumes tags contain no colons.cleanupHistoryhardcodes default limits (3 and 1) that match the kubebuilder defaults — consider extracting these as constants to keep them in sync.
Overall this is solid work. The main actionable item is the suspended-schedule lifecycle gap (finding #1) and the label ordering issue (#4). The others are improvements worth considering.
| r.updateActiveStatus(sib, active) | ||
|
|
||
| if isSuspended(sib) { | ||
| return r.handleSuspended(ctx, sib, savedStatus) |
There was a problem hiding this comment.
The suspension check returns early before handleCompletedBuilds, cleanupHistory, and updateActiveStatus run. Builds that complete while the schedule is suspended will not have their status reflected, catalog publishing will be skipped, and status.active may become stale.
Consider restructuring so that the completed-build handling and status tracking always run, and only the "should we create a new build" decision is gated by the suspend check.
| return &t | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
getTargetDefaults returns nil for any r.Get error, including transient network failures or RBAC errors. This silently falls back to qcow2 without any indication.
Consider distinguishing NotFound (safe to ignore) from other errors:
if err := r.Get(ctx, types.NamespacedName{...}, cm); err != nil {
if !errors.IsNotFound(err) {
log.Error(err, "Failed to read aib-target-defaults ConfigMap, falling back to defaults")
}
return nil
}| // PublishFromImageBuild creates a CatalogImage from a completed ImageBuild. | ||
| // An optional source parameter overrides the default PublishSourceImageBuild. | ||
| func (p *Publisher) PublishFromImageBuild( | ||
| ctx context.Context, |
There was a problem hiding this comment.
Nit: The variadic source ...PublishSource pattern is unusual for a single optional override. Consider using a pointer parameter (*PublishSource) or always requiring the source — this makes the API contract clearer and prevents callers from accidentally passing multiple values.
| func appendOCITagSuffix(ref, suffix string) string { | ||
| if i := strings.LastIndex(ref, ":"); i != -1 { | ||
| return ref[:i] + ":" + ref[i+1:] + suffix | ||
| } | ||
| return ref + suffix | ||
| } |
There was a problem hiding this comment.
When ref sent with no tag, the suffix is appended directly to the repo path: registry.example.com/repo + -qemu → registry.example.com/repo-qemu this creates a different repository name rather than adding a tag.
probably users expect tagged images, not different repos
| build.Annotations[AnnotationCatalogPublished] = "true" | ||
| if err := r.Patch(ctx, build, patch); err != nil { | ||
| log.Error(err, "Failed to annotate ImageBuild as published") | ||
| } |
There was a problem hiding this comment.
This will publish then patches the annotation catalog-published=true on the ImageBuild
If reconciliation runs twice before the annotation patch is visible (e.g., a second event fires while the first reconcile is still running), the build could be published twice.
The checkDuplicates call in publisher catches same-URL duplicates, if the duplicate check is by name (same ImageBuild → same catalog name), the second publish would fail with AlreadyExists that's caught as an error and logged as a warning, which is noisy but not harmful
| catalogName string, | ||
| tags []string, | ||
| authSecretRef *automotivev1alpha1.AuthSecretReference, | ||
| ) error |
There was a problem hiding this comment.
in the interface PublishFromImageBuild returns error only, while the real Publisher.PublishFromImageBuild publisher.go returns (*PublishResult, error).
The adapter in main bridges this by discarding the PublishResult. this works fine but means the controller can't classify between verified vs unverified publish
| if retainUnpublished && excess[i].Annotations[AnnotationCatalogPublished] == "" { | ||
| r.Log.Info("Retaining unpublished ImageBuild", "name", excess[i].Name) | ||
| continue | ||
| } |
There was a problem hiding this comment.
builds without the catalog-published annotation are never deleted by history cleanup. If publishing is broken (publisher down, auth issues), unpublished builds accumulate indefinitely
| now := time.Now() | ||
| builds := []automotivev1alpha1.ImageBuild{ | ||
| {Status: automotivev1alpha1.ImageBuildStatus{Phase: "Building"}}, | ||
| {Status: automotivev1alpha1.ImageBuildStatus{Phase: automotivev1alpha1.ImageBuildPhaseCompleted}}, | ||
| {Status: automotivev1alpha1.ImageBuildStatus{Phase: "Pending"}}, | ||
| {Status: automotivev1alpha1.ImageBuildStatus{Phase: automotivev1alpha1.ImageBuildPhaseFailed}}, | ||
| } | ||
| _ = now |
There was a problem hiding this comment.
now := time.Now() followed by _ = now that's a dead code and can be removed
| if count > 100 && mostRecent != nil { | ||
| for t := schedule.Next(*mostRecent); !t.After(now); t = schedule.Next(t) { | ||
| ts := t | ||
| mostRecent = &ts | ||
| } | ||
| } |
There was a problem hiding this comment.
fast-forward loop after 100-cap has no safety bound and could iterate millions of times for very frequent schedules with long gaps
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/controller/imagebuild/controller.go (1)
2643-2648: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
getTargetDefaultsstill silently swallows malformed YAML.The
r.Geterror path now logs non-NotFounderrors, but a malformedtarget-defaults.yamlstill falls throughyaml.Unmarshalfailure to a silentnilreturn, exactly the gap called out in the earlier review (which flagged bothr.Getandyaml.Unmarshalfailures). Operators get no signal that their ConfigMap is broken; the build silently falls back to"qcow2"/no extra args.🩺 Proposed fix
if err := yaml.Unmarshal([]byte(data), &parsed); err != nil { + r.Log.Error(err, "Failed to parse aib-target-defaults ConfigMap target-defaults.yaml, falling back to defaults") return 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 `@internal/controller/imagebuild/controller.go` around lines 2643 - 2648, Update getTargetDefaults so yaml.Unmarshal failures are logged with the relevant error and ConfigMap context before returning nil. Preserve the existing nil-return fallback, while ensuring malformed target-defaults.yaml no longer fails silently.
🧹 Nitpick comments (2)
internal/controller/catalogimage/publisher_test.go (1)
94-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract metadata building to avoid tautological tests.
The test validates the
ExportFormatandBootcvalues by manually copying the exact inline logic fromPublishFromImageBuild(e.g.,ib.Spec.GetContainerPush() != ""andib.Spec.GetMode() == "bootc"). As a result, the test is tautological—it doesn't exercise the actual production code and would still pass if the implementation inPublishFromImageBuildwere altered or removed.Consider extracting this inline logic into a helper function in
publisher.goso it can be tested directly, ensuring the production behavior is properly validated.♻️ Proposed fix to test the production logic
Update the test to use the extracted helper:
- exportFormat := resolvedExportFormat(ib) - if ib.Spec.GetContainerPush() != "" { - exportFormat = "oci" - } - - if exportFormat != tt.wantFormat { - t.Errorf("ExportFormat = %q, want %q", exportFormat, tt.wantFormat) - } - - bootc := ib.Spec.GetMode() == "bootc" - if bootc != tt.wantBootc { - t.Errorf("Bootc = %v, want %v", bootc, tt.wantBootc) - } + metadata := buildCatalogImageMetadata(ib) + + if metadata.ExportFormat != tt.wantFormat { + t.Errorf("ExportFormat = %q, want %q", metadata.ExportFormat, tt.wantFormat) + } + + if metadata.Bootc != tt.wantBootc { + t.Errorf("Bootc = %v, want %v", metadata.Bootc, tt.wantBootc) + }Apply the following out-of-bounds changes to extract the helper in
internal/controller/catalogimage/publisher.go:--- internal/controller/catalogimage/publisher.go +++ internal/controller/catalogimage/publisher.go @@ -183,23 +183,7 @@ catalogName = imageBuild.Name } - // Build metadata from ImageBuild - exportFormat := resolvedExportFormat(imageBuild) - if imageBuild.Spec.GetContainerPush() != "" { - exportFormat = "oci" - } - - metadata := &automotivev1alpha1.CatalogImageMetadata{ - Architecture: NormalizeArchitecture(imageBuild.Spec.Architecture), - Distro: imageBuild.Spec.GetDistro(), - BuildMode: imageBuild.Spec.GetMode(), - ExportFormat: exportFormat, - Bootc: imageBuild.Spec.GetMode() == "bootc", - } - - // Add hardware target if specified - if imageBuild.Spec.GetTarget() != "" { - metadata.Targets = []automotivev1alpha1.HardwareTarget{ - {Name: imageBuild.Spec.GetTarget(), Verified: true}, - } - } + metadata := buildCatalogImageMetadata(imageBuild) publishSource := source if publishSource == "" { @@ -359,6 +343,26 @@ +func buildCatalogImageMetadata(imageBuild *automotivev1alpha1.ImageBuild) *automotivev1alpha1.CatalogImageMetadata { + exportFormat := resolvedExportFormat(imageBuild) + if imageBuild.Spec.GetContainerPush() != "" { + exportFormat = "oci" + } + + metadata := &automotivev1alpha1.CatalogImageMetadata{ + Architecture: NormalizeArchitecture(imageBuild.Spec.Architecture), + Distro: imageBuild.Spec.GetDistro(), + BuildMode: imageBuild.Spec.GetMode(), + ExportFormat: exportFormat, + Bootc: imageBuild.Spec.GetMode() == "bootc", + } + + if imageBuild.Spec.GetTarget() != "" { + metadata.Targets = []automotivev1alpha1.HardwareTarget{ + {Name: imageBuild.Spec.GetTarget(), Verified: true}, + } + } + + return metadata +} + func resolvedExportFormat(imageBuild *automotivev1alpha1.ImageBuild) string {🤖 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 `@internal/controller/catalogimage/publisher_test.go` around lines 94 - 106, Extract the shared metadata-building logic for ExportFormat and Bootc from PublishFromImageBuild into a helper in publisher.go, preserving the current container-push and mode behavior. Update publisher_test.go to call that production helper instead of duplicating the inline logic, so the test directly validates the implementation.internal/controller/imagebuild/controller.go (1)
1103-1106: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
resolveExtraArgsis re-resolved a second time within the samecreateBuildTaskRuncall.
resolveExtraArgsis called once for theaib-extra-argsparam (line 1106) and again insidecreateOrUpdateManifestConfigMap(line 1579), each re-fetching and re-parsing theaib-target-defaultsConfigMap. This mirrors the drift risk that was just fixed forexportFormatby resolving once and persisting/reusing it — extra args have no equivalent single-resolution guarantee, so the PipelineRun param and the manifest ConfigMap contents could theoretically diverge if the ConfigMap changes mid-reconcile, plus it's redundant work.♻️ Suggested direction
func (r *ImageBuildReconciler) createBuildTaskRun(...) error { ... exportFormat := r.resolveExportFormat(ctx, imageBuild) + extraArgs := r.resolveExtraArgs(ctx, imageBuild) ... { Name: "aib-extra-args", Value: tektonv1.ParamValue{ Type: tektonv1.ParamTypeString, - StringVal: strings.Join(r.resolveExtraArgs(ctx, imageBuild), "\n"), + StringVal: strings.Join(extraArgs, "\n"), }, }, ... - manifestConfigMapName, err := r.createOrUpdateManifestConfigMap(ctx, imageBuild) + manifestConfigMapName, err := r.createOrUpdateManifestConfigMap(ctx, imageBuild, extraArgs) } -func (r *ImageBuildReconciler) createOrUpdateManifestConfigMap(ctx context.Context, imageBuild *automotivev1alpha1.ImageBuild) (string, error) { +func (r *ImageBuildReconciler) createOrUpdateManifestConfigMap(ctx context.Context, imageBuild *automotivev1alpha1.ImageBuild, extraArgs []string) (string, error) { ... - if extraArgs := r.resolveExtraArgs(ctx, imageBuild); len(extraArgs) > 0 { + if len(extraArgs) > 0 { cm.Data["aib-extra-args.txt"] = strings.Join(extraArgs, "\n") }Also applies to: 1576-1581
🤖 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 `@internal/controller/imagebuild/controller.go` around lines 1103 - 1106, Resolve extra arguments once within createBuildTaskRun, store the result in a local value, and reuse that value both for the aib-extra-args PipelineRun parameter and the createOrUpdateManifestConfigMap call. Update the relevant createOrUpdateManifestConfigMap inputs so it no longer invokes resolveExtraArgs independently, preserving a single consistent resolution throughout the reconcile.
🤖 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 `@internal/controller/scheduledimagebuild/controller.go`:
- Around line 700-705: The appendOCITagSuffix function incorrectly treats a
registry port colon as a tag separator. First add a failing test covering an
untagged host:port reference, then update the separator check to use the last
colon only when it occurs after the last slash; preserve existing behavior for
tagged references and refs without a tag.
- Around line 396-407: Update Reconciler.updateActiveStatus to assign each
status.active ObjectReference the explicit ImageBuild GVK from
automotivev1alpha1.GroupVersion.WithKind("ImageBuild") instead of copying
APIVersion and Kind from active entries, while preserving the existing name,
namespace, and UID values. Add or update a test verifying status.active contains
the expected APIVersion and Kind.
---
Duplicate comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 2643-2648: Update getTargetDefaults so yaml.Unmarshal failures are
logged with the relevant error and ConfigMap context before returning nil.
Preserve the existing nil-return fallback, while ensuring malformed
target-defaults.yaml no longer fails silently.
---
Nitpick comments:
In `@internal/controller/catalogimage/publisher_test.go`:
- Around line 94-106: Extract the shared metadata-building logic for
ExportFormat and Bootc from PublishFromImageBuild into a helper in publisher.go,
preserving the current container-push and mode behavior. Update
publisher_test.go to call that production helper instead of duplicating the
inline logic, so the test directly validates the implementation.
In `@internal/controller/imagebuild/controller.go`:
- Around line 1103-1106: Resolve extra arguments once within createBuildTaskRun,
store the result in a local value, and reuse that value both for the
aib-extra-args PipelineRun parameter and the createOrUpdateManifestConfigMap
call. Update the relevant createOrUpdateManifestConfigMap inputs so it no longer
invokes resolveExtraArgs independently, preserving a single consistent
resolution throughout the reconcile.
🪄 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: CHILL
Plan: Pro
Run ID: 2f59903a-497e-477e-a05f-51dabba0ae77
⛔ Files ignored due to path filters (20)
config/crd/bases/automotive.sdv.cloud.redhat.com_catalogimages.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_scheduledimagebuilds.yamlis excluded by!config/crd/bases/**config/rbac/role.yamlis excluded by!config/rbac/role.yamlgo.sumis excluded by!**/*.sum,!**go.sumvendor/github.com/robfig/cron/v3/.gitignoreis excluded by!vendor/**vendor/github.com/robfig/cron/v3/.travis.ymlis excluded by!vendor/**vendor/github.com/robfig/cron/v3/LICENSEis excluded by!vendor/**vendor/github.com/robfig/cron/v3/README.mdis excluded by!vendor/**vendor/github.com/robfig/cron/v3/chain.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/constantdelay.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/cron.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/doc.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/logger.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/option.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/parser.gois excluded by!vendor/**vendor/github.com/robfig/cron/v3/spec.gois excluded by!vendor/**vendor/k8s.io/utils/clock/testing/fake_clock.gois excluded by!vendor/**vendor/k8s.io/utils/clock/testing/simple_interval_clock.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (28)
PROJECTapi/v1alpha1/catalogimage_types.goapi/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.goapi/v1alpha1/scheduledimagebuild_types.goapi/v1alpha1/scheduledimagebuild_types_test.goapi/v1alpha1/zz_generated.deepcopy.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gocmd/main.goconfig/crd/kustomization.yamlconfig/manifests/bases/automotive-dev-operator.clusterserviceversion.yamlconfig/samples/automotive_v1alpha1_scheduledimagebuild.yamlconfig/samples/scheduledimagebuild_qa_ebbr.yamlgo.modinternal/buildapi/catalog/handlers.gointernal/buildapi/catalog/handlers_test.gointernal/buildapi/catalog/models.gointernal/buildapi/catalog/routes.gointernal/buildapi/server.gointernal/common/tasks/scripts/common.shinternal/controller/catalogimage/catalogimage_controller.gointernal/controller/catalogimage/publisher.gointernal/controller/catalogimage/publisher_test.gointernal/controller/imagebuild/controller.gointernal/controller/imagebuild/controller_test.gointernal/controller/scheduledimagebuild/controller.gointernal/controller/scheduledimagebuild/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- config/manifests/bases/automotive-dev-operator.clusterserviceversion.yaml
- cmd/caib/catalog/get.go
- api/v1alpha1/scheduledimagebuild_types_test.go
- api/v1alpha1/labels.go
- internal/buildapi/catalog/routes.go
- PROJECT
- config/crd/kustomization.yaml
- config/samples/scheduledimagebuild_qa_ebbr.yaml
- config/samples/automotive_v1alpha1_scheduledimagebuild.yaml
- api/v1alpha1/imagebuild_types.go
- internal/buildapi/server.go
- internal/buildapi/catalog/handlers_test.go
- cmd/main.go
- api/v1alpha1/scheduledimagebuild_types.go
- internal/buildapi/catalog/handlers.go
- internal/common/tasks/scripts/common.sh
- internal/controller/imagebuild/controller_test.go
- internal/buildapi/catalog/models.go
- cmd/caib/catalog/list.go
- api/v1alpha1/zz_generated.deepcopy.go
- internal/controller/scheduledimagebuild/controller_test.go
Initial implementation of the catalog feature, include a new CRD ScheduledImageBuild (SIB) Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
Initial implementation of the catalog feature, include a new CRD ScheduledImageBuild (SIB) Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
2 participants
Summary
Introduced scheduled builds to catalog.
For example:
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
New Features
Bug Fixes