feat: improve catalog discovery UX for scheduled builds - #430
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughCatalog publishing now upserts by registry URL and propagates schedule metadata. Catalog listing adds sorting and latest filtering, while CLI tables and detail views show schedule names, ages, tags, formatted sizes, and expanded metadata. ChangesCatalog lifecycle and discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ImageBuild
participant Publisher
participant CatalogImage
ImageBuild->>Publisher: provide schedule metadata
Publisher->>CatalogImage: find by registry URL
Publisher->>CatalogImage: update existing or create new
sequenceDiagram
participant CLI
participant CatalogAPI
participant CatalogImages
CLI->>CatalogAPI: request sort/latest options
CatalogAPI->>CatalogImages: sort and deduplicate results
CatalogAPI-->>CLI: return catalog responses
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (3)
test/e2e/catalog_discovery_test.go (1)
52-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
applyCatalogImageemits an emptylabels:block iflabelsis nil.All current callers pass labels, but the helper produces invalid YAML (
labels:followed byspec:) otherwise. Emit thelabels:key only when the map is non-empty, mirroring thetagshandling.🤖 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 `@test/e2e/catalog_discovery_test.go` around lines 52 - 87, Update the applyCatalogImage helper so labelYAML includes the labels: key only when labels is non-empty, matching the existing tagsYAML conditional behavior. Ensure CatalogImage manifests generated without labels remain valid YAML while preserving the current formatting when labels are provided.cmd/caib/catalog/list.go (1)
64-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
--sortclient-side.Any value is forwarded, and the API silently falls through to unsorted output for unrecognized values, so
--sort=nmaefails quietly. Reject anything other thancreated/namebefore issuing the request.♻️ Proposed fix
if listSort != "" { + if listSort != "created" && listSort != "name" { + return fmt.Errorf("invalid --sort value %q (supported: created, name)", listSort) + } params.Set("sort", listSort) }Also applies to: 147-149
🤖 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 `@cmd/caib/catalog/list.go` at line 64, Validate the listSort value in the list command before issuing the API request, accepting only "created" and "name" and rejecting all other values with an error. Apply this validation to the flag defined by cmd.Flags().StringVar and the request flow covering the related lines, ensuring invalid input never reaches the API.internal/controller/catalogimage/publisher_test.go (1)
66-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest asserts on its own fixture, not on
PublishFromImageBuild.Nothing here invokes production code beyond
GetContainerPush(); the schedule-name assertion just re-reads the label the test set. UsenewFakePublisher()and callPublishFromImageBuildso the propagation into the CatalogImage label is actually covered.♻️ Sketch
- registryURL := ib.Spec.GetContainerPush() - if registryURL == "" { - t.Fatal("expected container push URL") - } - - scheduleName := ib.Labels[automotivev1alpha1.LabelScheduledImageBuildName] - if scheduleName != "nightly-autosd-qemu" { - t.Errorf("expected schedule name from ImageBuild label, got %q", scheduleName) - } + pub := newFakePublisher() + res, err := pub.PublishFromImageBuild(context.Background(), ib, "", nil, nil, PublishSourceScheduled) + if err != nil { + t.Fatalf("PublishFromImageBuild() error: %v", err) + } + if got := res.CatalogImage.Labels[automotivev1alpha1.LabelScheduledImageBuildName]; got != "nightly-autosd-qemu" { + t.Errorf("expected schedule label propagated, got %q", got) + }Note
PublishFromImageBuildsetsVerifyAccessibility: true, so the fake publisher needs a stubRegistryClientrather thannilto avoid a nil dereference inverifyAndExtractMetadata.🤖 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 66 - 93, Rewrite TestPublishFromImageBuild_PropagatesScheduleName to use newFakePublisher and invoke PublishFromImageBuild with the ImageBuild fixture, rather than reading the fixture’s label directly. Configure the fake publisher with a stub RegistryClient because PublishFromImageBuild enables VerifyAccessibility and verifyAndExtractMetadata must not dereference nil; assert the resulting CatalogImage label contains the expected schedule name.
🤖 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 `@cmd/caib/catalog/list.go`:
- Around line 239-250: Update the catalog list rendering loop around displayName
so NAME always uses img.Name, and add a separate SCHEDULE column populated from
img.ScheduleName. Preserve the existing target and age values, and ensure
scheduled and unscheduled rows remain distinguishable without replacing the
resource identifier.
In `@internal/buildapi/catalog/handlers.go`:
- Around line 140-168: The catalog image handler must apply sorting and
latest-per-group filtering to the complete result set, not a server-paginated
subset. Update the List call and surrounding flow in the handler so client-side
Limit is omitted when sorting or params.Latest is active, then truncate to the
requested limit after the sort and latestGroupKey deduplication and omit the
Continue token; preserve normal pagination when neither is active. Also validate
unrecognized params.Sort values and return HTTP 400 instead of silently
returning unsorted results.
In `@internal/controller/catalogimage/publisher.go`:
- Around line 250-277: Update updateCatalogImage to clear derived labels
LabelBootc, LabelTarget, and LabelScheduledImageBuildName before conditionally
setting them from the current PublishOptions, so removed metadata does not
persist. Also refresh catalogImage.Status.SourceImageBuild from
opts.SourceImageBuildName during Publish after the catalog update succeeds,
ensuring the status change is persisted.
In `@test/e2e/catalog_discovery_test.go`:
- Around line 328-349: The catalog sorting test should verify ordering between
the two fixtures rather than assuming aName is first globally. In the “should
sort alphabetically with --sort name” test, locate both aName and zName in
result.Items and assert that aName’s index is less than zName’s index, while
preserving the existing minimum-item and command-success checks.
---
Nitpick comments:
In `@cmd/caib/catalog/list.go`:
- Line 64: Validate the listSort value in the list command before issuing the
API request, accepting only "created" and "name" and rejecting all other values
with an error. Apply this validation to the flag defined by
cmd.Flags().StringVar and the request flow covering the related lines, ensuring
invalid input never reaches the API.
In `@internal/controller/catalogimage/publisher_test.go`:
- Around line 66-93: Rewrite TestPublishFromImageBuild_PropagatesScheduleName to
use newFakePublisher and invoke PublishFromImageBuild with the ImageBuild
fixture, rather than reading the fixture’s label directly. Configure the fake
publisher with a stub RegistryClient because PublishFromImageBuild enables
VerifyAccessibility and verifyAndExtractMetadata must not dereference nil;
assert the resulting CatalogImage label contains the expected schedule name.
In `@test/e2e/catalog_discovery_test.go`:
- Around line 52-87: Update the applyCatalogImage helper so labelYAML includes
the labels: key only when labels is non-empty, matching the existing tagsYAML
conditional behavior. Ensure CatalogImage manifests generated without labels
remain valid YAML while preserving the current formatting when labels are
provided.
🪄 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 Plus
Run ID: 8baf0401-9882-4ec9-b095-61965eab8b34
📒 Files selected for processing (10)
cmd/caib/catalog/catalog_test.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gointernal/buildapi/catalog/handlers.gointernal/buildapi/catalog/handlers_test.gointernal/buildapi/catalog/models.gointernal/controller/catalogimage/catalogimage_controller.gointernal/controller/catalogimage/publisher.gointernal/controller/catalogimage/publisher_test.gotest/e2e/catalog_discovery_test.go
There was a problem hiding this comment.
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)
internal/buildapi/catalog/handlers.go (1)
97-133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
needsPostProcessingomits phase/tags filters, so limit is applied before filtering.When only
phaseortagsis set (nosort/latest),client.Limit(...)truncates the raw list beforepostFilterAndSortfilters by phase/tags (Lines 369-388). This can return an incomplete page (or empty page) even though more matching items exist — the same class of bug previously flagged for sort/latest, now surfacing for phase/tags.🐛 Proposed fix
- needsPostProcessing := params.Sort != "" || params.Latest + needsPostProcessing := params.Sort != "" || params.Latest || params.Phase != "" || params.Tags != ""🤖 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.go` around lines 97 - 133, Update needsPostProcessing in the catalog list handler to include phase and tags filters, so client.Limit and continuation options are bypassed whenever postFilterAndSort must filter by those parameters. Preserve the existing post-filtering and response limit behavior for all such requests.
🤖 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/buildapi/catalog/handlers.go`:
- Around line 421-431: Update latestGroupKey so images with empty architecture,
distro, and target labels do not share the same deduplication key. Return a
unique per-image key for this all-empty metadata case, while preserving
schedule-based grouping and the existing distro/arch/target grouping for images
with metadata.
- Around line 390-416: Update the sorting and deduplication flow around sortBy
and params.Latest so latest grouping always selects the item with the greatest
CreationTimestamp, independent of the requested display sort. Perform latest
selection in creation-time order, then re-apply the requested sort (including
sortByName) to the filtered items before returning them.
---
Outside diff comments:
In `@internal/buildapi/catalog/handlers.go`:
- Around line 97-133: Update needsPostProcessing in the catalog list handler to
include phase and tags filters, so client.Limit and continuation options are
bypassed whenever postFilterAndSort must filter by those parameters. Preserve
the existing post-filtering and response limit behavior for all such requests.
🪄 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 Plus
Run ID: 4957be33-dedd-4dfa-9a8e-093f998065f0
📒 Files selected for processing (15)
cmd/caib/catalog/add.gocmd/caib/catalog/catalog.gocmd/caib/catalog/catalog_test.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gocmd/caib/catalog/publish.gocmd/caib/catalog/remove.gocmd/caib/catalog/verify.gointernal/buildapi/catalog/handlers.gointernal/buildapi/catalog/handlers_test.gointernal/buildapi/catalog/models.gointernal/controller/catalogimage/catalogimage_controller.gointernal/controller/catalogimage/publisher.gointernal/controller/catalogimage/publisher_test.gotest/e2e/catalog_discovery_test.go
💤 Files with no reviewable changes (1)
- cmd/caib/catalog/catalog.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/caib/catalog/get.go
- internal/controller/catalogimage/catalogimage_controller.go
- internal/buildapi/catalog/handlers_test.go
- test/e2e/catalog_discovery_test.go
- internal/controller/catalogimage/publisher_test.go
- internal/controller/catalogimage/publisher.go
Propagate schedule name labels from ImageBuild to CatalogImage, add sort/latest API params, and improve CLI table output with schedule names, AGE column, and smart TAGS column hiding. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Scheduled builds push to a fixed tag, so successive builds produce CatalogImages with the same registry URL. The old check-and-reject pattern caused all but the first publish to fail silently. Replace checkDuplicates() with a find-or-create pattern: look up an existing CatalogImage by registry URL field index, update its metadata if found, or create a new one otherwise. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
The catalog CLI defaulted to namespace "default" when --namespace wasn't provided, causing lookups to fail when CatalogImages live in the operator namespace. Since users have no direct OpenShift access, namespace selection is unnecessary — the server already resolves its own namespace from the pod's service account. Remove --namespace/-n and --all-namespaces flags from all catalog subcommands. Strip namespace query params from CLI requests and namespace fields from API models. Server handlers now always use h.defaultNamespace instead of accepting client-provided namespace overrides. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
3 participants
Propagate schedule name labels from ImageBuild to CatalogImage,
add sort/latest API params, and improve CLI table output with
schedule names, AGE column, and smart TAGS column hiding.
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
New Features
--latestand--sortoptions for catalog listing, with latest-per-schedule filtering and newest-first default behavior.AGE, optionalTAGScolumn, and friendly sizes; table rows now better reflect available metadata.Tests