[BUILD-1746] feat: map DockerStrategy.From to runtime-stage-from param - #143
Conversation
|
Warning Review limit reached
Next review available in: 31 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughDocker BuildConfig conversion now maps Docker strategy ChangesDocker strategy From conversion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BuildConfigConverter
participant KubernetesClient
participant ShipwrightBuild
BuildConfigConverter->>BuildConfigConverter: validate Docker From kind
BuildConfigConverter->>KubernetesClient: fetch ImageStreamImage
KubernetesClient-->>BuildConfigConverter: return DockerImageReference
BuildConfigConverter->>ShipwrightBuild: append runtime-stage-from parameter
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
…m param Maps BuildConfig DockerStrategy.From field to Shipwright's runtime-stage-from parameter, supporting ImageStreamTag, ImageStreamImage, and DockerImage kinds. Resolves: BUILD-1746 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nstant - Remove RuntimeStageFromRFE from rfe.go (feature now implemented) - Collapse identical ImageStreamTag/ImageStreamImage cases into single case - Extract "runtime-stage-from" to RuntimeStageFromParamName constant - Update tests to use constant instead of hardcoded strings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
… of ImageStreamTag resolveImageStreamRef always fetched ImageStreamTag objects (expects name:tag format), causing failures when called with ImageStreamImage refs (name@sha256:digest). Added resolveImageStreamImageRef that fetches the correct API type and returns DockerImageReference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
convert/buildconfigs.go (2)
326-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate parameter creation logic.
The
ParamValuestruct initialization andappendcall are identical across the cases. Extracting the image resolution and parameter construction reduces code duplication and improves maintainability.♻️ Proposed refactor
+ var imageRef string + var err error switch fromKind := from.Kind; fromKind { case ImageStreamTag: - imageRef, err := t.resolveImageStreamRef(from.Name, from.Namespace) - if err != nil { - return err - } - paramValue := shipwrightv1beta1.ParamValue{ - Name: RuntimeStageFromParamName, - SingleValue: &shipwrightv1beta1.SingleValue{ - Value: &imageRef, - }, - } - b.Spec.ParamValues = append(b.Spec.ParamValues, paramValue) + imageRef, err = t.resolveImageStreamRef(from.Name, from.Namespace) case ImageStreamImage: - imageRef, err := t.resolveImageStreamImageRef(from.Name, from.Namespace) - if err != nil { - return err - } - paramValue := shipwrightv1beta1.ParamValue{ - Name: RuntimeStageFromParamName, - SingleValue: &shipwrightv1beta1.SingleValue{ - Value: &imageRef, - }, - } - b.Spec.ParamValues = append(b.Spec.ParamValues, paramValue) + imageRef, err = t.resolveImageStreamImageRef(from.Name, from.Namespace) case DockerImage: - paramValue := shipwrightv1beta1.ParamValue{ - Name: RuntimeStageFromParamName, - SingleValue: &shipwrightv1beta1.SingleValue{ - Value: &from.Name, - }, - } - b.Spec.ParamValues = append(b.Spec.ParamValues, paramValue) + imageRef = from.Name default: return fmt.Errorf("docker strategy 'From' kind %s is unknown for BuildConfig %s", fromKind, bc.Name) } + + if err != nil { + return err + } + + paramValue := shipwrightv1beta1.ParamValue{ + Name: RuntimeStageFromParamName, + SingleValue: &shipwrightv1beta1.SingleValue{ + Value: &imageRef, + }, + } + b.Spec.ParamValues = append(b.Spec.ParamValues, paramValue)🤖 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 `@convert/buildconfigs.go` around lines 326 - 361, Consolidate the duplicated ParamValue construction and b.Spec.ParamValues append logic in the from.Kind switch. Have each case only determine the image reference—resolving ImageStreamTag and ImageStreamImage as needed, while using from.Name for DockerImage—then construct and append the shared parameter once after the switch, preserving existing error returns and unknown-kind handling.
238-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead code for Docker strategies.
Since Docker strategy
Fromfields are now exclusively routed to the newly introducedprocessDockerStrategyFromFieldmethod, theDockerStrategyfallback block here is unreachable and can be safely removed.♻️ Proposed refactor
var from *corev1.ObjectReference - if bc.Spec.Strategy.DockerStrategy != nil && bc.Spec.Strategy.DockerStrategy.From != nil { - from = bc.Spec.Strategy.DockerStrategy.From - } else if bc.Spec.Strategy.SourceStrategy != nil && bc.Spec.Strategy.SourceStrategy.From.Name != "" { + if bc.Spec.Strategy.SourceStrategy != nil && bc.Spec.Strategy.SourceStrategy.From.Name != "" { from = &bc.Spec.Strategy.SourceStrategy.From🤖 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 `@convert/buildconfigs.go` around lines 238 - 241, Remove the DockerStrategy condition and assignment from the strategy conversion logic, leaving the SourceStrategy From handling intact. The DockerStrategy From field is now handled exclusively by processDockerStrategyFromField, so update the surrounding if/else flow without changing SourceStrategy behavior.
🤖 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.
Nitpick comments:
In `@convert/buildconfigs.go`:
- Around line 326-361: Consolidate the duplicated ParamValue construction and
b.Spec.ParamValues append logic in the from.Kind switch. Have each case only
determine the image reference—resolving ImageStreamTag and ImageStreamImage as
needed, while using from.Name for DockerImage—then construct and append the
shared parameter once after the switch, preserving existing error returns and
unknown-kind handling.
- Around line 238-241: Remove the DockerStrategy condition and assignment from
the strategy conversion logic, leaving the SourceStrategy From handling intact.
The DockerStrategy From field is now handled exclusively by
processDockerStrategyFromField, so update the surrounding if/else flow without
changing SourceStrategy behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18ae5071-7e65-421e-8bdc-c0496fff90ce
📒 Files selected for processing (3)
convert/buildconfigs.goconvert/buildconfigs_test.goconvert/rfe.go
💤 Files with no reviewable changes (1)
- convert/rfe.go
51aea8a to
2782121
Compare
…trategy fallback - Extract image resolution into switch cases and build ParamValue once after - Remove unreachable DockerStrategy branch from processStrategyFromField since Docker From is now handled by processDockerStrategyFromField Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Spec.Strategy.DockerStrategy.Fromfield to Shipwright Build'sruntime-stage-fromparamFrom.Kindtypes: DockerImage (literal), ImageStreamTag (resolved), ImageStreamImage (resolved via dedicated API)RuntimeStageFromRFEwarning constant — the feature is now implementedChanges
processDockerStrategyFromField()— new function that resolves the From reference and sets theruntime-stage-fromparam valueresolveImageStreamImageRef()— new function for resolvingImageStreamImagereferences (separate from the existingresolveImageStreamRefwhich handlesImageStreamTag)ImageStreamImagecase inprocessStrategyFromFieldandprocessCustomBuildSourceImagesto use the correct resolverRuntimeStageFromRFEconstant fromrfe.goTest plan
Dependencies
runtime-stage-fromparam to buildah strategy)Related
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes