adjust imagebuild api#40
Merged
Merged
Conversation
📝 WalkthroughWalkthroughRefactors ImageBuild spec to introduce nested AIB and Export blocks, removes several legacy top-level fields, adds provenance status fields, implements many Get* accessors and DeepCopy support for new types, and updates server, handlers, controllers, samples, tests, and CRD/manifest schemas to the new spec shape. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API_Server as BuildAPI
participant Controller
participant Tekton as PipelineRun
participant Registry
Client->>API_Server: Create BuildRequest
API_Server->>API_Server: buildAIBSpec(), buildExportSpec()
API_Server-->>Controller: Create ImageBuild CR (spec.aib/spec.export)
Controller->>Tekton: Create PipelineRun (build tasks, push task if HasDiskExport)
Tekton-->>Controller: PipelineRun status/events
Controller->>Controller: extractProvenance(pipelineRun) -> update status (AIBImageUsed/BuilderImageUsed)
alt Has disk export
Controller->>Registry: Push artifact (using PushSecretRef/Export.OCI or container push)
Registry-->>Controller: Push result
end
Controller-->>Client: Update ImageBuild status (phase, provenance, completion)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
split-image-build
branch
from
7fa3d09 to
020472b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@api/v1alpha1/imagebuild_types.go`:
- Around line 293-310: The current HasDiskExport() (on ImageBuildSpec) should
not assume a legacy disk export when both Export and AIB are nil; change the
logic to avoid false positives by either (A) adding and checking an explicit
legacy marker field (e.g., a boolean like LegacyDiskExport or
LegacyFlatStructure) on ImageBuildSpec and only returning true when that marker
is set, or (B) tightening legacy detection to look for specific legacy top-level
fields (use concrete legacy field names available in the struct) instead of just
Export==nil && AIB==nil; update the HasDiskExport implementation to reference
the chosen symbol (the new LegacyDiskExport flag or the specific legacy field
names) and update related docs/comments accordingly.
In `@internal/buildapi/server.go`:
- Around line 879-896: In buildExportSpec: do not blindly copy req.ExportFormat
into ExportSpec.Format; validate/normalize it against the allowed set
{"raw","qcow2","simg"} and map or default any legacy/invalid values (e.g. map
"image" → "raw" or use "raw" as a safe default) before assigning to
export.Format, and when req.ExportOCI is non-empty ensure disk export will
actually be produced by enforcing export.BuildDiskImage = true (or set
req.BuildDiskImage to true) so a disk export is not silently skipped; adjust
buildExportSpec to perform these checks and normalization before returning the
ExportSpec.
In `@internal/controller/imagebuild/controller.go`:
- Around line 637-658: The push TaskRun is being created with potentially empty
params: repository-url built from imageBuild.Spec.GetLegacyExportURL() and
artifact-filename built from GetDistro()/GetTarget()/GetExportFormat(), which
can produce malformed values; before constructing the tekton params (or before
creating the push TaskRun), validate that imageBuild.Spec.GetLegacyExportURL()
is non-empty and that GetDistro()/GetTarget()/GetExportFormat() produce
non-empty components (or that imageBuild.Spec.HasDiskExport() implies those
fields are present), and if validation fails either return an error/clear
failure from the controller or skip creating the push TaskRun; update the code
path that builds the ParamValue slice (the repository-url and artifact-filename
entries) to only add those params after validation and include a clear error
return from the function that creates the TaskRun when required fields are
missing.
🧹 Nitpick comments (2)
api/v1alpha1/imagebuild_types.go (1)
312-327:GetLegacyExportURLreturns empty string for actual legacy builds.The comment states this is a "temporary compatibility function," but returning an empty string for legacy builds will cause errors rather than providing a migration path. Consider either:
- Removing this function if legacy support isn't needed
- Adding clear documentation that users must migrate
The current implementation only delegates to
GetExportOCI(), making the "Legacy" naming misleading.♻️ Suggested clarification
-// GetLegacyExportURL attempts to determine the export URL for legacy ImageBuilds -// This is a temporary compatibility function -func (s *ImageBuildSpec) GetLegacyExportURL() string { - // For legacy builds, we don't have access to the old Publishers field anymore - // since we removed it from the type. The best we can do is provide a reasonable default - // or require the user to update their ImageBuild to the new structure. - - // If this is a new structure build, use the proper export URL - if url := s.GetExportOCI(); url != "" { - return url - } - - // For legacy builds, we need the user to migrate to the new structure - // Return empty string to force an error that guides them to update - return "" -} +// GetExportURL returns the OCI export URL for disk images. +// Returns empty string if no disk export is configured. +func (s *ImageBuildSpec) GetExportURL() string { + return s.GetExportOCI() +}internal/controller/imagebuild/controller.go (1)
609-611: Consider improving the error message for missing disk export.The current error message "no disk export configured" could be more helpful by guiding users to configure
spec.export.disk.oci.💡 Improved error message
if !imageBuild.Spec.HasDiskExport() { - return fmt.Errorf("no disk export configured") + return fmt.Errorf("no disk export configured: set spec.export.disk.oci to enable pushing to registry") }
Comment on lines
+293
to
+310
| // HasDiskExport returns true if any disk export is configured | ||
| // Includes backward compatibility for legacy ImageBuilds | ||
| func (s *ImageBuildSpec) HasDiskExport() bool { | ||
| // New structure: check export.disk.oci | ||
| if s.Export != nil && s.Export.Disk != nil && s.Export.Disk.OCI != "" { | ||
| return true | ||
| } | ||
|
|
||
| // Legacy compatibility: if this appears to be an old ImageBuild structure, | ||
| // assume disk export is wanted (old behavior was to always export) | ||
| // We detect old structure by checking if Export is nil but other top-level fields exist | ||
| if s.Export == nil && s.AIB == nil { | ||
| // This appears to be a legacy flat structure ImageBuild | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
Review backward compatibility logic in HasDiskExport.
The fallback at lines 304-307 returns true when both Export and AIB are nil, assuming this is a legacy flat-structure ImageBuild. This could cause unintended behavior if a user creates a minimal ImageBuild with neither field set (not legacy, just incomplete).
Consider adding a more explicit legacy detection mechanism or documenting this behavior clearly.
💡 Suggestion: Add explicit legacy marker or improve detection
// HasDiskExport returns true if any disk export is configured
// Includes backward compatibility for legacy ImageBuilds
func (s *ImageBuildSpec) HasDiskExport() bool {
// New structure: check export.disk.oci
if s.Export != nil && s.Export.Disk != nil && s.Export.Disk.OCI != "" {
return true
}
- // Legacy compatibility: if this appears to be an old ImageBuild structure,
- // assume disk export is wanted (old behavior was to always export)
- // We detect old structure by checking if Export is nil but other top-level fields exist
- if s.Export == nil && s.AIB == nil {
- // This appears to be a legacy flat structure ImageBuild
- return true
- }
+ // For new-style ImageBuilds (with AIB or Export defined),
+ // only export if explicitly configured
+ // Legacy builds should be migrated to use the new structure
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // HasDiskExport returns true if any disk export is configured | |
| // Includes backward compatibility for legacy ImageBuilds | |
| func (s *ImageBuildSpec) HasDiskExport() bool { | |
| // New structure: check export.disk.oci | |
| if s.Export != nil && s.Export.Disk != nil && s.Export.Disk.OCI != "" { | |
| return true | |
| } | |
| // Legacy compatibility: if this appears to be an old ImageBuild structure, | |
| // assume disk export is wanted (old behavior was to always export) | |
| // We detect old structure by checking if Export is nil but other top-level fields exist | |
| if s.Export == nil && s.AIB == nil { | |
| // This appears to be a legacy flat structure ImageBuild | |
| return true | |
| } | |
| return false | |
| } | |
| // HasDiskExport returns true if any disk export is configured | |
| // Includes backward compatibility for legacy ImageBuilds | |
| func (s *ImageBuildSpec) HasDiskExport() bool { | |
| // New structure: check export.disk.oci | |
| if s.Export != nil && s.Export.Disk != nil && s.Export.Disk.OCI != "" { | |
| return true | |
| } | |
| // For new-style ImageBuilds (with AIB or Export defined), | |
| // only export if explicitly configured | |
| // Legacy builds should be migrated to use the new structure | |
| return false | |
| } |
🤖 Prompt for AI Agents
In `@api/v1alpha1/imagebuild_types.go` around lines 293 - 310, The current
HasDiskExport() (on ImageBuildSpec) should not assume a legacy disk export when
both Export and AIB are nil; change the logic to avoid false positives by either
(A) adding and checking an explicit legacy marker field (e.g., a boolean like
LegacyDiskExport or LegacyFlatStructure) on ImageBuildSpec and only returning
true when that marker is set, or (B) tightening legacy detection to look for
specific legacy top-level fields (use concrete legacy field names available in
the struct) instead of just Export==nil && AIB==nil; update the HasDiskExport
implementation to reference the chosen symbol (the new LegacyDiskExport flag or
the specific legacy field names) and update related docs/comments accordingly.
Comment on lines
+879
to
+896
| // buildExportSpec creates ExportSpec configuration from build request | ||
| func buildExportSpec(req *BuildRequest) *automotivev1alpha1.ExportSpec { | ||
| export := &automotivev1alpha1.ExportSpec{ | ||
| Format: string(req.ExportFormat), | ||
| Compression: req.Compression, | ||
| BuildDiskImage: req.BuildDiskImage, | ||
| Container: req.ContainerPush, | ||
| } | ||
| return &automotivev1alpha1.Publishers{ | ||
| Registry: &automotivev1alpha1.RegistryPublisher{ | ||
| RepositoryURL: pushRepository, | ||
| Secret: pushSecretName, | ||
| }, | ||
|
|
||
| // Set disk export if OCI URL is specified | ||
| if req.ExportOCI != "" { | ||
| export.Disk = &automotivev1alpha1.DiskExport{ | ||
| OCI: req.ExportOCI, | ||
| } | ||
| } | ||
|
|
||
| return export | ||
| } |
There was a problem hiding this comment.
Avoid writing invalid export.format values (e.g., "image").
ExportSpec.Format is constrained to raw/qcow2/simg; defaulting req.ExportFormat to "image" will serialize an invalid value and can cause CRD validation failures. Also consider enforcing BuildDiskImage when ExportOCI is set so disk export isn't silently skipped.
🛠️ Suggested fix
func buildExportSpec(req *BuildRequest) *automotivev1alpha1.ExportSpec {
export := &automotivev1alpha1.ExportSpec{
- Format: string(req.ExportFormat),
- Compression: req.Compression,
- BuildDiskImage: req.BuildDiskImage,
- Container: req.ContainerPush,
+ Compression: req.Compression,
+ BuildDiskImage: req.BuildDiskImage,
+ Container: req.ContainerPush,
}
+ if req.ExportFormat != "" && req.ExportFormat != formatImage {
+ export.Format = string(req.ExportFormat)
+ }
+
// Set disk export if OCI URL is specified
if req.ExportOCI != "" {
export.Disk = &automotivev1alpha1.DiskExport{
OCI: req.ExportOCI,
}
+ export.BuildDiskImage = true
}
return export
}🤖 Prompt for AI Agents
In `@internal/buildapi/server.go` around lines 879 - 896, In buildExportSpec: do
not blindly copy req.ExportFormat into ExportSpec.Format; validate/normalize it
against the allowed set {"raw","qcow2","simg"} and map or default any
legacy/invalid values (e.g. map "image" → "raw" or use "raw" as a safe default)
before assigning to export.Format, and when req.ExportOCI is non-empty ensure
disk export will actually be produced by enforcing export.BuildDiskImage = true
(or set req.BuildDiskImage to true) so a disk export is not silently skipped;
adjust buildExportSpec to perform these checks and normalization before
returning the ExportSpec.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundle/manifests/automotive.sdv.cloud.redhat.com_imagebuilds.yaml (1)
39-131: Bundle CRD is out of sync with base CRD—regenerate manifests.The bundle manifest at
bundle/manifests/automotive.sdv.cloud.redhat.com_imagebuilds.yamlstill contains the old flat structure (e.g.,spec.distro,spec.exportFormat,spec.containerPush) while the base CRD atconfig/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlhas been refactored to use a nested structure withspec.aibandspec.export. After modifying API types, runmake generate manifeststo regenerate the bundle manifests and keep them in sync with the base CRD schema.
🤖 Fix all issues with AI agents
In `@internal/buildapi/server.go`:
- Around line 980-987: The code creates pushSecretName via createPushSecret()
but never wires it into automotivev1alpha1.ImageBuildSpec, so the push task only
sees imageBuild.Spec.SecretRef; either (A) add a new field PushSecretRef to
ImageBuildSpec and set it when constructing ImageBuildSpec (assign
PushSecretRef: pushSecretName), then update the push task/controller to consume
imageBuild.Spec.PushSecretRef instead of or alongside SecretRef, or (B) if the
env-based registry secret is sufficient, remove the createPushSecret() call and
any related pushSecretName usage; update buildAIBSpec/build/export callers if
they expect the push secret. Ensure you change all references to push logic to
use the chosen field (ImageBuildSpec.PushSecretRef) and adjust
createPushSecret()/push task accordingly.
♻️ Duplicate comments (2)
api/v1alpha1/imagebuild_types.go (1)
293-310: Backward compatibility logic inHasDiskExportneeds attention.The logic at lines 304-307 returns
truewhen bothExportandAIBarenil, assuming legacy flat-structure ImageBuilds. This could incorrectly returntruefor minimal/incomplete new-style ImageBuilds.internal/buildapi/server.go (1)
879-896: InvalidFormatvalue still being written to ExportSpec.The previous review comment about this issue has not been addressed. When
req.ExportFormatdefaults to"image"(set inapplyBuildDefaultsat line 783), line 882 writes this invalid value directly toExportSpec.Format, which is constrained toraw/qcow2/simgper the CRD schema.Consider normalizing or omitting the format when it's
"image":🛠️ Suggested fix
func buildExportSpec(req *BuildRequest) *automotivev1alpha1.ExportSpec { export := &automotivev1alpha1.ExportSpec{ - Format: string(req.ExportFormat), Compression: req.Compression, BuildDiskImage: req.BuildDiskImage, Container: req.ContainerPush, } + // Only set Format for valid disk export formats (raw, qcow2, simg) + if req.ExportFormat != "" && req.ExportFormat != formatImage { + export.Format = string(req.ExportFormat) + } + // Set disk export if OCI URL is specified if req.ExportOCI != "" { export.Disk = &automotivev1alpha1.DiskExport{ OCI: req.ExportOCI, } } return export }
bennyz
force-pushed
the
split-image-build
branch
2 times, most recently
from
fe7ac9f to
d8f9602
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/imagebuild/controller.go`:
- Around line 609-687: The artifact filename is being constructed with a
hardcoded ".gz" which breaks when spec.export.compression is "lz4" or "xz";
update the logic around artifactFilename (used for the push Task generated by
GeneratePushArtifactRegistryTask) to call imageBuild.Spec.GetCompression(), map
the compression to the correct extension (e.g., "gz" for gzip, "lz4" for lz4,
"xz" for xz), and use that extension when building artifactFilename (still using
distro, target and exportFormat); ensure the produced filename is passed into
the "artifact-filename" tekton param.
♻️ Duplicate comments (2)
api/v1alpha1/imagebuild_types.go (1)
302-318: Legacy detection still returns true when both AIB/Export are nil.
Same concern as earlier review: this can yield false positives for incomplete specs.internal/buildapi/server.go (1)
879-896: Avoid writing invalidexport.formatvalues (e.g., "image").The
applyBuildDefaultsfunction setsreq.ExportFormatto"image"when unspecified (line 783), butExportSpec.Formatis constrained by the CRD enum toraw,qcow2, orsimg. Writing"image"will cause CRD validation failure. Since the CRD has a default ofqcow2, consider omitting the format field when the value is"image"or not in the allowed set.🛠️ Suggested fix
func buildExportSpec(req *BuildRequest) *automotivev1alpha1.ExportSpec { export := &automotivev1alpha1.ExportSpec{ - Format: string(req.ExportFormat), Compression: req.Compression, BuildDiskImage: req.BuildDiskImage, Container: req.ContainerPush, } + // Only set format if it's a valid disk format (not the legacy "image" value) + if req.ExportFormat != "" && req.ExportFormat != formatImage { + export.Format = string(req.ExportFormat) + } + // Set disk export if OCI URL is specified if req.ExportOCI != "" { export.Disk = &automotivev1alpha1.DiskExport{ OCI: req.ExportOCI, } } return export }
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
split-image-build
branch
from
d8f9602 to
faea82d
Compare
This was referenced Jan 26, 2026
Merged
This was referenced Feb 8, 2026
This was referenced Apr 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
handles #35
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.