-
Notifications
You must be signed in to change notification settings - Fork 291
ci-operator: add automatic dockerfile inputs detection #4851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,7 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/openshift/ci-tools/pkg/api" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/openshift/ci-tools/pkg/api/configresolver" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/openshift/ci-tools/pkg/dockerfile" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/openshift/ci-tools/pkg/kubernetes" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/openshift/ci-tools/pkg/labeledclient" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/openshift/ci-tools/pkg/lease" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1037,9 +1038,130 @@ func runtimeStepConfigsForBuild( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buildSteps = append(buildSteps, getSourceStepsForJobSpec(jobSpec, injectedTest)...) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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...) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1041
to
+1047
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make Dockerfile-input detection best-effort instead of failing the build graph Right now any error from Given this feature is an additive convenience, it’s safer to degrade gracefully on detection errors and continue without auto-generated 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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return buildSteps, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // detectDockerfileInputs reads Dockerfiles for project images and detects registry.ci.openshift.org | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // references that should be added as base images | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func detectDockerfileInputs(config *api.ReleaseBuildConfiguration, readFile readFile) ([]api.StepConfiguration, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var steps []api.StepConfiguration | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i := range config.Images { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image := &config.Images[i] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why you need to do that here. Just do |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfileContent, dockerfilePath, err := readDockerfileForImage(image, readFile) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logrus.WithError(err).WithField("image", image.To).Debug("Failed to read Dockerfile for input detection, skipping") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if dockerfileContent == nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure how this will ever happen. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logrus.Infof("Detecting base images for \"%s\" from Dockerfile at \"%s\"", image.To, dockerfilePath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| imageSteps, err := processDetectedBaseImages(config, image, dockerfileContent, dockerfilePath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| steps = append(steps, imageSteps...) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return steps, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1041
to
+1075
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's examine the full context around the flagged lines
cd /repo 2>/dev/null || cd .
cat -n pkg/defaults/defaults.go | sed -n '1020,1100p'Repository: openshift/ci-tools Length of output: 3291 🏁 Script executed: # Search for the e2e test that validates this behavior
find . -type f -name "*.go" -path "*/e2e/*" | head -20Repository: openshift/ci-tools Length of output: 737 🏁 Script executed: # Look for test files related to Dockerfile input detection
fd "test" -e go | grep -i dockerfile | head -20Repository: openshift/ci-tools Length of output: 130 🏁 Script executed: # Search for references to image inputs and Dockerfile detection
rg "detectDockerfileInputs" -A 5 -B 5 --type goRepository: openshift/ci-tools Length of output: 1498 🏁 Script executed: # Examine the e2e test for dockerfile-inputs
cat -n test/e2e/dockerfile-inputs/e2e_test.goRepository: openshift/ci-tools Length of output: 3761 🏁 Script executed: # Look at the unit tests for inputs
cat -n pkg/dockerfile/inputs_test.goRepository: openshift/ci-tools Length of output: 7687 🏁 Script executed: # Search for input.As references in the codebase
rg "\.As" -B 3 -A 3 --type go | grep -i input | head -50Repository: openshift/ci-tools Length of output: 3393 🏁 Script executed: # Search for image.Inputs references
rg "image\.Inputs" --type go | head -20Repository: openshift/ci-tools Length of output: 1612 🏁 Script executed: # Look at the api.ProjectDirectoryImageBuildStepConfiguration struct
rg "type ProjectDirectoryImageBuildStepConfiguration" -A 30 --type goRepository: openshift/ci-tools Length of output: 1916 🏁 Script executed: # Find and read the e2e test config
find test/e2e/dockerfile-inputs -name "*.yaml" -o -name "config*"Repository: openshift/ci-tools Length of output: 109 🏁 Script executed: # Read the test config
cat -n test/e2e/dockerfile-inputs/testdata/config.yamlRepository: openshift/ci-tools Length of output: 2526 🏁 Script executed: # Find processDetectedBaseImages function
rg "func processDetectedBaseImages" -A 40 --type goRepository: openshift/ci-tools Length of output: 2501 🏁 Script executed: # Search for "Skipping Dockerfile inputs detection" message
rg "Skipping Dockerfile inputs detection" --type go -B 5 -A 5Repository: openshift/ci-tools Length of output: 2071 🏁 Script executed: # Search for the exact message expected by e2e test
rg "Skipping Dockerfile inputs detection: manual inputs defined" --type goRepository: openshift/ci-tools Length of output: 167 🏁 Script executed: # Let's check if there's any logic to skip entire image detection based on manual inputs
rg "manual.*input" --type go -i | grep -i skipRepository: openshift/ci-tools Length of output: 241 🏁 Script executed: # Look for where manual inputs are checked at the image level (not individual references)
rg "len.*Inputs.*>.*0" --type go -B 3 -A 3 | head -40Repository: openshift/ci-tools Length of output: 427 🏁 Script executed: # Check the detectDockerfileInputs function more carefully - does it have any early return logic?
cat -n pkg/defaults/defaults.go | sed -n '1053,1075p'Repository: openshift/ci-tools Length of output: 1030 🏁 Script executed: # Search for any condition checking if an image has inputs before processing
rg "if.*image\.Inputs" --type go -B 2 -A 2 | head -50Repository: openshift/ci-tools Length of output: 1295 🏁 Script executed: # Look at the unit test case "skip when manual inputs exist"
rg -A 30 "skip when manual inputs exist" pkg/defaults/defaults_test.goRepository: openshift/ci-tools Length of output: 1102 🏁 Script executed: # Get full context of defaults_test.go around this test case
grep -n "skip when manual inputs exist" pkg/defaults/defaults_test.goRepository: openshift/ci-tools Length of output: 110 🏁 Script executed: # Read the test to understand expected behavior
cat -n pkg/defaults/defaults_test.go | sed -n '1,50p'Repository: openshift/ci-tools Length of output: 1884 🏁 Script executed: # Find the test case in defaults_test.go
rg "\"skip when manual inputs exist\"" -A 50 --type goRepository: openshift/ci-tools Length of output: 3155 🏁 Script executed: # Let's search more broadly for "manual inputs defined"
rg "manual inputs defined" --type goRepository: openshift/ci-tools Length of output: 167 🏁 Script executed: # Let's look at which function the unit test is testing
grep -B 20 "skip when manual inputs exist" pkg/defaults/defaults_test.go | head -30Repository: openshift/ci-tools Length of output: 646 🏁 Script executed: # Find the test function name
cat -n pkg/defaults/defaults_test.go | sed -n '2430,2480p'Repository: openshift/ci-tools Length of output: 2037 🏁 Script executed: # Check if there's another version of detectDockerfileInputs or processDetectedBaseImages
rg "detectDockerfileInputs|processDetectedBaseImages" --type go -lRepository: openshift/ci-tools Length of output: 116 🏁 Script executed: # Find the test function that uses these test cases
cat -n pkg/defaults/defaults_test.go | sed -n '2400,2430p'Repository: openshift/ci-tools Length of output: 1283 🏁 Script executed: # Look for the function that processes these test cases
rg "for.*testCases|for.*tc.*:=" pkg/defaults/defaults_test.go -B 5 -A 2 | head -50Repository: openshift/ci-tools Length of output: 1377 🏁 Script executed: # Get the full test function name and see what it's testing
cat -n pkg/defaults/defaults_test.go | sed -n '2390,2410p'Repository: openshift/ci-tools Length of output: 911 🏁 Script executed: # Search for where the test cases are defined and what function they test
rg "testCases\s*:=\s*\[\]struct" -B 50 pkg/defaults/defaults_test.go | tail -60Repository: openshift/ci-tools Length of output: 1512 🏁 Script executed: # Look at processDetectedBaseImages more completely
cat -n pkg/defaults/defaults.go | sed -n '1102,1170p'Repository: openshift/ci-tools Length of output: 3192 🏁 Script executed: # Check if there's a guard at the beginning of processDetectedBaseImages
rg "func processDetectedBaseImages" -A 60 pkg/defaults/defaults.goRepository: openshift/ci-tools Length of output: 2316 🏁 Script executed: # Search for the "manual inputs defined" log message - it should be somewhere
rg "manual.*inputs.*defined" --type go -B 3 -A 3Repository: openshift/ci-tools Length of output: 934 🏁 Script executed: # Let me check if there's a different version or if the code was recently changed
git log --oneline pkg/defaults/defaults.go | head -10Repository: openshift/ci-tools Length of output: 103 🏁 Script executed: # Check the git log more carefully - when was the unit test added?
git log --oneline -p pkg/defaults/defaults_test.go | grep -B 5 -A 5 "skip when manual inputs exist" | head -30Repository: openshift/ci-tools Length of output: 461 🏁 Script executed: # Search for where "Skipping Dockerfile inputs detection: manual inputs defined" should be logged
rg "Skipping.*Dockerfile" --type goRepository: openshift/ci-tools Length of output: 303 🏁 Script executed: # Let's verify what the code currently does - does it skip the entire image or just individual references?
rg "if len.*image\.Inputs" --type go -B 3 -A 3Repository: openshift/ci-tools Length of output: 427 🏁 Script executed: # Check if detectDockerfileInputs has been recently modified
git log --oneline -p pkg/defaults/defaults.go | grep -A 30 "func detectDockerfileInputs" | head -50Repository: openshift/ci-tools Length of output: 1290 🏁 Script executed: # Let me check what the exact error message is when running tests
# Search for the test execution to see if it's skipped or something else
rg "manual.*inputs.*exist" pkg/defaults/defaults_test.go -B 5 -A 10Repository: openshift/ci-tools Length of output: 556 🏁 Script executed: # Let me check if there's a recent change to add this guard
git log -p pkg/defaults/defaults.go | grep -B 20 -A 20 "len.*Inputs.*> 0" | head -60Repository: openshift/ci-tools Length of output: 44 🏁 Script executed: # Check the git history for the e2e test to see when it was added
git log --oneline test/e2e/dockerfile-inputs/e2e_test.goRepository: openshift/ci-tools Length of output: 103 🏁 Script executed: # Look at the specific commit for the e2e test
git show de386a04a:test/e2e/dockerfile-inputs/e2e_test.go | grep -A 10 -B 10 "manual inputs defined"Repository: openshift/ci-tools Length of output: 1402 🏁 Script executed: # Check if detectDockerfileInputs should have a guard but doesn't
git log -p --all pkg/defaults/defaults.go | grep -B 5 -A 15 "len(image.Inputs) > 0" | head -80Repository: openshift/ci-tools Length of output: 44 Add guard to skip image detection when manual inputs are already defined. The code currently does not skip Dockerfile input detection for images that have manual inputs configured, but both the unit test and e2e test expect this behavior. When an image has if len(image.Inputs) > 0 {
logrus.Infof("Skipping Dockerfile inputs detection: manual inputs defined for \"%s\"", image.To)
continue
}This prevents auto-detection from overriding explicitly configured manual inputs. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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`
} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: lets remove that line. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if image.DockerfileLiteral != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return []byte(*image.DockerfileLiteral), "dockerfile_literal", nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfilePath := "Dockerfile" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if image.DockerfilePath != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfilePath = image.DockerfilePath | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if image.ContextDir != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block can be simplified as: dockerfilePath = fmt.Sprintf("./%s", dockerfilePath)
if image.ContextDir != "" {
dockerfilePath = fmt.Sprintf("%s/%s", image.ContextDir, dockerfilePath)
} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfilePath = fmt.Sprintf("%s/%s", image.ContextDir, dockerfilePath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfilePath = fmt.Sprintf("./%s", dockerfilePath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfileContent, err := readFile(dockerfilePath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, dockerfilePath, fmt.Errorf("failed to read Dockerfile at %s: %w", dockerfilePath, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return dockerfileContent, dockerfilePath, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1077
to
+1100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harden Dockerfile path handling (use filepath.Join + prevent This reads from a source checkout in PR contexts; config-controlled Proposed fix (join + clean)- dockerfilePath := "Dockerfile"
+ dockerfilePath := "Dockerfile"
if image.DockerfilePath != "" {
dockerfilePath = image.DockerfilePath
}
- if image.ContextDir != "" {
- dockerfilePath = fmt.Sprintf("%s/%s", image.ContextDir, dockerfilePath)
- } else {
- dockerfilePath = fmt.Sprintf("./%s", dockerfilePath)
- }
+ if image.ContextDir != "" {
+ dockerfilePath = filepath.Join(image.ContextDir, dockerfilePath)
+ }
+ dockerfilePath = filepath.Clean(dockerfilePath)
+ if strings.HasPrefix(dockerfilePath, ".."+string(filepath.Separator)) || dockerfilePath == ".." {
+ return nil, dockerfilePath, fmt.Errorf("invalid Dockerfile path (escapes context): %s", dockerfilePath)
+ }📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // processDetectedBaseImages detects base images from a Dockerfile and updates the configuration | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func processDetectedBaseImages( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's put all args in one line. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config *api.ReleaseBuildConfiguration, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image *api.ProjectDirectoryImageBuildStepConfiguration, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfileContent []byte, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfilePath string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) ([]api.StepConfiguration, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| detectedBaseImages, err := dockerfile.DetectInputsFromDockerfile(dockerfileContent, image.Inputs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, fmt.Errorf("failed to detect inputs from Dockerfile %s: %w", dockerfilePath, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(detectedBaseImages) == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if image.Inputs == nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image.Inputs = make(map[string]api.ImageBuildInputs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var steps []api.StepConfiguration | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for alias, baseImage := range detectedBaseImages { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if config.BaseImages == nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need to make this check on each iteration? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config.BaseImages = make(map[string]api.ImageStreamTagReference) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if presentAlias := isBaseImagePresent(config, baseImage); presentAlias != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image.Inputs[presentAlias] = api.ImageBuildInputs{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| As: []string{baseImage.As}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logrus.Infof("Detected base image for \"%s\" from \"%s\" matches existing base image \"%s\"", image.To, dockerfilePath, presentAlias) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of escaping the quotes in the message, it is idiomatic in the logrus logger to put that information under fields. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config.BaseImages[alias] = baseImage | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image.Inputs[alias] = api.ImageBuildInputs{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| As: []string{baseImage.As}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stepConfig := api.InputImageTagStepConfiguration{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| InputImage: api.InputImage{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BaseImage: defaultImageFromReleaseTag(alias, baseImage, config.ReleaseTagConfiguration), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| To: api.PipelineImageStreamTagReference(alias), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sources: []api.ImageStreamSource{{SourceType: api.ImageStreamSourceBase, Name: alias}}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| steps = append(steps, api.StepConfiguration{InputImageTagStepConfiguration: &stepConfig}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logrus.Infof("Detected base image for \"%s\" from \"%s\": will tag into pipeline:%s", image.To, dockerfilePath, alias) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return steps, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func isBaseImagePresent(config *api.ReleaseBuildConfiguration, baseImage api.ImageStreamTagReference) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for presentAlias, presentBaseImage := range config.BaseImages { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if baseImage.Name == presentBaseImage.Name && baseImage.Namespace == presentBaseImage.Namespace && baseImage.Tag == presentBaseImage.Tag { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return presentAlias | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func getSourceStepsForJobSpec(jobSpec *api.JobSpec, injectedTest bool) []api.StepConfiguration { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var sourceSteps []api.StepConfiguration | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| primaryRef := determinePrimaryRef(jobSpec, injectedTest) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detection errors should not fail the build graph
Per earlier feedback,
detectDockerfileInputserrors 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
🤖 Prompt for AI Agents