ci-operator: add automatic dockerfile inputs detection #4851
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThe changes add automatic Dockerfile input detection to the build system. New functions detect base image references from Dockerfiles, parse them into ImageStreamTagReference entries, and integrate them into build step configuration. Production logic, unit tests, and end-to-end tests are included. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes ✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prucek 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 |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/defaults/defaults_test.go (1)
2445-2457: Prefer standard librarystrings.Contains.These helper functions reimplement functionality available in the standard library. Using
strings.Containsdirectly would simplify the code and improve maintainability.Apply this diff:
-func containsString(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || - (len(s) > 0 && len(substr) > 0 && contains(s, substr))) -} - -func contains(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}Then update line 2237 to use
strings.Containsdirectly:+import "strings" + if tc.expectedErrContain != "" && err != nil { - if !containsString(err.Error(), tc.expectedErrContain) { + if !strings.Contains(err.Error(), tc.expectedErrContain) { t.Errorf("expected error to contain %q, got: %v", tc.expectedErrContain, err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
pkg/defaults/defaults.go(2 hunks)pkg/defaults/defaults_test.go(3 hunks)pkg/dockerfile/inputs.go(1 hunks)pkg/dockerfile/inputs_test.go(1 hunks)test/e2e/dockerfile-inputs/config.yaml(1 hunks)test/e2e/dockerfile-inputs/e2e_test.go(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.copy-from(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.manual(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.multiple(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.no-refs(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.quay-proxy(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.single(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/dockerfile-inputs/config.yamltest/e2e/dockerfile-inputs/testdata/Dockerfile.quay-proxytest/e2e/dockerfile-inputs/testdata/Dockerfile.manualtest/e2e/dockerfile-inputs/testdata/Dockerfile.multipletest/e2e/dockerfile-inputs/testdata/Dockerfile.copy-fromtest/e2e/dockerfile-inputs/testdata/Dockerfile.no-refspkg/dockerfile/inputs_test.gopkg/dockerfile/inputs.gopkg/defaults/defaults_test.gopkg/defaults/defaults.gotest/e2e/dockerfile-inputs/e2e_test.gotest/e2e/dockerfile-inputs/testdata/Dockerfile.single
🧬 Code graph analysis (4)
pkg/dockerfile/inputs_test.go (2)
pkg/api/types.go (2)
ImageBuildInputs(2748-2758)ImageStreamTagReference(494-501)pkg/dockerfile/inputs.go (1)
DetectInputsFromDockerfile(33-90)
pkg/dockerfile/inputs.go (1)
pkg/api/types.go (2)
ImageBuildInputs(2748-2758)ImageStreamTagReference(494-501)
pkg/defaults/defaults.go (3)
pkg/api/types.go (9)
ReleaseBuildConfiguration(37-135)StepConfiguration(618-632)ProjectDirectoryImageBuildStepConfiguration(2661-2684)ImageBuildInputs(2748-2758)ImageStreamTagReference(494-501)InputImageTagStepConfiguration(638-641)InputImage(680-687)PipelineImageStreamTagReference(2493-2493)ImageStreamSource(699-702)pkg/dockerfile/inputs.go (1)
DetectInputsFromDockerfile(33-90)cmd/repo-init/api.go (1)
BaseImages(69-69)
test/e2e/dockerfile-inputs/e2e_test.go (1)
pkg/testhelper/accessory.go (1)
T(51-57)
🔇 Additional comments (15)
test/e2e/dockerfile-inputs/testdata/Dockerfile.no-refs (1)
1-2: LGTM!Appropriate test fixture for verifying the "no registry references" detection path.
test/e2e/dockerfile-inputs/testdata/Dockerfile.manual (1)
1-2: LGTM!Appropriate fixture for testing manual inputs precedence over auto-detection.
test/e2e/dockerfile-inputs/testdata/Dockerfile.quay-proxy (1)
1-2: LGTM!Appropriate test fixture for verifying quay-proxy reference detection and parsing.
test/e2e/dockerfile-inputs/testdata/Dockerfile.single (1)
1-2: LGTM!Appropriate fixture for the basic single-reference auto-detection scenario.
test/e2e/dockerfile-inputs/testdata/Dockerfile.multiple (1)
1-6: LGTM!Appropriate multi-stage fixture for testing detection of multiple base image references.
test/e2e/dockerfile-inputs/testdata/Dockerfile.copy-from (1)
1-6: LGTM!Appropriate fixture for testing COPY --from directive handling in multi-stage builds.
pkg/defaults/defaults_test.go (2)
7-7: LGTM!The new imports are appropriately used in the test functions.
Also applies to: 20-20
2138-2443: Comprehensive test coverage.The test functions provide thorough coverage of Dockerfile reading and base image detection scenarios, including edge cases for custom paths, literals, manual inputs, and multi-stage builds.
test/e2e/dockerfile-inputs/config.yaml (1)
1-30: LGTM!Well-structured e2e test configuration that covers the key scenarios: single/multiple auto-detection, quay-proxy references, manual inputs precedence, COPY --from handling, and no-registry-references cases.
pkg/defaults/defaults.go (1)
1029-1137: LGTM: Clean integration of Dockerfile input detection.The implementation follows good practices:
- Soft-fail approach (logs and continues) for individual image failures is appropriate, as one malformed Dockerfile shouldn't block the entire build
- Functions are well-factored and focused on single responsibilities
- Error handling is consistent with the rest of the codebase
- Mutations to
config.BaseImagesandimage.Inputsfollow existing patterns in this filetest/e2e/dockerfile-inputs/e2e_test.go (1)
12-90: LGTM: Comprehensive e2e test coverage.The test cases cover the key scenarios:
- Auto-detection of single and multiple registry references
- Quay-proxy reference handling
- Manual input override behavior
- COPY --from references
- No registry references case
Test structure is clean and maintainable.
pkg/dockerfile/inputs_test.go (1)
11-293: LGTM: Thorough unit test coverage.The test suite covers:
- Edge cases (empty dockerfile, duplicates, mixed registry/non-registry refs)
- Multiple registry formats (registry.ci, registry.svc.ci, quay-proxy)
- Error conditions (malformed references)
- Manual input override behavior
Good use of table-driven tests and clear assertions.
pkg/dockerfile/inputs.go (3)
18-19: Regex pattern is permissive but acceptable.The pattern
\S+matches any non-whitespace characters after the registry domain, which is quite permissive. However, this is acceptable because:
orgRepoTagFromPullString()validates the format downstream (Lines 138-162)- Invalid formats are caught and returned as errors
- The simple approach aligns with the stated goal of parity with registry-replacer
33-90: LGTM: Well-structured detection logic.The function correctly:
- Returns early for empty input or when manual inputs are defined
- Deduplicates registry references
- Handles parsing errors appropriately
- Logs useful diagnostic information at appropriate levels
The approach of honoring manual
inputs.as[]configuration while auto-detecting when not present is the right design choice.
181-220: Incorrect review comment. The functionextractReplacementCandidatesFromDockerfileatpkg/dockerfile/inputs.go:183IS actively called in the codebase atcmd/registry-replacer/main.go:305. It is not dead code or unused. The function serves its intended purpose within the codebase.Likely an incorrect or invalid review comment.
|
/test e2e |
|
|
||
| // readDockerfileForImage reads the Dockerfile content for a given image configuration | ||
| // Returns content, a description of the source, and any error | ||
| func readDockerfileForImage(image *api.ProjectDirectoryImageBuildStepConfiguration, readFile readFile) ([]byte, string, error) { |
There was a problem hiding this comment.
The nesting levels of this body can be decreased by doing so:
func readDockerfileForImage(image *api.ProjectDirectoryImageBuildStepConfiguration, readFile readFile) ([]byte, string, error) {
if image.DockerfileLiteral != nil {
return []byte(*image.DockerfileLiteral), "dockerfile_literal"
}
// Everything else here, no `else`
}
|
/test images |
Prucek
force-pushed
the
dockerfile-inputs
branch
from
fdaf457 to
f2792f4
Compare
|
/test e2e |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/dockerfile/inputs.go (1)
93-101: Clarify or remove currently-unused per-reference manual override helper
hasManualInputsshort-circuits auto-detection when anyinputs.asentry is present, which matches the intended “manual config wins” semantics. Given that, the per-reference helperhasManualReplacementForis currently unreachable in production: with the existing call pattern it can only run in cases where there are no manualinputs.asentries.That’s not harmful but slightly confusing for future readers. Two options:
- If you plan to support a “mixed” mode (some refs auto-detected, some overridden manually), add a comment on
DetectInputsFromDockerfileexplaining the intended future use ofhasManualReplacementFor, or adjust the short-circuit logic accordingly.- If not, consider dropping
hasManualReplacementForfor now and reintroducing it when such a mode is actually needed.This keeps the detection path easier to reason about and avoids dead-code-like helpers.
Also applies to: 129-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
pkg/defaults/defaults.go(2 hunks)pkg/defaults/defaults_test.go(3 hunks)pkg/dockerfile/inputs.go(1 hunks)pkg/dockerfile/inputs_test.go(1 hunks)test/e2e/dockerfile-inputs/config.yaml(1 hunks)test/e2e/dockerfile-inputs/e2e_test.go(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.copy-from(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.manual(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.multiple(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.no-refs(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.quay-proxy(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.single(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- test/e2e/dockerfile-inputs/testdata/Dockerfile.quay-proxy
- test/e2e/dockerfile-inputs/testdata/Dockerfile.manual
- test/e2e/dockerfile-inputs/testdata/Dockerfile.single
- pkg/defaults/defaults_test.go
- test/e2e/dockerfile-inputs/testdata/Dockerfile.copy-from
- test/e2e/dockerfile-inputs/testdata/Dockerfile.multiple
- test/e2e/dockerfile-inputs/config.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/dockerfile-inputs/testdata/Dockerfile.no-refstest/e2e/dockerfile-inputs/e2e_test.gopkg/defaults/defaults.gopkg/dockerfile/inputs.gopkg/dockerfile/inputs_test.go
🧬 Code graph analysis (2)
test/e2e/dockerfile-inputs/e2e_test.go (1)
pkg/testhelper/accessory.go (1)
T(51-57)
pkg/dockerfile/inputs.go (1)
pkg/api/types.go (2)
ImageBuildInputs(2748-2758)ImageStreamTagReference(494-501)
🔇 Additional comments (3)
test/e2e/dockerfile-inputs/testdata/Dockerfile.no-refs (1)
1-2: No-registry fixture correctly exercises negative pathThis Dockerfile intentionally contains no
registry.ci.openshift.org/quay-proxyrefs and is appropriate for the "no auto-detect" scenario.test/e2e/dockerfile-inputs/e2e_test.go (1)
1-90: E2E coverage for Dockerfile inputs is comprehensive and wired correctlyThe table-driven
TestDockerfileInputsexercises all key scenarios (single/multiple/quay-proxy/manual/COPY-from/no-refs) via the standard e2e framework, passing pull secrets andJOB_SPECappropriately. Assertions on success and log output look sound.pkg/dockerfile/inputs_test.go (1)
1-293: Unit tests thoroughly exercise detection and parsing helpers
TestDetectInputsFromDockerfile,TestOrgRepoTagFromPullString, andTestExtractReplacementCandidatesFromDockerfilecollectively cover the important happy-path and edge cases (multiple registries, quay-proxy, legacyregistry.svc, manual inputs, mixed/duplicate refs, and COPY/AS stages). This gives good confidence in the new parsing logic.
| // Detect Dockerfile inputs for project images and add InputImageTagStepConfiguration entries | ||
| dockerfileInputSteps, err := detectDockerfileInputs(config, readFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to detect Dockerfile inputs: %w", err) | ||
| } | ||
| buildSteps = append(buildSteps, dockerfileInputSteps...) | ||
|
|
There was a problem hiding this comment.
Make Dockerfile-input detection best-effort instead of failing the build graph
Right now any error from detectDockerfileInputs (e.g., an unexpected registry reference that DetectInputsFromDockerfile cannot parse) will cause runtimeStepConfigsForBuild to return an error and abort graph construction. That’s a behavior change compared to today, where unusual but syntactically valid Dockerfiles still run, just without auto-detected inputs.
Given this feature is an additive convenience, it’s safer to degrade gracefully on detection errors and continue without auto-generated InputImageTagStepConfigurations. Otherwise, a single odd-looking reference to registry.ci.openshift.org/quay-proxy.ci.openshift.org can break the entire job.
Consider handling errors like this instead:
- // Detect Dockerfile inputs for project images and add InputImageTagStepConfiguration entries
- dockerfileInputSteps, err := detectDockerfileInputs(config, readFile)
- if err != nil {
- return nil, fmt.Errorf("failed to detect Dockerfile inputs: %w", err)
- }
- buildSteps = append(buildSteps, dockerfileInputSteps...)
+ // Detect Dockerfile inputs for project images and add InputImageTagStepConfiguration entries.
+ // This is best-effort and should not break existing configurations.
+ dockerfileInputSteps, err := detectDockerfileInputs(config, readFile)
+ if err != nil {
+ logrus.WithError(err).Warn("Dockerfile inputs detection failed; continuing without auto-detected base images")
+ } else {
+ buildSteps = append(buildSteps, dockerfileInputSteps...)
+ }This keeps the new functionality while avoiding regressions for configs with unusual but valid image references.
Also applies to: 1042-1131
🤖 Prompt for AI Agents
In pkg/defaults/defaults.go around lines 1029 to 1035 (and similarly for
1042–1131), the call to detectDockerfileInputs currently returns an error that
aborts graph construction; make detection best‑effort by catching and logging
the error instead of returning it. Change the code to call
detectDockerfileInputs, and if it returns an error, write a warning-level log or
comment (including the error message) and continue without appending
dockerfileInputSteps; only return on fatal/unexpected errors, not on
parse/registry detection failures. Ensure the same non-fatal behavior is applied
to the other block(s) noted (1042–1131).
| func DetectInputsFromDockerfile( | ||
| dockerfile []byte, | ||
| existingInputs map[string]api.ImageBuildInputs, | ||
| ) (map[string]api.ImageStreamTagReference, error) { | ||
| if len(dockerfile) == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Avoid failing on unparseable registry refs; treat them as best-effort skips
DetectInputsFromDockerfile currently returns an error if orgRepoTagFromPullString cannot parse a matched registry reference (e.g., if the path doesn’t look like exactly /<org>/<repo>:<tag>). Combined with the caller in runtimeStepConfigsForBuild, this means:
- Any Dockerfile line that happens to match
registryRegexbut uses a slightly different yet syntactically valid shape (extra path segment, missing explicit tag, future quay-proxy variant, etc.) can cause the entire job to fail, even though auto-detection is an additive feature. - The repo already has explicit unit tests for error cases in
orgRepoTagFromPullString; those shouldn’t necessarily translate into hard failures for ci-operator.
To keep the feature robust while still surfacing problems, I’d recommend treating parse failures as “skip this ref with a warning” instead of aborting:
for _, ref := range registryRefs {
- // Skip if there's already a manual input for this reference
+ // Skip if there's already a manual input for this reference
if hasManualReplacementFor(existingInputs, ref) {
logrus.WithField("reference", ref).Debug("Skipping auto-detection: manual replacement exists")
continue
}
- // Parse the registry reference into org/repo/tag
- orgRepoTag, err := orgRepoTagFromPullString(ref)
- if err != nil {
- return nil, fmt.Errorf("failed to parse registry reference %s: %w", ref, err)
- }
+ // Parse the registry reference into org/repo/tag. If the shape is unexpected,
+ // log and skip instead of failing the whole job.
+ orgRepoTag, err := orgRepoTagFromPullString(ref)
+ if err != nil {
+ logrus.WithError(err).Warnf("Dockerfile-inputs: skipping unrecognized registry reference %q", ref)
+ continue
+ }With this change (and making the caller in runtimeStepConfigsForBuild best-effort as suggested earlier), detection cannot regress existing jobs simply because of a new or slightly different registry path pattern, while still emitting useful diagnostics when something looks off.
Also applies to: 56-67, 139-163
🤖 Prompt for AI Agents
In pkg/dockerfile/inputs.go around lines 34-40 (and similarly at 56-67 and
139-163), the function currently treats failures from orgRepoTagFromPullString
as fatal errors; change that behavior to be best-effort: when
orgRepoTagFromPullString returns an error, emit a warning (or debug log)
describing the unparseable ref and continue without adding that ref to the
result map instead of returning the error, so DetectInputsFromDockerfile skips
problematic registry refs but keeps processing the rest of the Dockerfile.
|
/test images |
Prucek
force-pushed
the
dockerfile-inputs
branch
from
f2792f4 to
eede1fe
Compare
|
/test e2e |
Prucek
force-pushed
the
dockerfile-inputs
branch
from
eede1fe to
b1d622c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
test/e2e/dockerfile-inputs/config.yaml(1 hunks)test/e2e/dockerfile-inputs/e2e_test.go(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.copy-from(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.manual(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.multiple(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.no-refs(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.quay-proxy(1 hunks)test/e2e/dockerfile-inputs/testdata/Dockerfile.single(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/e2e/dockerfile-inputs/testdata/Dockerfile.manual
🚧 Files skipped from review as they are similar to previous changes (5)
- test/e2e/dockerfile-inputs/testdata/Dockerfile.quay-proxy
- test/e2e/dockerfile-inputs/config.yaml
- test/e2e/dockerfile-inputs/testdata/Dockerfile.single
- test/e2e/dockerfile-inputs/testdata/Dockerfile.multiple
- test/e2e/dockerfile-inputs/testdata/Dockerfile.copy-from
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/dockerfile-inputs/testdata/Dockerfile.no-refstest/e2e/dockerfile-inputs/e2e_test.go
🧬 Code graph analysis (1)
test/e2e/dockerfile-inputs/e2e_test.go (1)
pkg/testhelper/accessory.go (1)
T(51-57)
🔇 Additional comments (3)
test/e2e/dockerfile-inputs/testdata/Dockerfile.no-refs (1)
1-2: LGTM!Valid test fixture for the no-registry-references case. The Dockerfile correctly uses a public base image and explicitly documents the test intent.
test/e2e/dockerfile-inputs/e2e_test.go (2)
1-13: LGTM!Standard e2e test structure with appropriate build tags and minimal imports. The defaultJobSpec is lengthy but acceptable as test fixture data.
15-73: LGTM!Comprehensive test coverage with clear table-driven structure. The test cases appropriately cover various scenarios including single/multiple references, quay-proxy, manual overrides, COPY --from, and the no-references case.
|
/test e2e |
Prucek
force-pushed
the
dockerfile-inputs
branch
from
b1d622c to
4c6b6d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @pkg/defaults/defaults.go:
- Around line 1039-1044: The call to detectDockerfileInputs currently propagates
errors and aborts graph construction; change it to treat errors as non-fatal:
call detectDockerfileInputs(config, readFile), and if it returns an error log a
warning (using the package's logger) that includes the error message and
context, do not return the error, and only append returned dockerfileInputSteps
to buildSteps when non-nil/non-empty so auto-detection stays additive rather
than failing the build graph.
In @pkg/dockerfile/inputs.go:
- Around line 58-61: The code currently treats a failure from
orgRepoTagFromPullString(ref) as fatal; change this to a best-effort approach by
catching the error, emitting a warning (using the existing logger in scope) that
includes ref and the error, and skipping/continuing past this ref instead of
returning the error so processing can continue for other entries; make this
change around the orgRepoTagFromPullString call and ensure any downstream code
that expects orgRepoTag is not executed when parsing fails.
🧹 Nitpick comments (1)
pkg/dockerfile/inputs.go (1)
96-99: Inconsistent case handling for Dockerfile instructionsDockerfile instructions are case-insensitive. The check includes lowercase
copybut not lowercasefrom, which could miss valid references.🔎 Proposed fix
for _, line := range bytes.Split(dockerfile, []byte("\n")) { - if !bytes.Contains(line, []byte("FROM")) && !bytes.Contains(line, []byte("COPY")) && !bytes.Contains(line, []byte("copy")) { + upperLine := bytes.ToUpper(line) + if !bytes.Contains(upperLine, []byte("FROM")) && !bytes.Contains(upperLine, []byte("COPY")) { continue }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
pkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/dockerfile/inputs.gopkg/dockerfile/inputs_test.gotest/e2e/dockerfile-inputs/config.yamltest/e2e/dockerfile-inputs/e2e_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/dockerfile/inputs_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/defaults/defaults_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/dockerfile-inputs/e2e_test.gotest/e2e/dockerfile-inputs/config.yamlpkg/defaults/defaults.gopkg/dockerfile/inputs.go
🧬 Code graph analysis (3)
test/e2e/dockerfile-inputs/e2e_test.go (1)
pkg/testhelper/accessory.go (1)
T(51-57)
pkg/defaults/defaults.go (2)
pkg/api/types.go (7)
StepConfiguration(623-637)ProjectDirectoryImageBuildStepConfiguration(2699-2722)ImageBuildInputs(2786-2796)ImageStreamTagReference(499-506)InputImageTagStepConfiguration(643-646)InputImage(685-692)ReleaseTagConfiguration(516-532)pkg/dockerfile/inputs.go (1)
DetectInputsFromDockerfile(34-79)
pkg/dockerfile/inputs.go (1)
pkg/api/types.go (2)
ImageBuildInputs(2786-2796)ImageStreamTagReference(499-506)
🔇 Additional comments (4)
pkg/defaults/defaults.go (1)
1098-1141: LGTM!The function correctly detects base images, updates the configuration, and generates appropriate
InputImageTagStepConfigurationsteps. The mutation ofconfig.BaseImagesandimage.Inputsis intentional for proper pipeline integration.test/e2e/dockerfile-inputs/config.yaml (1)
1-67: LGTM!The test configuration provides good coverage of Dockerfile input detection scenarios: single/multiple registry references, quay-proxy format, manual inputs override, COPY --from handling, and no-registry-refs case.
test/e2e/dockerfile-inputs/e2e_test.go (1)
12-100: LGTM!The table-driven test structure provides comprehensive coverage of the Dockerfile input detection feature. Test cases align well with the scenarios defined in
config.yaml.pkg/dockerfile/inputs.go (1)
169-208: These functions are actively used in the codebase.extractReplacementCandidatesFromDockerfileis called inpkg/dockerfile/inputs_test.go:270andcmd/registry-replacer/main.go:305, whilenodeHasFromRefis used incmd/registry-replacer/main.go:565and within the extraction function itself at line 196.Likely an incorrect or invalid review comment.
| // Detect Dockerfile inputs for project images and add InputImageTagStepConfiguration entries | ||
| dockerfileInputSteps, err := detectDockerfileInputs(config, readFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to detect Dockerfile inputs: %w", err) | ||
| } | ||
| buildSteps = append(buildSteps, dockerfileInputSteps...) |
There was a problem hiding this comment.
Detection errors should not fail the build graph
Per earlier feedback, detectDockerfileInputs errors should be logged as warnings, not propagated as fatal errors. This keeps the auto-detection feature additive without breaking existing configurations.
🔎 Proposed fix
// Detect Dockerfile inputs for project images and add InputImageTagStepConfiguration entries
dockerfileInputSteps, err := detectDockerfileInputs(config, readFile)
if err != nil {
- return nil, fmt.Errorf("failed to detect Dockerfile inputs: %w", err)
+ logrus.WithError(err).Warn("Dockerfile inputs detection failed; continuing without auto-detected base images")
+ } else {
+ buildSteps = append(buildSteps, dockerfileInputSteps...)
}
- buildSteps = append(buildSteps, dockerfileInputSteps...)📝 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.
| // Detect Dockerfile inputs for project images and add InputImageTagStepConfiguration entries | |
| dockerfileInputSteps, err := detectDockerfileInputs(config, readFile) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to detect Dockerfile inputs: %w", err) | |
| } | |
| buildSteps = append(buildSteps, dockerfileInputSteps...) | |
| // Detect Dockerfile inputs for project images and add InputImageTagStepConfiguration entries | |
| dockerfileInputSteps, err := detectDockerfileInputs(config, readFile) | |
| if err != nil { | |
| logrus.WithError(err).Warn("Dockerfile inputs detection failed; continuing without auto-detected base images") | |
| } else { | |
| buildSteps = append(buildSteps, dockerfileInputSteps...) | |
| } |
🤖 Prompt for AI Agents
In @pkg/defaults/defaults.go around lines 1039 - 1044, The call to
detectDockerfileInputs currently propagates errors and aborts graph
construction; change it to treat errors as non-fatal: call
detectDockerfileInputs(config, readFile), and if it returns an error log a
warning (using the package's logger) that includes the error message and
context, do not return the error, and only append returned dockerfileInputSteps
to buildSteps when non-nil/non-empty so auto-detection stays additive rather
than failing the build graph.
| orgRepoTag, err := orgRepoTagFromPullString(ref) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse registry reference %s: %w", ref, err) | ||
| } |
There was a problem hiding this comment.
Parse errors should be best-effort, not fatal
This hard failure on parse errors can break jobs when encountering unexpected but valid registry reference formats. Per earlier review feedback, consider logging a warning and skipping instead of returning an error.
🔎 Proposed fix
orgRepoTag, err := orgRepoTagFromPullString(ref)
if err != nil {
- return nil, fmt.Errorf("failed to parse registry reference %s: %w", ref, err)
+ logrus.WithError(err).Warnf("Dockerfile-inputs: skipping unrecognized registry reference %q", ref)
+ continue
}📝 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.
| orgRepoTag, err := orgRepoTagFromPullString(ref) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse registry reference %s: %w", ref, err) | |
| } | |
| orgRepoTag, err := orgRepoTagFromPullString(ref) | |
| if err != nil { | |
| logrus.WithError(err).Warnf("Dockerfile-inputs: skipping unrecognized registry reference %q", ref) | |
| continue | |
| } |
🤖 Prompt for AI Agents
In @pkg/dockerfile/inputs.go around lines 58 - 61, The code currently treats a
failure from orgRepoTagFromPullString(ref) as fatal; change this to a
best-effort approach by catching the error, emitting a warning (using the
existing logger in scope) that includes ref and the error, and
skipping/continuing past this ref instead of returning the error so processing
can continue for other entries; make this change around the
orgRepoTagFromPullString call and ensure any downstream code that expects
orgRepoTag is not executed when parsing fails.
| if testCase.success != (err == nil) { | ||
| t.Fatalf("%s: didn't expect an error from ci-operator: %v; output:\n%v", testCase.name, err, string(output)) | ||
| } |
There was a problem hiding this comment.
Clarify error messages for both failure modes
The error message is misleading when testCase.success is false but the command succeeds. Per earlier feedback, split into distinct messages for unexpected success vs. unexpected failure.
🔎 Proposed fix
- if testCase.success != (err == nil) {
- t.Fatalf("%s: didn't expect an error from ci-operator: %v; output:\n%v", testCase.name, err, string(output))
- }
+ if testCase.success && err != nil {
+ t.Fatalf("%s: expected success but got error: %v; output:\n%v", testCase.name, err, string(output))
+ } else if !testCase.success && err == nil {
+ t.Fatalf("%s: expected failure but command succeeded; output:\n%v", testCase.name, string(output))
+ }📝 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.
| if testCase.success != (err == nil) { | |
| t.Fatalf("%s: didn't expect an error from ci-operator: %v; output:\n%v", testCase.name, err, string(output)) | |
| } | |
| if testCase.success && err != nil { | |
| t.Fatalf("%s: expected success but got error: %v; output:\n%v", testCase.name, err, string(output)) | |
| } else if !testCase.success && err == nil { | |
| t.Fatalf("%s: expected failure but command succeeded; output:\n%v", testCase.name, string(output)) | |
| } |
|
/test e2e |
|
@Prucek: The following test failed, say
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. |
4 participants
Added
pkg/dockerfile/inputs.go- Core detection logic:registry.ci.openshift.organdquay-proxy.ci.openshift.orgreferences in Dockerfilesorg/repo/tagcomponentsbase_imagesandinputs.as[]configuration on-the-flyIntegrated into
pkg/defaults/defaults.go:detectDockerfileInputs()function reads Dockerfiles from source checkoutDetectInputsFromDockerfile()during build graph constructioninputs.as[]takes precedence)