ci-operator: add automatic dockerfile inputs detection - #4851
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughIntroduces Dockerfile-based input detection by creating new parsing utilities for extracting registry references and parsing pull strings, integrating them into build step generation, and refactoring registry-replacer to use shared utilities with comprehensive test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed Comment |
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 |
|
/test images |
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.
|
/test images |
f2792f4 to
eede1fe
Compare
|
/test e2e |
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 |
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.
|
/test e2e |
4c6b6d5 to
275363c
Compare
|
/test e2e |
275363c to
966e16b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/dockerfile/inputs_test.go:
- Around line 185-190: Update the test case name string "wrong reguistry format"
to the correct spelling "wrong registry format" in the table-driven test entry
(the struct literal with fields name, pullString, expected, expectError) so the
test description is accurate; search for the entry containing pullString
"registry.ci.openshift.org/ocp:latest" or the name value to locate and fix it.
🧹 Nitpick comments (2)
pkg/dockerfile/inputs.go (1)
75-78: Dockerfile instructions are case-insensitive; consider normalizingThe check filters lines containing
FROM,COPY, orcopy, but Dockerfile instructions are case-insensitive. A line likefrom registry.ci...orFrom registry.ci...would be skipped.♻️ Suggested 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 }pkg/defaults/defaults.go (1)
1065-1068: Inconsistent error handling: consider making processing failures best-effort tooRead failures at lines 1056-1059 are gracefully skipped with a debug log, but processing failures here are fatal. For consistency with the best-effort approach, consider logging a warning and continuing instead.
♻️ Suggested fix
imageSteps, err := processDetectedBaseImages(config, image, dockerfileContent, dockerfilePath) if err != nil { - return nil, err + logrus.WithError(err).WithField("image", image.To).Warn("Failed to process detected base images, skipping") + 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 as they are similar to previous changes (2)
- pkg/defaults/defaults_test.go
- 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/e2e_test.gopkg/dockerfile/inputs.gopkg/defaults/defaults.gopkg/dockerfile/inputs_test.go
🧬 Code graph analysis (3)
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(2786-2796)ImageStreamTagReference(499-506)
pkg/defaults/defaults.go (2)
pkg/api/types.go (8)
StepConfiguration(623-637)ImageBuildInputs(2786-2796)ImageStreamTagReference(499-506)InputImageTagStepConfiguration(643-646)InputImage(685-692)PipelineImageStreamTagReference(2531-2531)ImageStreamSource(704-707)ImageStreamSourceBase(698-698)pkg/dockerfile/inputs.go (1)
DetectInputsFromDockerfile(34-68)
🔇 Additional comments (7)
pkg/dockerfile/inputs.go (2)
108-129: LGTM!The parsing logic correctly handles both standard registry references and the special quay-proxy format with underscore-encoded tags.
148-187: The function is actively used in production code atcmd/registry-replacer/main.go:305, so no action is needed here.Likely an incorrect or invalid review comment.
pkg/defaults/defaults.go (2)
1075-1096: LGTM!The function structure is clean with an early return for the literal case, and the path construction logic correctly handles the various configuration options.
1119-1147: LGTM!The logic correctly handles duplicate detection via
isBaseImagePresent, adds new base images to the configuration, and creates appropriateInputImageTagStepConfigurationentries. The informative logging will help users understand what's being auto-detected.test/e2e/dockerfile-inputs/e2e_test.go (1)
21-34: LGTM!Good coverage of the expected detection scenarios including single/multiple references, quay-proxy format, manual input skipping, and existing base image matching.
pkg/dockerfile/inputs_test.go (2)
11-167: LGTM!Comprehensive test coverage including empty dockerfile, single/multiple registry references, quay-proxy variants, manual replacement skipping, non-registry references, mixed scenarios, and duplicate handling.
226-293: LGTM!Good test coverage for the replacement candidates extraction, including multi-stage builds and COPY --from scenarios.
966e16b to
041eb39
Compare
|
/test e2e |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @pkg/defaults/defaults.go:
- Around line 1128-1134: When isBaseImagePresent returns a presentAlias but the
matched presentBaseImage has an empty As, avoid inserting an empty string into
image.Inputs[presentAlias].As; update the block that assigns
image.Inputs[presentAlias] so it checks presentBaseImage.As and if empty uses
presentAlias (or otherwise omits the As slice), e.g. compute a non-empty asValue
= presentBaseImage.As != "" ? presentBaseImage.As : presentAlias and set
ImageBuildInputs.As to []string{asValue}; alternatively add a validation step in
config loading to ensure BaseImages always have As populated.
In @pkg/dockerfile/inputs_test.go:
- Around line 185-190: Update the test case name string to correct the typo:
change the "name" field value in the failing test case from "wrong reguistry
format" to "wrong registry format" in the test table entry (the struct literal
with pullString "registry.ci.openshift.org/ocp:latest" and expectError true) so
the test description is spelled correctly.
🧹 Nitpick comments (1)
pkg/defaults/defaults.go (1)
1084-1092: Consider consistent path formatting.The path construction prepends
./when there's no ContextDir (line 1091), but doesn't for paths with ContextDir (line 1089). This inconsistency is minor but could cause confusion in log messages.♻️ Suggested fix for consistency
if image.ContextDir != "" { dockerfilePath = fmt.Sprintf("%s/%s", image.ContextDir, dockerfilePath) - } else { - dockerfilePath = fmt.Sprintf("./%s", dockerfilePath) }
📜 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 (7)
pkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/dockerfile/inputs.gopkg/dockerfile/inputs_test.gotest/e2e/dockerfile-inputs/e2e_test.gotest/e2e/dockerfile-inputs/testdata/config.yamltest/e2e/dockerfile-inputs/testdata/real.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/dockerfile-inputs/e2e_test.go
- pkg/dockerfile/inputs.go
- 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/testdata/config.yamlpkg/defaults/defaults.gotest/e2e/dockerfile-inputs/testdata/real.yamlpkg/dockerfile/inputs_test.go
🧬 Code graph analysis (2)
pkg/defaults/defaults.go (2)
pkg/api/types.go (7)
StepConfiguration(623-637)ImageBuildInputs(2792-2802)ImageStreamTagReference(499-506)InputImageTagStepConfiguration(643-646)InputImage(685-692)ImageStreamSource(704-707)ImageStreamSourceBase(698-698)pkg/dockerfile/inputs.go (1)
DetectInputsFromDockerfile(34-68)
pkg/dockerfile/inputs_test.go (2)
pkg/api/types.go (2)
ImageBuildInputs(2792-2802)ImageStreamTagReference(499-506)pkg/dockerfile/inputs.go (1)
DetectInputsFromDockerfile(34-68)
🔇 Additional comments (7)
pkg/defaults/defaults.go (3)
1041-1047: LGTM on integration point.The integration of Dockerfile input detection is cleanly placed after source steps and before returning build steps. Error handling properly wraps and propagates failures.
1051-1075: LGTM on detection orchestration.The function appropriately handles read failures gracefully by logging at debug level and continuing. This ensures the auto-detection feature doesn't break builds when Dockerfiles are unavailable or malformed.
1156-1163: LGTM on base image presence check.The comparison correctly uses the image identity fields (Namespace, Name, Tag) while excluding
Aswhich is just an alias.pkg/dockerfile/inputs_test.go (2)
11-167: Solid test coverage for DetectInputsFromDockerfile.The test cases comprehensively cover the main scenarios including empty input, single/multiple references, different registry formats (registry.ci.openshift.org, quay-proxy, registry.svc.ci.openshift.org), manual replacement skipping, and deduplication.
226-293: LGTM on extraction tests.Good coverage of FROM instruction parsing including multi-stage builds and COPY --from scenarios. The set membership verification approach is appropriate for unordered results.
test/e2e/dockerfile-inputs/testdata/real.yaml (1)
1-30: LGTM on realistic test configuration.This configuration provides a real-world example that exercises the build pipeline with
from_repository: true, external base images, and container-based tests. Good addition for e2e validation.test/e2e/dockerfile-inputs/testdata/config.yaml (1)
1-71: Excellent test configuration coverage.This config comprehensively tests the Dockerfile input detection feature including:
- Single and multiple registry references
- quay-proxy format handling
- Manual input precedence (
manual-inputscase)- COPY --from scenarios
- Non-registry references (should be ignored)
- Existing base image detection
The
detect-existing-base-imagecase (lines 59-62) correctly tests against theosbase image defined in lines 2-5, ensuring the duplicate detection logic works.
041eb39 to
36e9780
Compare
284953f to
dba6f04
Compare
|
/test e2e |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/defaults/defaults.go`:
- Around line 1096-1137: In processDetectedBaseImages, avoid clobbering
preconfigured image.Inputs and silently overwriting base image aliases: when
assigning image.Inputs[alias], merge the new As value into any existing
api.ImageBuildInputs (preserving existing Paths and other fields) instead of
replacing the struct; before writing (*baseImages)[alias] check for collisions
by comparing the existing baseImage reference and if different, compute a unique
alias (e.g., alias-2, incrementing suffix until unused), normalize the stored
baseImage.As to match the chosen alias, log the collision with both aliases and
chosen suffix, then write the unique alias into both *baseImages and
image.Inputs; update uses of alias (stepConfig To/Sources and logs) to use the
final resolved alias.
♻️ Duplicate comments (2)
test/e2e/dockerfile-inputs/e2e_test.go (1)
47-49: Split “unexpected success” vs “unexpected failure” error messages.Current failure message is misleading when
success == falsebut the command succeeds (and vice versa).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)) + }pkg/defaults/defaults.go (1)
1072-1094: Harden Dockerfile path handling (prevent..escape / absolute paths; usefilepath.Join). This currently allows traversal viaContextDir/DockerfilePath.Proposed fix
func readDockerfileForImage(image *api.ProjectDirectoryImageBuildStepConfiguration, readFile readFile) (dockerfileDetails, error) { if image.DockerfileLiteral != nil { return dockerfileDetails{content: []byte(*image.DockerfileLiteral), path: "dockerfile_literal"}, nil } details := dockerfileDetails{path: "Dockerfile"} if image.DockerfilePath != "" { details.path = image.DockerfilePath } - dockerfilePath := fmt.Sprintf("./%s", details.path) - if image.ContextDir != "" { - dockerfilePath = fmt.Sprintf("%s/%s", image.ContextDir, details.path) - } - details.path = dockerfilePath + dockerfilePath := filepath.Join(".", details.path) + if image.ContextDir != "" { + dockerfilePath = filepath.Join(image.ContextDir, details.path) + } + dockerfilePath = filepath.Clean(dockerfilePath) + if filepath.IsAbs(dockerfilePath) || + strings.HasPrefix(dockerfilePath, ".."+string(filepath.Separator)) || dockerfilePath == ".." { + return dockerfileDetails{}, fmt.Errorf("invalid Dockerfile path (escapes repo): %s", dockerfilePath) + } + details.path = dockerfilePath dockerfileContent, err := readFile(details.path) if err != nil { return dockerfileDetails{}, fmt.Errorf("failed to read Dockerfile at %s: %w", dockerfilePath, err) } details.content = dockerfileContent return details, nil }
🧹 Nitpick comments (2)
test/e2e/dockerfile-inputs/e2e_test.go (2)
13-13: Move the huge inlinefakeJobSpecto testdata to reduce churn and improve readability.Inlining the full JOB_SPEC JSON makes future updates noisy and hard to review. Consider placing it in
testdata/job_spec.jsonand reading it in the test (keeping only the minimal fields ci-operator actually requires).Proposed refactor (requires adding
testdata/job_spec.json)import ( + "os" "testing" "github.com/openshift/ci-tools/test/e2e/framework" ) func TestDockerfileInputs(t *testing.T) { - const fakeJobSpec = `{"type":"postsubmit","job":"branch-ci-openshift-ci-tools-master-ci-operator-e2e","buildid":"0","prowjobid":"uuid","refs":{"org":"openshift","repo":"ci-tools","base_ref":"master","base_sha":"6d231cc37652e85e0f0e25c21088b73d644d89ad","pulls":[]},"decoration_config":{"timeout":"4h0m0s","grace_period":"30m0s","utility_images":{"clonerefs":"quay-proxy.ci.openshift.org/openshift/ci:ci_clonerefs_latest","initupload":"quay-proxy.ci.openshift.org/openshift/ci:ci_initupload_latest","entrypoint":"quay-proxy.ci.openshift.org/openshift/ci:ci_entrypoint_latest","sidecar":"quay-proxy.ci.openshift.org/openshift/ci:ci_sidecar_latest"},"resources":{"clonerefs":{"limits":{"memory":"3Gi"},"requests":{"cpu":"100m","memory":"500Mi"}},"initupload":{"limits":{"memory":"200Mi"},"requests":{"cpu":"100m","memory":"50Mi"}},"place_entrypoint":{"limits":{"memory":"100Mi"},"requests":{"cpu":"100m","memory":"25Mi"}},"sidecar":{"limits":{"memory":"2Gi"},"requests":{"cpu":"100m","memory":"250Mi"}}},"gcs_configuration":{"bucket":"test-platform-results","path_strategy":"single","default_org":"openshift","default_repo":"origin","mediaTypes":{"log":"text/plain"}},"gcs_credentials_secret":"gce-sa-credentials-gcs-publisher"}}` + fakeJobSpecBytes, err := os.ReadFile("testdata/job_spec.json") + if err != nil { + t.Fatalf("failed to read testdata/job_spec.json: %v", err) + } + fakeJobSpec := string(fakeJobSpecBytes) var testCases = []struct { name string args []string configPath string jobSpec string
28-36: Output assertions are likely brittle across log/message or release bumps; consider relaxing the match.These exact strings (including specific OCP versions/tags) may need frequent updates even when behavior is correct. If
framework.VerboseOutputContainssupports it, prefer asserting on fewer/stable substrings (e.g.,Detected base image, will tag into pipeline:) and the specific “manual inputs defined” behavior, instead of enumerating every detected tag.
📜 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 (4)
pkg/defaults/defaults.gopkg/defaults/defaults_test.gotest/e2e/dockerfile-inputs/e2e_test.gotest/e2e/dockerfile-inputs/testdata/config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/dockerfile-inputs/testdata/config.yaml
- 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.gopkg/defaults/defaults.go
🧬 Code graph analysis (1)
test/e2e/dockerfile-inputs/e2e_test.go (1)
pkg/testhelper/accessory.go (1)
T(51-57)
🔇 Additional comments (4)
pkg/defaults/defaults.go (4)
33-36: dockerfile import looks fine.
1039-1045: Nice integration point for auto-detected Dockerfile inputs. The detection is additive and doesn’t appear to fail graph construction.
1048-1070: Index-based loop is the right choice (mutations persist). Good call passing&images[i]soimage.Inputsupdates stick.
1139-1146: Helper looks correct for matching by (namespace, name, tag).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/retest-required |
dba6f04 to
7379518
Compare
|
/test e2e |
7379518 to
e97062d
Compare
|
/retest-required |
|
/test e2e |
e97062d to
70931eb
Compare
|
/test e2e |
|
/retest-required |
|
/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) (dockerfileDetails, error) { | ||
| if image.DockerfileLiteral != nil { |
There was a problem hiding this comment.
This check should be done outside this function since you are utilizing directly the image.DockerfileLiteral
There was a problem hiding this comment.
I think the code is cleaner now. This is not better:
var dockerfileDetails dockerfileDetails
if image.DockerfileLiteral != nil {
dockerfileDetails = dockerfileDetails{content: []byte(*image.DockerfileLiteral), path: "dockerfile_literal"}
} else {
dockerfileDetails, err = readDockerfileForImage(&images[i], readFile)
}
readDockerfileForImage is a single purpose function that reads the dockerfile, regardless if it is literal or no
70931eb to
81eef5e
Compare
|
@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. |
| continue | ||
| } | ||
| imageSteps, image := processDetectedBaseImages(baseImages, image, dockerfileDetails) | ||
| images[i] = image |
There was a problem hiding this comment.
You are mutating the list directly here, which can be a problem in general. In Go, it's idiomatic to construct a new list with the new data that you want to mutate and return it. In this case, this can be trivial, but this is food for thought.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droslean, jmguzik, 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 |
|
/retest-required |
|
/test e2e |
|
/override ci/prow/integration-optional-test |
|
@Prucek: Overrode contexts on behalf of Prucek: ci/prow/integration-optional-test 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. |
7a2f189
into
openshift:main
5 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)Real example config using a mocked prowjob: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-api-operator/63/prucek-test-dockerfile-inputs/2009259226555224064