NO-JIRA: Align CI builder image with hypershift (openshift-4.23)#252
NO-JIRA: Align CI builder image with hypershift (openshift-4.23)#252jparrill wants to merge 1 commit intoopenshift:mainfrom
Conversation
Update the Dockerfile CI builder from openshift-4.22 to openshift-4.23 to stay aligned with the hypershift repository. Add integration tests to validate golang version consistency across all Dockerfiles and detect stale OCP version references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThe Docker build stage base image is updated from OpenShift 4.22 to 4.23. A new integration test suite validates Go version consistency across Dockerfiles and prevents introduction of stale OpenShift version references. ChangesBase Image and Dockerfile Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/builders/builders_test.go`:
- Around line 51-57: The test currently appends dockerfileInfo even when
extractGolangVersionsFromDockerfile(df.path) returns an empty versions slice,
allowing a file with no parsed Go versions to slip through; update the test loop
so after calling extractGolangVersionsFromDockerfile (the call using df.path and
df.name) you assert that the returned versions slice is non-empty (fail fast per
file) before appending to parsedFiles, e.g. use a Gomega Expect/Require on
len(versions) > 0 with a message referencing df.name to surface parser drift or
tag-format changes immediately; apply the same per-file guard for the other loop
block that handles files at lines 63-71.
- Around line 198-204: The helper function extractGolangVersionsFromDockerfile
currently ignores errors from file.Close(), which can hide IO failures; change
the function to use named return values (e.g., results []string, err error) and
replace defer file.Close() with a deferred closure that captures the close error
(if cerr := file.Close(); cerr != nil && err == nil { err = fmt.Errorf("close
failed: %w", cerr) }) so the close error is propagated while preserving any
existing error from the function.
- Around line 150-153: The tmpFile.Close() call currently ignores its returned
error; change it to capture and assert the result (e.g., err = tmpFile.Close();
g.Expect(err).NotTo(HaveOccurred()) or use a short variable like if cerr :=
tmpFile.Close(); g.Expect(cerr).NotTo(HaveOccurred())). Update the Close
invocation where tmpFile is used so any flush/I/O errors are checked and cause
the test to fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a20185c-4858-494c-9b83-f36709f49c8b
📒 Files selected for processing (2)
Dockerfiletests/integration/builders/builders_test.go
| versions, err := extractGolangVersionsFromDockerfile(df.path) | ||
| g.Expect(err).NotTo(HaveOccurred(), "Should parse %s without error", df.name) | ||
|
|
||
| parsedFiles = append(parsedFiles, dockerfileInfo{ | ||
| path: df.name, | ||
| golangVersions: versions, | ||
| }) |
There was a problem hiding this comment.
Add a per-Dockerfile guard for empty extracted Go versions.
Right now, consistency can still pass if one file contributes no extracted version at all. Please fail fast per file so parser drift or tag format changes are caught.
Suggested fix
versions, err := extractGolangVersionsFromDockerfile(df.path)
g.Expect(err).NotTo(HaveOccurred(), "Should parse %s without error", df.name)
+ g.Expect(versions).NotTo(BeEmpty(),
+ "%s should expose at least one golang version via FROM or #@follow_tag", df.name)
parsedFiles = append(parsedFiles, dockerfileInfo{
path: df.name,
golangVersions: versions,
})Also applies to: 63-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/builders/builders_test.go` around lines 51 - 57, The test
currently appends dockerfileInfo even when
extractGolangVersionsFromDockerfile(df.path) returns an empty versions slice,
allowing a file with no parsed Go versions to slip through; update the test loop
so after calling extractGolangVersionsFromDockerfile (the call using df.path and
df.name) you assert that the returned versions slice is non-empty (fail fast per
file) before appending to parsedFiles, e.g. use a Gomega Expect/Require on
len(versions) > 0 with a message referencing df.name to surface parser drift or
tag-format changes immediately; apply the same per-file guard for the other loop
block that handles files at lines 63-71.
| _, err = tmpFile.WriteString(tc.dockerfileContent) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| tmpFile.Close() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/integration/builders/builders_test.go | sed -n '145,160p'Repository: openshift/hypershift-oadp-plugin
Length of output: 599
🏁 Script executed:
rg -A 2 'WriteString' tests/integration/builders/builders_test.goRepository: openshift/hypershift-oadp-plugin
Length of output: 188
🏁 Script executed:
rg -A 2 'os.CreateTemp' tests/ | head -30Repository: openshift/hypershift-oadp-plugin
Length of output: 310
🏁 Script executed:
rg '\.Close\(\)' tests/ -A 1 | head -30Repository: openshift/hypershift-oadp-plugin
Length of output: 988
🏁 Script executed:
fd -e yml -e yaml | xargs rg -l 'errcheck|linter' 2>/dev/null | head -5Repository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
fd -e toml -e yaml -e yml | xargs cat 2>/dev/null | rg -i 'errcheck|lint' | head -20Repository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
find . -maxdepth 2 -name '.golangci.*' -o -name 'Makefile' -o -name '.github' | head -10Repository: openshift/hypershift-oadp-plugin
Length of output: 97
🏁 Script executed:
cat Makefile | rg -i 'lint|errcheck' -A 2 -B 2Repository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
find . -maxdepth 3 -name '.golangci*' -o -name 'golangci*'Repository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
cat -n go.mod | head -30Repository: openshift/hypershift-oadp-plugin
Length of output: 1400
🏁 Script executed:
ls -la | grep -i golangciRepository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
cat .github/workflows/* 2>/dev/null | rg -i 'errcheck|lint|golangci' | head -20Repository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
rg -i 'errcheck' . --type go | head -10Repository: openshift/hypershift-oadp-plugin
Length of output: 558
Check the tmpFile.Close() error explicitly.
Line 152 ignores a returned error from Close(); unchecked file close operations can mask flush and I/O failures.
Suggested fix
_, err = tmpFile.WriteString(tc.dockerfileContent)
g.Expect(err).NotTo(HaveOccurred())
- tmpFile.Close()
+ err = tmpFile.Close()
+ g.Expect(err).NotTo(HaveOccurred())📝 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.
| _, err = tmpFile.WriteString(tc.dockerfileContent) | |
| g.Expect(err).NotTo(HaveOccurred()) | |
| tmpFile.Close() | |
| _, err = tmpFile.WriteString(tc.dockerfileContent) | |
| g.Expect(err).NotTo(HaveOccurred()) | |
| err = tmpFile.Close() | |
| g.Expect(err).NotTo(HaveOccurred()) | |
🧰 Tools
🪛 golangci-lint (2.12.1)
[error] 152-152: Error return value of tmpFile.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/builders/builders_test.go` around lines 150 - 153, The
tmpFile.Close() call currently ignores its returned error; change it to capture
and assert the result (e.g., err = tmpFile.Close();
g.Expect(err).NotTo(HaveOccurred()) or use a short variable like if cerr :=
tmpFile.Close(); g.Expect(cerr).NotTo(HaveOccurred())). Update the Close
invocation where tmpFile is used so any flush/I/O errors are checked and cause
the test to fail.
| func extractGolangVersionsFromDockerfile(path string) ([]string, error) { | ||
| file, err := os.Open(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open %s: %w", path, err) | ||
| } | ||
| defer file.Close() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file to see the actual code
find . -type f -name "builders_test.go" -path "*/tests/integration/builders/*"Repository: openshift/hypershift-oadp-plugin
Length of output: 122
🏁 Script executed:
# Read the file around the mentioned lines to see the actual implementation
cat -n tests/integration/builders/builders_test.go | sed -n '195,225p'Repository: openshift/hypershift-oadp-plugin
Length of output: 1038
🏁 Script executed:
# Also search for the function name to ensure we're looking at the right location
rg "extractGolangVersionsFromDockerfile" -n -A 15 tests/integration/builders/builders_test.goRepository: openshift/hypershift-oadp-plugin
Length of output: 1800
Capture errors from deferred file.Close() in the helper function.
Line 203 drops close errors. This triggers errcheck and can hide IO failures (e.g., write buffer flushes). Use named return values with a deferred function wrapper to propagate close errors properly.
Suggested fix
-func extractGolangVersionsFromDockerfile(path string) ([]string, error) {
+func extractGolangVersionsFromDockerfile(path string) (versions []string, err error) {
file, err := os.Open(path)
if err != nil {
return nil, fmt.Errorf("failed to open %s: %w", path, err)
}
- defer file.Close()
+ defer func() {
+ if cerr := file.Close(); err == nil && cerr != nil {
+ err = fmt.Errorf("failed to close %s: %w", path, cerr)
+ }
+ }()
- var versions []string
scanner := bufio.NewScanner(file)
for scanner.Scan() {
line := strings.TrimSpace(scanner.Text())
if strings.HasPrefix(line, "#@follow_tag") || strings.HasPrefix(line, "FROM") {
matches := golangVersionPattern.FindStringSubmatch(line)
if len(matches) >= 2 {
versions = append(versions, matches[1])
}
}
}
- return versions, scanner.Err()
+ if scanErr := scanner.Err(); scanErr != nil {
+ return nil, scanErr
+ }
+ return versions, nil
}📝 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.
| func extractGolangVersionsFromDockerfile(path string) ([]string, error) { | |
| file, err := os.Open(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to open %s: %w", path, err) | |
| } | |
| defer file.Close() | |
| func extractGolangVersionsFromDockerfile(path string) (versions []string, err error) { | |
| file, err := os.Open(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to open %s: %w", path, err) | |
| } | |
| defer func() { | |
| if cerr := file.Close(); err == nil && cerr != nil { | |
| err = fmt.Errorf("failed to close %s: %w", path, cerr) | |
| } | |
| }() | |
| scanner := bufio.NewScanner(file) | |
| for scanner.Scan() { | |
| line := strings.TrimSpace(scanner.Text()) | |
| if strings.HasPrefix(line, "#@follow_tag") || strings.HasPrefix(line, "FROM") { | |
| matches := golangVersionPattern.FindStringSubmatch(line) | |
| if len(matches) >= 2 { | |
| versions = append(versions, matches[1]) | |
| } | |
| } | |
| } | |
| if scanErr := scanner.Err(); scanErr != nil { | |
| return nil, scanErr | |
| } | |
| return versions, nil | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.1)
[error] 203-203: Error return value of file.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/builders/builders_test.go` around lines 198 - 204, The
helper function extractGolangVersionsFromDockerfile currently ignores errors
from file.Close(), which can hide IO failures; change the function to use named
return values (e.g., results []string, err error) and replace defer file.Close()
with a deferred closure that captures the close error (if cerr := file.Close();
cerr != nil && err == nil { err = fmt.Errorf("close failed: %w", cerr) }) so the
close error is propagated while preserving any existing error from the function.
2 participants
Summary
openshift-4.22toopenshift-4.23to align with the hypershift repositoryDockerfile.oadpandkonflux.Dockerfilealready use brew-based images (rhel_9_golang_1.25) and require no changesTest plan
go build ./...passesgo test -race ./tests/integration/builders/...passes (14 test cases)openshift-4.23builder🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests