Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

FROM registry.ci.openshift.org/openshift/release:rhel-9-release-golang-1.25-openshift-4.22 AS build
FROM registry.ci.openshift.org/openshift/release:rhel-9-release-golang-1.25-openshift-4.23 AS build
WORKDIR /go/src/github.com/openshift/hypershift-oadp-plugin
COPY . .
RUN CGO_ENABLED=0 go build -o /go/bin/hypershift-oadp-plugin .
Expand Down
258 changes: 258 additions & 0 deletions tests/integration/builders/builders_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,258 @@
package builders

import (
"bufio"
"fmt"
"os"
"path/filepath"
"regexp"
"strings"
"testing"

. "github.com/onsi/gomega"
)

var golangVersionPattern = regexp.MustCompile(`golang[_-]([\d.]+)`)

// staleOCPVersions lists OCP versions that should no longer appear in builder images.
// Update this list when adopting a newer OCP release.
var staleOCPVersions = []string{"4.21", "4.22"}

type dockerfileInfo struct {
path string
golangVersions []string
}

func TestBuilderImages(t *testing.T) {
g := NewWithT(t)

rootDir, err := findProjectRoot()
g.Expect(err).NotTo(HaveOccurred())

t.Run("When checking all Dockerfiles it should use a consistent golang version", func(t *testing.T) {
g := NewWithT(t)

dockerfiles := []struct {
name string
path string
}{
{name: "Dockerfile", path: filepath.Join(rootDir, "Dockerfile")},
{name: "Dockerfile.oadp", path: filepath.Join(rootDir, "Dockerfile.oadp")},
{name: "konflux.Dockerfile", path: filepath.Join(rootDir, "konflux.Dockerfile")},
}

var parsedFiles []dockerfileInfo
for _, df := range dockerfiles {
if _, err := os.Stat(df.path); os.IsNotExist(err) {
t.Logf("Skipping %s (not found)", df.name)
continue
}

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,
})
Comment on lines +51 to +57
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

t.Logf("%s uses golang versions: %v", df.name, versions)
}

g.Expect(parsedFiles).NotTo(BeEmpty(), "Should find at least one Dockerfile")

var allVersions []string
for _, f := range parsedFiles {
allVersions = append(allVersions, f.golangVersions...)
}

uniqueVersions := uniqueStrings(allVersions)
g.Expect(uniqueVersions).To(HaveLen(1),
"All Dockerfiles should use the same golang version, but found %v across files: %s",
uniqueVersions, formatFileVersions(parsedFiles))
})

t.Run("When checking builder image references it should not use stale OCP versions", func(t *testing.T) {
g := NewWithT(t)

dockerfilePath := filepath.Join(rootDir, "Dockerfile")
content, err := os.ReadFile(dockerfilePath)
g.Expect(err).NotTo(HaveOccurred())

for _, staleVersion := range staleOCPVersions {
staleVersion := staleVersion
absent := "openshift-" + staleVersion
t.Run(fmt.Sprintf("When Dockerfile references %s it should fail because it is stale", absent), func(t *testing.T) {
g := NewWithT(t)
g.Expect(string(content)).NotTo(ContainSubstring(absent),
"Dockerfile should not reference %s", absent)
})
}
})
}

func TestExtractGolangVersions(t *testing.T) {
tests := []struct {
name string
dockerfileContent string
expectedVersions []string
}{
{
name: "When Dockerfile has a single FROM with golang version it should extract it",
dockerfileContent: `FROM registry.ci.openshift.org/openshift/release:rhel-9-release-golang-1.25-openshift-4.23 AS build
WORKDIR /app
RUN go build .`,
expectedVersions: []string{"1.25"},
},
{
name: "When Dockerfile has follow_tag and FROM with golang it should extract both",
dockerfileContent: `#@follow_tag(registry-proxy.engineering.redhat.com/rh-osbs/openshift-golang-builder:rhel_9_golang_1.25)
FROM brew.registry.redhat.io/rh-osbs/openshift-golang-builder:rhel_9_golang_1.25 AS builder
COPY . /workspace
FROM registry.redhat.io/ubi9/ubi-minimal:latest`,
expectedVersions: []string{"1.25", "1.25"},
},
{
name: "When Dockerfile has mismatched golang versions it should extract all of them",
dockerfileContent: `FROM registry.ci.openshift.org/openshift/release:rhel-9-release-golang-1.25-openshift-4.23 AS build
FROM brew.registry.redhat.io/rh-osbs/openshift-golang-builder:rhel_9_golang_1.26 AS builder2`,
expectedVersions: []string{"1.25", "1.26"},
},
{
name: "When Dockerfile has no golang builder images it should return empty",
dockerfileContent: `FROM registry.access.redhat.com/ubi9-minimal
RUN mkdir /plugins
USER 65532:65532`,
expectedVersions: nil,
},
{
name: "When Dockerfile has only comments and no FROM lines it should return empty",
dockerfileContent: `# This is a comment
# Another comment
WORKDIR /app`,
expectedVersions: nil,
},
{
name: "When Dockerfile has golang in a RUN line it should not extract it",
dockerfileContent: `FROM registry.access.redhat.com/ubi9-minimal
RUN dnf install -y golang-1.25
WORKDIR /app`,
expectedVersions: nil,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

tmpFile, err := os.CreateTemp(t.TempDir(), "Dockerfile-*")
g.Expect(err).NotTo(HaveOccurred())

_, err = tmpFile.WriteString(tc.dockerfileContent)
g.Expect(err).NotTo(HaveOccurred())
tmpFile.Close()

Comment on lines +150 to +153
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.go

Repository: openshift/hypershift-oadp-plugin

Length of output: 188


🏁 Script executed:

rg -A 2 'os.CreateTemp' tests/ | head -30

Repository: openshift/hypershift-oadp-plugin

Length of output: 310


🏁 Script executed:

rg '\.Close\(\)' tests/ -A 1 | head -30

Repository: openshift/hypershift-oadp-plugin

Length of output: 988


🏁 Script executed:

fd -e yml -e yaml | xargs rg -l 'errcheck|linter' 2>/dev/null | head -5

Repository: 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 -20

Repository: openshift/hypershift-oadp-plugin

Length of output: 58


🏁 Script executed:

find . -maxdepth 2 -name '.golangci.*' -o -name 'Makefile' -o -name '.github' | head -10

Repository: openshift/hypershift-oadp-plugin

Length of output: 97


🏁 Script executed:

cat Makefile | rg -i 'lint|errcheck' -A 2 -B 2

Repository: 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 -30

Repository: openshift/hypershift-oadp-plugin

Length of output: 1400


🏁 Script executed:

ls -la | grep -i golangci

Repository: openshift/hypershift-oadp-plugin

Length of output: 58


🏁 Script executed:

cat .github/workflows/* 2>/dev/null | rg -i 'errcheck|lint|golangci' | head -20

Repository: openshift/hypershift-oadp-plugin

Length of output: 58


🏁 Script executed:

rg -i 'errcheck' . --type go | head -10

Repository: 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.

Suggested change
_, 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.

versions, err := extractGolangVersionsFromDockerfile(tmpFile.Name())
g.Expect(err).NotTo(HaveOccurred())
g.Expect(versions).To(Equal(tc.expectedVersions))
})
}
}

func TestVersionConsistencyDetection(t *testing.T) {
tests := []struct {
name string
versions []string
expectedUnique int
}{
{
name: "When all versions match it should report one unique version",
versions: []string{"1.25", "1.25", "1.25"},
expectedUnique: 1,
},
{
name: "When versions differ it should report multiple unique versions",
versions: []string{"1.25", "1.26", "1.25"},
expectedUnique: 2,
},
{
name: "When all versions are different it should report all as unique",
versions: []string{"1.24", "1.25", "1.26"},
expectedUnique: 3,
},
{
name: "When there is a single version it should report one unique",
versions: []string{"1.25"},
expectedUnique: 1,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)
unique := uniqueStrings(tc.versions)
g.Expect(unique).To(HaveLen(tc.expectedUnique))
})
}
}

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()

Comment on lines +198 to +204
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.go

Repository: 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.

Suggested change
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.

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()
}

func uniqueStrings(s []string) []string {
seen := make(map[string]bool)
var result []string
for _, v := range s {
if !seen[v] {
seen[v] = true
result = append(result, v)
}
}
return result
}

func formatFileVersions(files []dockerfileInfo) string {
var parts []string
for _, f := range files {
parts = append(parts, fmt.Sprintf("%s=%v", f.path, f.golangVersions))
}
return strings.Join(parts, ", ")
}

func findProjectRoot() (string, error) {
dir, err := os.Getwd()
if err != nil {
return "", err
}

for {
if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
return dir, nil
}
parent := filepath.Dir(dir)
if parent == dir {
break
}
dir = parent
}

return "", fmt.Errorf("go.mod not found")
}