fix(ci-operator): keep registry.ci builder FROM when final stage is external#5244
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExtractRegistryReferences now collapses backslash-newline continuations, inspects FROM, COPY, and RUN lines for RegistryRegex, tracks the registry match per-FROM line (resetting on each FROM), deduplicates matches, and excludes only the final matching FROM registry when ChangesDockerfile Registry Reference Tracking
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/dockerfile/extract.go (1)
26-27: 💤 Low valueConsider documenting the
fromparameter's behavior.The function comment doesn't explain what the
fromparameter does (i.e., when non-empty, it excludes the final stage's registry reference). While the inline comment at line 57 is clear, adding a sentence to the function doc would help callers understand the contract without reading the implementation.📝 Suggested documentation enhancement
-// ExtractRegistryReferences finds all registry.ci.openshift.org and quay-proxy.ci.openshift.org references in the Dockerfile +// ExtractRegistryReferences finds all registry.ci.openshift.org and quay-proxy.ci.openshift.org references in the Dockerfile. +// If from is non-empty, the registry reference from the final FROM stage is excluded from the result. func ExtractRegistryReferences(dockerfile []byte, from api.PipelineImageStreamTagReference) []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 `@pkg/dockerfile/extract.go` around lines 26 - 27, Update the ExtractRegistryReferences function comment to document the behavior of the from parameter: state that when the from parameter (api.PipelineImageStreamTagReference) is non-empty, the function will exclude registry references found in the final stage of the Dockerfile from the returned slice; if from is empty, all matching registry references (including the final stage) are returned. Mention this contract alongside the existing description so callers understand the parameter's effect without reading the implementation.
🤖 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 `@pkg/dockerfile/extract.go`:
- Around line 26-27: Update the ExtractRegistryReferences function comment to
document the behavior of the from parameter: state that when the from parameter
(api.PipelineImageStreamTagReference) is non-empty, the function will exclude
registry references found in the final stage of the Dockerfile from the returned
slice; if from is empty, all matching registry references (including the final
stage) are returned. Mention this contract alongside the existing description so
callers understand the parameter's effect without reading the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 24132de9-e2f7-417c-9bc9-3f5cb5a81397
📒 Files selected for processing (2)
pkg/dockerfile/extract.gopkg/dockerfile/inputs_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
openshift-ci
Bot
added
the
approved
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
deepsm007
force-pushed
the
fix/dockerfile-inputs-build-root-wire
branch
2 times, most recently
from
a1ce255 to
bf11200
Compare
openshift-ci
Bot
removed
the
do-not-merge/hold
…quay-proxy Collapse Dockerfile backslash line continuations before scanning so registry.ci references split across RUN lines are detected for quay-proxy inputs.
deepsm007
force-pushed
the
fix/dockerfile-inputs-build-root-wire
branch
from
bf11200 to
61b8243
Compare
|
/test e2e |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/dockerfile/extract.go (2)
39-46: 💤 Low valueMinor: Avoid redundant regex evaluation on FROM lines.
Lines 41-42 call
RegistryRegex.Find(line)to populatelastFromLineRegistryRef, then line 46 calls it again on the same FROM line. The match from line 41 could be reused to avoid evaluating the regex twice.♻️ Proposed optimization
if bytes.HasPrefix(upper, []byte("FROM")) { lastFromLineRegistryRef = "" - if match := RegistryRegex.Find(line); match != nil { - lastFromLineRegistryRef = string(match) - } } - match := RegistryRegex.Find(line) + match := RegistryRegex.Find(line) + if bytes.HasPrefix(upper, []byte("FROM")) && match != nil { + lastFromLineRegistryRef = string(match) + } + if match == nil { continue }🤖 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 `@pkg/dockerfile/extract.go` around lines 39 - 46, The RegistryRegex is being evaluated twice for the same FROM line; refactor to call RegistryRegex.Find(line) only once and reuse the result: inside the FROM branch capture the result into a local variable (e.g., match) and set lastFromLineRegistryRef when match != nil, then use that same match for the subsequent logic instead of calling RegistryRegex.Find(line) again; update references to the variable names lastFromLineRegistryRef and RegistryRegex in pkg/dockerfile/extract.go accordingly so the regex is only executed once per line.
35-37: ⚡ Quick winInconsistent instruction detection may cause false positives.
Line 35 uses
bytes.Containsto check forFROM,COPY, andRUN, which matches these keywords anywhere in the line (including comments or string literals). Line 39 usesbytes.HasPrefixforFROM, which is more precise. This inconsistency meansCOPYandRUNkeywords inside comments (e.g.,# Note: COPY --from=...) or strings could trigger processing.Consider using
bytes.HasPrefixconsistently for all three instructions, or use a Dockerfile parser library for robust instruction detection.🤖 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 `@pkg/dockerfile/extract.go` around lines 35 - 37, The condition that checks Dockerfile instructions uses bytes.Contains for COPY and RUN which can match keywords inside comments or strings; update the check that uses the variable upper (the upper-cased line) to use bytes.HasPrefix after trimming leading whitespace so it consistently mirrors the existing FROM detection (replace the bytes.Contains checks for "COPY" and "RUN" with bytes.HasPrefix on bytes.TrimLeft(upper, " \t")), and ensure lines beginning with '#' are skipped before this check to avoid false positives.
🤖 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 `@pkg/dockerfile/extract.go`:
- Around line 39-46: The RegistryRegex is being evaluated twice for the same
FROM line; refactor to call RegistryRegex.Find(line) only once and reuse the
result: inside the FROM branch capture the result into a local variable (e.g.,
match) and set lastFromLineRegistryRef when match != nil, then use that same
match for the subsequent logic instead of calling RegistryRegex.Find(line)
again; update references to the variable names lastFromLineRegistryRef and
RegistryRegex in pkg/dockerfile/extract.go accordingly so the regex is only
executed once per line.
- Around line 35-37: The condition that checks Dockerfile instructions uses
bytes.Contains for COPY and RUN which can match keywords inside comments or
strings; update the check that uses the variable upper (the upper-cased line) to
use bytes.HasPrefix after trimming leading whitespace so it consistently mirrors
the existing FROM detection (replace the bytes.Contains checks for "COPY" and
"RUN" with bytes.HasPrefix on bytes.TrimLeft(upper, " \t")), and ensure lines
beginning with '#' are skipped before this check to avoid false positives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8b4e967e-210c-4611-842a-43a7e136f07b
📒 Files selected for processing (2)
pkg/dockerfile/extract.gopkg/dockerfile/inputs_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/dockerfile/inputs_test.go
|
/override ci/prow/integration unrelated failures to the PR |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/integration DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/images |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/images DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, hector-vido The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/override ci/prow/unit |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/images, ci/prow/unit DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@deepsm007: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
/cc @openshift/test-platform
Dockerfile Input Detection with External Final Stage
Component: ci-operator (Dockerfile parsing for build input detection)
Problem: ci-operator's Dockerfile scanner previously missed some registry.ci.openshift.org references in real-world Dockerfiles. It also incorrectly excluded all registry.ci references when the final stage used an external base image (via
from), which broke multi-stage builds that relied on registry.ci builder stages.What changed:
podman pullare detected.fromis specified — earlier builder-stage registry.ci references are preserved.Behavioral impact: Multi-stage Dockerfiles that use registry.ci builder images (e.g., OCP builders) in intermediate stages and an external final base image (e.g., Red Hat UBI) will now have the registry.ci builder references correctly detected and wired as build inputs. Detection is also more robust across line continuations and image pulls inside RUN commands.
Tests: Added table-driven tests (pkg/dockerfile/inputs_test.go) covering:
podman pull(single-line and continued-line forms),Practical benefit: Reduces manual configuration and prevents accidental omission of required builder images from CI build inputs, improving reliability for multi-stage builds that mix internal builder images and external base images.