chore: add script to update argocd dependencies and images #1877
Conversation
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
2 times, most recently
from
37bc3ba to
82325f7
Compare
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
2 times, most recently
from
770c447 to
7842460
Compare
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
7842460 to
b4fa01d
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a Makefile target and Go-based automation to update Argo CD dependencies, a CI workflow job that runs that target and verifies no unexpected diffs, a background CI helper script, and supporting docs and utilities to perform the dependency upgrade. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Make as Makefile
participant Runner as run.sh
participant Updater as updater (main.go)
participant ArgoCD as argo-cd repo
participant Repo as operator repo
Note over Make,Updater: Triggered by CI job or manual `make update-dependencies`
Dev->>Make: make update-dependencies
Make->>Runner: exec hack/update-dependencies-script/run.sh
Runner->>Updater: go run .
Updater->>Make: read ARGO_CD_TARGET_VERSION
Updater->>ArgoCD: git clone --branch <version>
Updater->>Repo: update images (defaults.go, Dockerfile, tests)
Updater->>Repo: copy selected CRD YAMLs
Updater->>Repo: copy go.mod replace block & run go mod tidy
Updater->>Repo: regenerate manifests/bundle
Updater-->>Dev: print validation guidance / exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
config/crd/bases/argoproj.io_applications.yaml (1)
5890-5890: Formatting-only change LGTM; consider explicit status subresource for consistency.No functional change detected (trailing newline;
subresources: {}unchanged). If aligning with ApplicationSet CRD and controller behavior, you may want to explicitly enable the status subresource here as well.Optional tweak:
- subresources: {} + subresources: + status: {}Please confirm whether upstream Argo CD’s Application CRD currently declares
statusas a subresource; if it does, mirroring it here avoids mixed spec/status updates and eases RBAC separation.hack/update-dependencies-script/go.mod (2)
1-3: Consider aligning Go version with the main project.The script uses
go 1.20, while the main project's go.mod specifiesgo 1.24.6. This version mismatch could lead to compatibility issues or inconsistent behavior.Apply this diff to align the Go version:
module github.com/argoproj-labs/argocd-operator/dependency-upgrade -go 1.20 +go 1.24.6
6-6: Consider using a more recent go-github version.The script uses
github.com/google/go-github/v58, while the main project uses v69 and v72. Although this may work fine for the script's needs, using a more aligned version could prevent potential API incompatibilities.hack/update-dependencies-script/README.md (1)
10-12: Add language identifiers to fenced code blocks.Markdown best practice is to specify the language for syntax highlighting.
Apply this diff:
-``` +```makefile ARGO_CD_TARGET_VERSION ?= 3.1.8Then run the script:
-+bash
make update-dependenciesAlso applies to: 15-17
Makefile (1)
373-377: Consider quoting the script path.The script path should be quoted to handle edge cases with spaces or special characters in paths.
Apply this diff:
.PHONY: update-dependencies update-dependencies: - "hack/update-dependencies-script/run.sh" + ./hack/update-dependencies-script/run.shAlternatively, if you prefer the quoted form, ensure it's properly quoted:
- "hack/update-dependencies-script/run.sh" + ./hack/update-dependencies-script/run.shNote: The
./prefix is conventional in Makefiles for relative script paths and doesn't require quotes. If you keep the quoted form, it works but the./prefix is more idiomatic.hack/update-dependencies-script/utils.go (1)
53-63: Remove unreachable return statement.After
exitWithErrorcallsos.Exit(1), the return on line 58 is never reached.Apply this diff:
func stripImagePrefix(line string) string { line = strings.TrimSpace(line) if !strings.HasPrefix(line, "image:") { exitWithError(fmt.Errorf("unexpected image format on line: %s", line)) - return "" } return strings.TrimPrefix(line, "image: ") - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumhack/update-dependencies-script/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.github/workflows/lint.yaml(2 hunks)Makefile(2 hunks)build/util/Dockerfile(1 hunks)common/defaults.go(2 hunks)config/crd/bases/argoproj.io_applications.yaml(1 hunks)config/crd/bases/argoproj.io_applicationsets.yaml(1 hunks)config/crd/bases/argoproj.io_appprojects.yaml(1 hunks)controllers/argocd/applicationset_test.go(2 hunks)controllers/argocd/dex_test.go(1 hunks)go.mod(2 hunks)hack/update-dependencies-script/README.md(1 hunks)hack/update-dependencies-script/go.mod(1 hunks)hack/update-dependencies-script/main.go(1 hunks)hack/update-dependencies-script/run.sh(1 hunks)hack/update-dependencies-script/utils.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/argocd/applicationset_test.go (1)
common/defaults.go (1)
ArgoCDDefaultArgoVersion(73-73)
🪛 checkmake (0.2.2)
Makefile
[warning] 375-375: Missing required phony target "all"
(minphony)
[warning] 375-375: Missing required phony target "clean"
(minphony)
[warning] 375-375: Missing required phony target "test"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
hack/update-dependencies-script/README.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
hack/update-dependencies-script/run.sh
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build
- GitHub Check: build-operator
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Verify Argo CD has updated dependencies, for the given target version
🔇 Additional comments (11)
go.mod (2)
6-6: LGTM! Argo CD dependency updated to v3.1.9.The version bump aligns with the PR objectives to upgrade Argo CD dependencies.
178-178: Good documentation practice.The comment clearly indicates the origin of the replace block, making it easier to trace and maintain.
config/crd/bases/argoproj.io_appprojects.yaml (1)
366-366: LGTM! Formatting adjustment only.This change appears to be an end-of-file newline adjustment with no semantic impact.
controllers/argocd/dex_test.go (1)
586-586: Good documentation practice.The inline comment clearly indicates that this image value is managed by the dependency update script, helping maintainers understand the maintenance workflow.
config/crd/bases/argoproj.io_applicationsets.yaml (1)
17724-17724: Enable status subresource for ApplicationSet CRD.The addition of the empty
status: {}subresource declaration enables the status subresource for the ApplicationSet CRD, which is a standard Kubernetes feature allowing the status field to be updated independently from the spec. This change aligns with the ArgoCD v3.1.9 dependency upgrade.Please confirm that this change is an intentional part of the ArgoCD v3.1.9 upgrade and that similar updates have been applied to other CRDs as needed.
common/defaults.go (1)
73-73: LGTM! Version constants updated correctly.The Argo CD and Redis image digests have been updated appropriately with inline version comments for traceability.
Also applies to: 194-194, 197-197
build/util/Dockerfile (1)
1-2: LGTM! Dockerfile base image updated correctly.The Argo CD base image has been updated to v3.1.9 with the correct digest, consistent with the version constant in common/defaults.go.
.github/workflows/lint.yaml (1)
1-1: LGTM! CI validation for dependency updates is well-designed.The new verification job ensures that dependency updates remain consistent with the target version by running the update script and checking for diffs. The
--ignore-matching-linesforcreatedAtis appropriate for handling generated timestamps.Also applies to: 32-43
controllers/argocd/applicationset_test.go (1)
454-454: LGTM! Tests now use centralized version constant.Refactoring the tests to use
common.ArgoCDDefaultArgoVersioninstead of hardcoded digests improves maintainability and ensures tests automatically reflect version updates.Also applies to: 1484-1484, 1505-1505
Makefile (1)
8-13: LGTM! Well-documented version tracking variable.The
ARGO_CD_TARGET_VERSIONvariable is clearly documented with helpful notes about usage and format requirements. This provides a clear entry point for dependency updates.hack/update-dependencies-script/utils.go (1)
81-96: Note: Order is not preserved during deduplication.The function uses a map for deduplication, which means the output order is non-deterministic due to Go's map iteration randomization. If order preservation is required, consider using a slice-based approach with a map for tracking seen elements.
If order preservation is important for your use case, consider this alternative:
func removeDuplicateLines(in []string) []string { seen := make(map[string]bool) var res []string for _, inVal := range in { if !seen[inVal] { seen[inVal] = true res = append(res, inVal) } } return res }
| // Replace the argocd image references in the Dockerfile with new version | ||
| if strings.HasPrefix(lines[idx], "# Argo CD v") && strings.HasPrefix(lines[idx+1], "FROM quay.io/argoproj/argocd@sha256") { | ||
| newContent += "# Argo CD " + argocdContainerImage.version + "\n" | ||
| newContent += "FROM quay.io/argoproj/argocd@" + argocdContainerImage.sha256Digest + " as argocd\n" | ||
| idx++ // Skip to the line after these 2 | ||
| match = true | ||
| } else { | ||
| newContent += lines[idx] + "\n" | ||
| } | ||
| } |
There was a problem hiding this comment.
Apply the same bounds check when updating build/util/Dockerfile
This loop also dereferences lines[idx+1] without verifying that the next element exists; hitting the end of the slice will panic. Add the same guard before accessing the next line.
- for idx := 0; idx < len(lines); idx++ {
- // Replace the argocd image references in the Dockerfile with new version
- if strings.HasPrefix(lines[idx], "# Argo CD v") && strings.HasPrefix(lines[idx+1], "FROM quay.io/argoproj/argocd@sha256") {
+ for idx := 0; idx < len(lines); idx++ {
+ if idx+1 >= len(lines) {
+ newContent += lines[idx] + "\n"
+ continue
+ }
+ // Replace the argocd image references in the Dockerfile with new version
+ if strings.HasPrefix(lines[idx], "# Argo CD v") && strings.HasPrefix(lines[idx+1], "FROM quay.io/argoproj/argocd@sha256") {
newContent += "# Argo CD " + argocdContainerImage.version + "\n"
newContent += "FROM quay.io/argoproj/argocd@" + argocdContainerImage.sha256Digest + " as argocd\n"
idx++ // Skip to the line after these 2
match = true
- } else {
- newContent += lines[idx] + "\n"
- }
+ continue
+ }
+ newContent += lines[idx] + "\n"
}🤖 Prompt for AI Agents
In hack/update-dependencies-script/main.go around lines 165 to 174, the loop
checks lines[idx+1] without ensuring the next element exists which can panic at
the end of the slice; add a bounds check (if idx+1 < len(lines)) before
accessing lines[idx+1], only perform the two-line prefix checks and the idx++
increment when that guard passes, and otherwise fall back to the existing else
path (append the current line and continue) so the code never dereferences
beyond the slice.
| replaceBlockFromArgoCDLines, replaceBlockStart, replaceBlockEnd := readReplaceBlockFromGoMod(argoCDRepoRootGoModPath) | ||
| replaceBlockFromArgoCD := replaceBlockFromArgoCDLines[replaceBlockStart:replaceBlockEnd] | ||
|
|
||
| argocdOperatorGoModPath := filepath.Join(argocdOperatorRoot, "go.mod") | ||
|
|
||
| argocdOperatorLines, argocdOperatorReplaceStart, argocdOperatorReplaceEnd := readReplaceBlockFromGoMod(argocdOperatorGoModPath) | ||
|
|
||
| newArgoCDOperatorGoModFileContents := "" | ||
|
|
||
| for x, line := range argocdOperatorLines { | ||
| if x == argocdOperatorReplaceStart { | ||
|
|
||
| newArgoCDOperatorGoModFileContents += "\t// This replace block is from Argo CD " + targetArgoCDVersion + " go.mod\n" | ||
|
|
||
| // inject the replace block from argocd's go.mod | ||
| for _, replaceBlockFromArgoCDLine := range replaceBlockFromArgoCD { | ||
| newArgoCDOperatorGoModFileContents += replaceBlockFromArgoCDLine + "\n" | ||
| } | ||
|
|
||
| } else if x > argocdOperatorReplaceStart && x < argocdOperatorReplaceEnd { |
There was a problem hiding this comment.
Copy the entire replace block (do not drop the last line)
readReplaceBlockFromGoMod returns indices inclusive of the block, but slicing with replaceBlockEnd as the upper bound excludes the final replace entry. The generated go.mod therefore loses the last replace line. Include replaceBlockEnd+1 in the slice.
- replaceBlockFromArgoCD := replaceBlockFromArgoCDLines[replaceBlockStart:replaceBlockEnd]
+ replaceBlockFromArgoCD := replaceBlockFromArgoCDLines[replaceBlockStart : replaceBlockEnd+1]📝 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.
| replaceBlockFromArgoCDLines, replaceBlockStart, replaceBlockEnd := readReplaceBlockFromGoMod(argoCDRepoRootGoModPath) | |
| replaceBlockFromArgoCD := replaceBlockFromArgoCDLines[replaceBlockStart:replaceBlockEnd] | |
| argocdOperatorGoModPath := filepath.Join(argocdOperatorRoot, "go.mod") | |
| argocdOperatorLines, argocdOperatorReplaceStart, argocdOperatorReplaceEnd := readReplaceBlockFromGoMod(argocdOperatorGoModPath) | |
| newArgoCDOperatorGoModFileContents := "" | |
| for x, line := range argocdOperatorLines { | |
| if x == argocdOperatorReplaceStart { | |
| newArgoCDOperatorGoModFileContents += "\t// This replace block is from Argo CD " + targetArgoCDVersion + " go.mod\n" | |
| // inject the replace block from argocd's go.mod | |
| for _, replaceBlockFromArgoCDLine := range replaceBlockFromArgoCD { | |
| newArgoCDOperatorGoModFileContents += replaceBlockFromArgoCDLine + "\n" | |
| } | |
| } else if x > argocdOperatorReplaceStart && x < argocdOperatorReplaceEnd { | |
| replaceBlockFromArgoCDLines, replaceBlockStart, replaceBlockEnd := readReplaceBlockFromGoMod(argoCDRepoRootGoModPath) | |
| replaceBlockFromArgoCD := replaceBlockFromArgoCDLines[replaceBlockStart : replaceBlockEnd+1] | |
| argocdOperatorGoModPath := filepath.Join(argocdOperatorRoot, "go.mod") | |
| argocdOperatorLines, argocdOperatorReplaceStart, argocdOperatorReplaceEnd := readReplaceBlockFromGoMod(argocdOperatorGoModPath) | |
| newArgoCDOperatorGoModFileContents := "" | |
| for x, line := range argocdOperatorLines { | |
| if x == argocdOperatorReplaceStart { | |
| newArgoCDOperatorGoModFileContents += "\t// This replace block is from Argo CD " + targetArgoCDVersion + " go.mod\n" | |
| // inject the replace block from argocd's go.mod | |
| for _, replaceBlockFromArgoCDLine := range replaceBlockFromArgoCD { | |
| newArgoCDOperatorGoModFileContents += replaceBlockFromArgoCDLine + "\n" | |
| } | |
| } else if x > argocdOperatorReplaceStart && x < argocdOperatorReplaceEnd { |
🤖 Prompt for AI Agents
In hack/update-dependencies-script/main.go around lines 379 to 398 the code
slices replaceBlockFromArgoCDLines using
replaceBlockFromArgoCDLines[replaceBlockStart:replaceBlockEnd], which omits the
final line because readReplaceBlockFromGoMod returns inclusive end indices;
change the slice to use
replaceBlockFromArgoCDLines[replaceBlockStart:replaceBlockEnd+1] so the entire
replace block (including the last replace entry) is copied into the generated
go.mod.
| // Extract Digest value from JSON | ||
| var jsonMap map[string]any | ||
| if err := json.Unmarshal([]byte(stdout), &jsonMap); err != nil { | ||
| exitWithError(fmt.Errorf("unexpected unmarshal error: %v", err)) | ||
| return nil | ||
| } | ||
| digestVal := jsonMap["Digest"] | ||
|
|
||
| if digestVal == "" { | ||
| exitWithError(fmt.Errorf("unable to extract digest val for %s", url)) | ||
| return nil | ||
| } | ||
|
|
||
| return &processedContainerImage{ | ||
| version: url[strings.Index(url, ":")+1:], | ||
| sha256Digest: digestVal.(string), | ||
| } |
There was a problem hiding this comment.
Defensively assert the Digest field before type asserting
If skopeo ever omits the Digest key (e.g., format change or registry quirk), digestVal.(string) will panic. Guard both presence and type so the script exits cleanly.
- digestVal := jsonMap["Digest"]
-
- if digestVal == "" {
+ digestVal, ok := jsonMap["Digest"].(string)
+ if !ok || digestVal == "" {
exitWithError(fmt.Errorf("unable to extract digest val for %s", url))
return nil
}
return &processedContainerImage{
version: url[strings.Index(url, ":")+1:],
- sha256Digest: digestVal.(string),
+ sha256Digest: digestVal,
}🤖 Prompt for AI Agents
In hack/update-dependencies-script/main.go around lines 526 to 542, the code
unsafely type-asserts jsonMap["Digest"] to string which can panic if the key is
missing or not a string; update the code to first check that the key exists and
that the value is a string (e.g., v, ok := jsonMap["Digest"]; if !ok || v == nil
{ exitWithError(...) } s, ok := v.(string); if !ok || s == "" {
exitWithError(...) }) and then use the validated string for sha256Digest so the
script exits cleanly rather than panicking.
|
|
||
| SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) | ||
|
|
||
| cd $SCRIPT_DIR |
There was a problem hiding this comment.
Add error handling for directory change.
The cd command should handle failures to prevent unexpected behavior if the directory doesn't exist or is inaccessible.
Apply this diff:
-cd $SCRIPT_DIR
+cd "$SCRIPT_DIR" || exit 1Note: Also added quotes around $SCRIPT_DIR to handle paths with spaces.
📝 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.
| cd $SCRIPT_DIR | |
| cd "$SCRIPT_DIR" || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In hack/update-dependencies-script/run.sh around line 5, the script does an
unguarded cd $SCRIPT_DIR which can fail and continue execution; change it to cd
"$SCRIPT_DIR" and check its exit status, printing an explanatory error to stderr
(or using echo) and exiting non‑zero (e.g., exit 1) if the cd fails so the
script stops instead of running in the wrong directory.
| func copyFile(srcParam, dstParam string) error { | ||
| srcFile, err := os.Open(srcParam) | ||
| if err != nil { | ||
| return fmt.Errorf("could not open source file %s: %w", srcParam, err) | ||
| } | ||
|
|
||
| destFile, err := os.Create(dstParam) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create destination file %s: %w", dstParam, err) | ||
| } | ||
|
|
||
| _, err = io.Copy(destFile, srcFile) | ||
| if err != nil { | ||
| return fmt.Errorf("could not copy content from %s to %s: %w", srcParam, dstParam, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit file closing with defer.
The files should be explicitly closed using defer to prevent resource leaks, especially if errors occur after opening.
Apply this diff:
func copyFile(srcParam, dstParam string) error {
srcFile, err := os.Open(srcParam)
if err != nil {
return fmt.Errorf("could not open source file %s: %w", srcParam, err)
}
+ defer srcFile.Close()
destFile, err := os.Create(dstParam)
if err != nil {
return fmt.Errorf("could not create destination file %s: %w", dstParam, err)
}
+ defer destFile.Close()
_, err = io.Copy(destFile, srcFile)
if err != nil {
return fmt.Errorf("could not copy content from %s to %s: %w", srcParam, dstParam, err)
}
return 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 copyFile(srcParam, dstParam string) error { | |
| srcFile, err := os.Open(srcParam) | |
| if err != nil { | |
| return fmt.Errorf("could not open source file %s: %w", srcParam, err) | |
| } | |
| destFile, err := os.Create(dstParam) | |
| if err != nil { | |
| return fmt.Errorf("could not create destination file %s: %w", dstParam, err) | |
| } | |
| _, err = io.Copy(destFile, srcFile) | |
| if err != nil { | |
| return fmt.Errorf("could not copy content from %s to %s: %w", srcParam, dstParam, err) | |
| } | |
| return nil | |
| } | |
| func copyFile(srcParam, dstParam string) error { | |
| srcFile, err := os.Open(srcParam) | |
| if err != nil { | |
| return fmt.Errorf("could not open source file %s: %w", srcParam, err) | |
| } | |
| defer srcFile.Close() | |
| destFile, err := os.Create(dstParam) | |
| if err != nil { | |
| return fmt.Errorf("could not create destination file %s: %w", dstParam, err) | |
| } | |
| defer destFile.Close() | |
| _, err = io.Copy(destFile, srcFile) | |
| if err != nil { | |
| return fmt.Errorf("could not copy content from %s to %s: %w", srcParam, dstParam, err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In hack/update-dependencies-script/utils.go around lines 98 to 115, the file
handles opened with os.Open and os.Create are not closed, risking resource
leaks; add defer calls to close both srcFile and destFile immediately after each
successful open/create, and capture/propagate any Close error from destFile
(wrap it with fmt.Errorf) so a failed Close doesn't get ignored (use a named
return error or check the copy error and then check destFile.Close() and return
the first non-nil error).
| func getFileContentsAsLines(filePath string) []string { | ||
| bytes, err := os.ReadFile(filePath) | ||
| if err != nil { | ||
| exitWithError(fmt.Errorf("unable to dex_test.go: %v", err)) | ||
| return nil | ||
| } | ||
|
|
||
| return strings.Split(string(bytes), "\n") | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix error message and remove unreachable code.
The error message incorrectly mentions "dex_test.go", and line 137 is unreachable after exitWithError.
Apply this diff:
func getFileContentsAsLines(filePath string) []string {
bytes, err := os.ReadFile(filePath)
if err != nil {
- exitWithError(fmt.Errorf("unable to dex_test.go: %v", err))
- return nil
+ exitWithError(fmt.Errorf("unable to read file %s: %v", filePath, err))
}
return strings.Split(string(bytes), "\n")
-
}📝 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 getFileContentsAsLines(filePath string) []string { | |
| bytes, err := os.ReadFile(filePath) | |
| if err != nil { | |
| exitWithError(fmt.Errorf("unable to dex_test.go: %v", err)) | |
| return nil | |
| } | |
| return strings.Split(string(bytes), "\n") | |
| } | |
| func getFileContentsAsLines(filePath string) []string { | |
| bytes, err := os.ReadFile(filePath) | |
| if err != nil { | |
| exitWithError(fmt.Errorf("unable to read file %s: %v", filePath, err)) | |
| } | |
| return strings.Split(string(bytes), "\n") | |
| } |
🤖 Prompt for AI Agents
In hack/update-dependencies-script/utils.go around lines 133 to 142, the error
message incorrectly references "dex_test.go" and the return after exitWithError
is unreachable; change the fmt.Errorf message to include the actual filePath
(e.g., fmt.Errorf("unable to read %s: %v", filePath, err)) and remove the
unreachable "return nil" after calling exitWithError so the function either
exits or returns only once; ensure the function still returns
strings.Split(string(bytes), "\n") on success.
Signed-off-by: Jonathan West <jgwest@gmail.com>
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
b4fa01d to
fc41142
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hack/update-dependencies-script/run.sh (1)
5-5: Add error handling for directory change (flagged in previous review).Line 5 contains an unguarded
cdwithout error handling. This issue was previously flagged and should be fixed before merge.Apply this diff:
-cd $SCRIPT_DIR +cd "$SCRIPT_DIR" || exit 1This ensures the script exits if the directory change fails (e.g., directory doesn't exist or lacks permissions) and handles paths containing spaces.
🧹 Nitpick comments (1)
hack/update-dependencies-script/README.md (1)
10-12: Add language specifiers to fenced code blocks for proper syntax highlighting.The code blocks are missing language identifiers, which helps with readability and enables syntax highlighting.
Apply this diff:
-Example: -``` +Example: +```makefile ARGO_CD_TARGET_VERSION ?= 3.1.8 -``` +``` -Then run the script: -``` +Then run the script: +```bash make update-dependencies -``` +```Also applies to: 15-17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
hack/update-dependencies-script/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/workflows/ci-build.yaml(5 hunks).github/workflows/lint.yaml(2 hunks)Makefile(2 hunks)hack/update-dependencies-script/README.md(1 hunks)hack/update-dependencies-script/go.mod(1 hunks)hack/update-dependencies-script/main.go(1 hunks)hack/update-dependencies-script/run.sh(1 hunks)hack/update-dependencies-script/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- hack/update-dependencies-script/go.mod
- .github/workflows/lint.yaml
- hack/update-dependencies-script/utils.go
- hack/update-dependencies-script/main.go
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 375-375: Missing required phony target "all"
(minphony)
[warning] 375-375: Missing required phony target "clean"
(minphony)
[warning] 375-375: Missing required phony target "test"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
hack/update-dependencies-script/README.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
hack/update-dependencies-script/run.sh
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-operator
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Verify Argo CD has updated dependencies, for the given target version
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/ci-build.yaml (1)
105-106: Verify the background script doesn't introduce test timing or resource conflicts.The new
hack/df-loop.sh &runs in the background throughout test execution without explicit synchronization or completion verification. Ensure this background process:
- Doesn't conflict with test resources or namespaces
- Properly terminates after tests complete
- Doesn't mask test failures if it exits early
Can you clarify the purpose of
hack/df-loop.shand verify that running it concurrently with the test suite doesn't introduce flakiness or resource contention? If this script should run until tests complete, consider replacing the bare&with explicit cleanup (e.g.,trapto kill the background process on exit).
| # Updates upstream dependencies throughout the repository | ||
| .PHONY: update-dependencies | ||
| update-dependencies: | ||
| "hack/update-dependencies-script/run.sh" |
There was a problem hiding this comment.
Remove quotes from script path in the make recipe.
Line 377 has the script path quoted, which causes the shell to interpret it as a literal filename (with quotes included) rather than executing the script. Makefile recipes are passed to the shell, so shell quoting rules apply.
Apply this diff:
# Updates upstream dependencies throughout the repository
.PHONY: update-dependencies
update-dependencies:
- "hack/update-dependencies-script/run.sh"
+ hack/update-dependencies-script/run.shAlternatively, if the script path needs quoting for safety (e.g., paths with spaces), use the shell's command expansion syntax:
# Updates upstream dependencies throughout the repository
.PHONY: update-dependencies
update-dependencies:
+ $(shell pwd)/hack/update-dependencies-script/run.shHowever, the simpler unquoted path is preferable for this use case.
📝 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.
| # Updates upstream dependencies throughout the repository | |
| .PHONY: update-dependencies | |
| update-dependencies: | |
| "hack/update-dependencies-script/run.sh" | |
| # Updates upstream dependencies throughout the repository | |
| .PHONY: update-dependencies | |
| update-dependencies: | |
| hack/update-dependencies-script/run.sh |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 375-375: Missing required phony target "all"
(minphony)
[warning] 375-375: Missing required phony target "clean"
(minphony)
[warning] 375-375: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In Makefile around lines 374 to 377, the update-dependencies recipe uses a
quoted script path which causes the shell to treat the quotes as part of the
filename; remove the surrounding double quotes so the shell executes
hack/update-dependencies-script/run.sh directly, or if you must protect spaces
use the shell command invocation form instead (e.g., use shell expansion) so the
script is executed rather than treated as a literal quoted filename.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/df-loop.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: build
- GitHub Check: build-operator
- GitHub Check: Run end-to-end tests (v1.27.1)
| #!/bin/bash | ||
|
|
||
| # Loop forever, running df -h and sleeping for 5 seconds | ||
| while true; do | ||
| df -h | ||
| sleep 5 | ||
| done |
There was a problem hiding this comment.
Add signal handling for graceful CI process termination.
The infinite loop lacks signal handlers for SIGTERM and SIGINT, which could prevent the background process from exiting cleanly when the CI workflow terminates it. This may cause orphaned processes or delayed test completion.
Additionally, the df -h output is sent to stdout without timestamps or context, making it harder to correlate disk events with test execution. Consider redirecting to a log file and adding timestamps.
Apply this diff to add signal handling and improve output management:
#!/bin/bash
-# Loop forever, running df -h and sleeping for 5 seconds
+# Loop forever, running df -h and sleeping for 5 seconds
+# This script monitors disk space during CI test execution.
+
+# Clean exit on signals
+trap 'exit 0' SIGTERM SIGINT
+
+LOG_FILE="${CI_OUTPUT_DIR:-/tmp}/df-loop.log"
+
while true; do
- df -h
+ echo "[$(date '+%Y-%m-%d %H:%M:%S')] $(df -h)" >> "$LOG_FILE"
sleep 5
doneThis ensures proper cleanup when the CI workflow sends termination signals and provides timestamped output for correlation with test events.
📝 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.
| #!/bin/bash | |
| # Loop forever, running df -h and sleeping for 5 seconds | |
| while true; do | |
| df -h | |
| sleep 5 | |
| done | |
| #!/bin/bash | |
| # Loop forever, running df -h and sleeping for 5 seconds | |
| # This script monitors disk space during CI test execution. | |
| # Clean exit on signals | |
| trap 'exit 0' SIGTERM SIGINT | |
| LOG_FILE="${CI_OUTPUT_DIR:-/tmp}/df-loop.log" | |
| while true; do | |
| echo "[$(date '+%Y-%m-%d %H:%M:%S')] $(df -h)" >> "$LOG_FILE" | |
| sleep 5 | |
| done |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Makefile (1)
374-377: Remove quotes from the script path in the Makefile recipe.The script path is wrapped in double quotes, which causes the shell to treat the quotes as part of the literal filename rather than executing the script. Makefile recipes are passed directly to the shell—shell quoting rules apply—but the quotes here will cause a "command not found" error at runtime.
Apply this diff to fix the issue:
# Updates upstream dependencies throughout the repository .PHONY: update-dependencies update-dependencies: - "hack/update-dependencies-script/run.sh" + hack/update-dependencies-script/run.shhack/df-loop.sh (1)
1-10: Add signal handling for graceful CI process termination (previously noted, still unaddressed).The script lacks signal handlers for
SIGTERMandSIGINT, preventing clean exit when the CI workflow terminates it. Since this runs as a background process, orphaned loops or delayed test completion are likely.Apply this diff to add signal handling:
#!/bin/bash -# Loop forever, running df -h and sleeping for 5 seconds +# Loop forever, running df -h and sleeping for 5 seconds +# Monitors disk and namespace state during CI test execution + +# Clean exit on termination signals +trap 'exit 0' SIGTERM SIGINT + while true; do echo "--------------------" kubectl get namespaces echo "----" df -h | grep root sleep 10 done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(4 hunks)hack/df-loop.sh(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 375-375: Missing required phony target "all"
(minphony)
[warning] 375-375: Missing required phony target "clean"
(minphony)
[warning] 375-375: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: build
- GitHub Check: Verify Argo CD has updated dependencies, for the given target version
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build-operator
🔇 Additional comments (3)
Makefile (3)
8-13: LGTM on ARGO_CD_TARGET_VERSION introduction.Clear documentation, sensible default (3.1.9), and explicit guidance on the expected format. The integration with the
update-dependenciestarget supports the overall dependency management workflow.
15-21: Improved container runtime detection with graceful fallback.Changing from a hard error to a warning when neither Docker nor Podman is found is a pragmatic improvement. It allows the Makefile to remain usable in environments where container operations aren't needed or possible (e.g., CI runners with pre-installed tooling or local development where the user hasn't yet installed a runtime).
349-349: Verify the rationale for reducing Ginkgo parallel concurrency from 4 to 3 processes.The change reduces parallel test concurrency from 4 procs to 3. This may reflect resource constraints or stability improvements in the test environment, but the change warrants verification to ensure it doesn't inadvertently reduce test throughput for CI pipelines or local development.
| # Loop forever, running df -h and sleeping for 5 seconds | ||
| while true; do | ||
| echo "--------------------" | ||
| kubectl get namespaces |
There was a problem hiding this comment.
Add error handling for kubectl commands that may fail without cluster access.
The kubectl get namespaces call has no error handling. If the cluster is unreachable, this will emit errors every 10 seconds, polluting CI logs.
Apply this diff to add a guard:
while true; do
echo "--------------------"
- kubectl get namespaces
+ kubectl get namespaces 2>/dev/null || echo " (kubectl unavailable)"
echo "----"
df -h | grep root
sleep 10
done📝 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.
| kubectl get namespaces | |
| kubectl get namespaces 2>/dev/null || echo " (kubectl unavailable)" |
🤖 Prompt for AI Agents
In hack/df-loop.sh around line 6, the plain `kubectl get namespaces` call lacks
error handling and will spam CI if the cluster is unreachable; change it to run
`kubectl get namespaces` with output/error redirected and check its exit status:
if it succeeds continue normally, if it fails either log a single concise error
message (or increment a retry counter) and sleep/exit as appropriate to avoid
repeated noisy errors; ensure you suppress kubectl stderr (redirect to
/dev/null) and only emit a controlled error message once or on state change.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-build.yaml(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build
- GitHub Check: build-operator
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: Run golangci-lint and gosec
🔇 Additional comments (3)
.github/workflows/ci-build.yaml (3)
5-7: YAML formatting and quoting improvements.These changes consistently quote string literals in YAML, which is a best practice for clarity and robustness. No functional changes here.
Also applies to: 12-14, 33-33, 80-80
49-49: Matrix and comment formatting adjustments.Minor formatting adjustments for consistency and clarity. No functional impact.
Also applies to: 70-72
54-56: KUTTL checksum verified as correct. The SHA256 sumad21c0d2be495a8f6cfc0821e592ae61afd191ebd453156d9b204e2524fceaf2matches the official KUTTL v0.10.0kubectl-kuttl_0.10.0_linux_x86_64binary. Version and checksum are consistent.
|
|
||
| func main() { | ||
|
|
||
| wd, err := os.Getwd() |
There was a problem hiding this comment.
I see this is correct when called as make>run.sh>main.go, though I would like to propose passing in the repo root as a parameter. That way, we can run go run straight from the Makefile and drop the shell script middleman. I find it more robust because then main.go would not rely on being invoked in a specific way - you test the root is the real root few lines after.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/ginkgo/fixture/fixture.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/fixture/fixture.go (1)
tests/ginkgo/fixture/os/fixture.go (1)
ExecCommandWithOutputParam(15-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Verify Argo CD has updated dependencies, for the given target version
- GitHub Check: build-operator
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build
🔇 Additional comments (2)
tests/ginkgo/fixture/fixture.go (2)
7-7: LGTM!The
strconvimport is correctly added to support integer parsing in the new disk-space check function.
89-89: Good placement for the disk-space check.Invoking the disk-space check at the start of
EnsureParallelCleanSlate()ensures tests don't proceed until sufficient space is available, which aligns well with the test infrastructure goals. However, this assumes the critical issues inwaitForRootPartitionToHaveMinimumDiskSpace()are resolved first.
| for { | ||
|
|
||
| output, err := osFixture.ExecCommandWithOutputParam(false, "df") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| GinkgoWriter.Println("------------------") | ||
| var rootEntry string | ||
| for line := range strings.SplitSeq(output, "\n") { | ||
| GinkgoWriter.Println("-", line) | ||
| if strings.Contains(line, "root") { | ||
| rootEntry = line | ||
| break | ||
| } | ||
| } | ||
| if rootEntry == "" { | ||
| GinkgoWriter.Println("No root entry to manage") | ||
| return | ||
| } | ||
|
|
||
| // Split output by whitespace to parse df fields | ||
| fields := strings.Fields(rootEntry) | ||
| Expect(len(fields)).To(BeNumerically(">=", 4), "df output should have at least 4 fields") | ||
|
|
||
| // Parse fields[3] which is the available space in bytes | ||
| availableBytes, err := strconv.ParseInt(fields[3], 10, 64) | ||
| Expect(err).ToNot(HaveOccurred(), "failed to parse available bytes from df output") | ||
|
|
||
| // Convert bytes to gigabytes (1 GB = 1024^3 bytes) | ||
| availableGB := availableBytes / (1024 * 1024 * 1024) | ||
|
|
||
| // If less than 6 GB available, sleep and continue the loop | ||
| if availableGB < 6 { | ||
| GinkgoWriter.Println("Waiting for root partition to have minimum size, current size:", availableGB, "GB") | ||
| time.Sleep(time.Second * 10) | ||
| } else { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a timeout to prevent infinite hangs.
The function uses an infinite loop with no timeout or maximum retry count. If disk space never reaches 6 GB (e.g., due to a persistent disk issue or misconfiguration), this will hang indefinitely and block all tests.
Consider adding a timeout or maximum retry count:
func waitForRootPartitionToHaveMinimumDiskSpace() {
+ const maxWaitTime = 10 * time.Minute
+ const minDiskSpaceGB = 6
+ startTime := time.Now()
for {
+ if time.Since(startTime) > maxWaitTime {
+ Fail(fmt.Sprintf("Timed out waiting for root partition to have at least %d GB available", minDiskSpaceGB))
+ }
output, err := osFixture.ExecCommandWithOutputParam(false, "df")
Expect(err).ToNot(HaveOccurred())
// ... rest of parsing logic ...
// If less than 6 GB available, sleep and continue the loop
- if availableGB < 6 {
+ if availableGB < minDiskSpaceGB {
- GinkgoWriter.Println("Waiting for root partition to have minimum size, current size:", availableGB, "GB")
+ GinkgoWriter.Printf("Waiting for root partition to have at least %d GB available, current: %d GB (elapsed: %v)\n", minDiskSpaceGB, availableGB, time.Since(startTime))
time.Sleep(time.Second * 10)
} else {
break
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/ginkgo/fixture/fixture.go around lines 48 to 84 there is an infinite
loop waiting for root to have >=6GB which can hang forever; add a timeout or max
retry mechanism that aborts the loop and fails the test when the wait exceeds a
safe limit (e.g., track start := time.Now() and check time.Since(start) against
a timeout like 5m, or introduce a retry counter with a max attempts value), keep
the existing sleep between attempts, and when the timeout/max retries is reached
call Expect or GinkgoFail with a clear message (including last observed
availableGB and possibly df output) so the test fails fast instead of hanging
indefinitely.
| for line := range strings.SplitSeq(output, "\n") { | ||
| GinkgoWriter.Println("-", line) | ||
| if strings.Contains(line, "root") { | ||
| rootEntry = line | ||
| break | ||
| } | ||
| } | ||
| if rootEntry == "" { | ||
| GinkgoWriter.Println("No root entry to manage") | ||
| return | ||
| } |
There was a problem hiding this comment.
Fix the root partition detection logic.
The current implementation searches for lines containing the substring "root", but the Linux root filesystem mounted at / typically does not have "root" in its df output line. For example:
Filesystem 1K-blocks Used Available Use% Mounted on
/dev/sda1 51474912 4184220 44649264 11% /
The root partition line shows / in the mount point column (typically the last field), not "root".
Apply this diff to fix the detection:
var rootEntry string
for line := range strings.SplitSeq(output, "\n") {
GinkgoWriter.Println("-", line)
- if strings.Contains(line, "root") {
+ // Look for the root filesystem mounted at "/"
+ fields := strings.Fields(line)
+ if len(fields) >= 6 && fields[5] == "/" {
rootEntry = line
break
}
}
if rootEntry == "" {
- GinkgoWriter.Println("No root entry to manage")
- return
+ // Root partition not found - this is unexpected and tests should not proceed
+ Expect(rootEntry).ToNot(BeEmpty(), "Root partition not found in df output")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for line := range strings.SplitSeq(output, "\n") { | |
| GinkgoWriter.Println("-", line) | |
| if strings.Contains(line, "root") { | |
| rootEntry = line | |
| break | |
| } | |
| } | |
| if rootEntry == "" { | |
| GinkgoWriter.Println("No root entry to manage") | |
| return | |
| } | |
| var rootEntry string | |
| for line := range strings.SplitSeq(output, "\n") { | |
| GinkgoWriter.Println("-", line) | |
| // Look for the root filesystem mounted at "/" | |
| fields := strings.Fields(line) | |
| if len(fields) >= 6 && fields[5] == "/" { | |
| rootEntry = line | |
| break | |
| } | |
| } | |
| if rootEntry == "" { | |
| // Root partition not found - this is unexpected and tests should not proceed | |
| Expect(rootEntry).ToNot(BeEmpty(), "Root partition not found in df output") | |
| } |
🤖 Prompt for AI Agents
In tests/ginkgo/fixture/fixture.go around lines 54 to 64, the code looks for the
substring "root" in each df output line which misses the actual root mount
(which shows "/" in the mountpoint column); update the detection to split each
non-empty line with strings.Fields, check the last field (mount point) for
exactly "/" (or for robustness, compare to "/" or to the configured root mount
variable), set rootEntry to the full line when that condition matches, and keep
the existing GinkgoWriter.Println logging and the early return when no rootEntry
is found.
| Expect(err).ToNot(HaveOccurred()) | ||
| GinkgoWriter.Println("------------------") | ||
| var rootEntry string | ||
| for line := range strings.SplitSeq(output, "\n") { |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Check go.mod for the Go version requirement
echo "=== Checking go.mod for Go version ==="
if [ -f go.mod ]; then
cat go.mod | head -20
else
echo "go.mod not found in repository root"
fiRepository: argoproj-labs/argocd-operator
<shell_result>
Repository: argoproj-labs/argocd-operator
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository argoproj-labs/argocd-operator: Clone operation failed
</shell_result>
What Go version introduced strings.SplitSeq function?
<web_search_result>
strings.SplitSeq was introduced in Go 1.24. [1][2]
Sources:
[1] Go 1.24 release blog.
[2] modernize analyzer (stringsseq) noting SplitSeq was added in Go 1.24.
</web_search_result>
Correct Go version requirement for strings.SplitSeq.
The code uses strings.SplitSeq, which is an iterator-based function introduced in Go 1.24 (not Go 1.23 as originally stated). Please ensure the project is using Go 1.24 or later.
🤖 Prompt for AI Agents
In tests/ginkgo/fixture/fixture.go around line 54, the code uses
strings.SplitSeq which requires Go 1.24+, so update the project toolchain to Go
1.24 or later: set the module Go version in go.mod to 1.24, update CI/workflow
configurations and any Dockerfiles or build images to use a Go 1.24+ image, and
run go mod tidy and test to verify; if upgrading the toolchain is not possible,
replace strings.SplitSeq with a compatibility implementation (e.g.,
strings.Split) instead.
tests/ginkgo/fixture/fixture.go
Outdated
| // Split output by whitespace to parse df fields | ||
| fields := strings.Fields(rootEntry) | ||
| Expect(len(fields)).To(BeNumerically(">=", 4), "df output should have at least 4 fields") | ||
|
|
||
| // Parse fields[3] which is the available space in bytes | ||
| availableBytes, err := strconv.ParseInt(fields[3], 10, 64) | ||
| Expect(err).ToNot(HaveOccurred(), "failed to parse available bytes from df output") | ||
|
|
||
| // Convert bytes to gigabytes (1 GB = 1024^3 bytes) | ||
| availableGB := availableBytes / (1024 * 1024 * 1024) |
There was a problem hiding this comment.
Fix the unit conversion - df outputs KB by default, not bytes.
The comment on Line 70 claims that fields[3] is "the available space in bytes," but df without any flags outputs in 1K-blocks (kilobytes), not bytes. The conversion on Line 75 incorrectly divides by 1024³, which would only be correct if the input were in bytes.
Apply this diff to fix the unit conversion:
- // Parse fields[3] which is the available space in bytes
+ // Parse fields[3] which is the available space in KB (1K-blocks is df default)
availableBytes, err := strconv.ParseInt(fields[3], 10, 64)
Expect(err).ToNot(HaveOccurred(), "failed to parse available bytes from df output")
- // Convert bytes to gigabytes (1 GB = 1024^3 bytes)
- availableGB := availableBytes / (1024 * 1024 * 1024)
+ // Convert KB to gigabytes (1 GB = 1024^2 KB)
+ availableGB := availableBytes / (1024 * 1024)📝 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.
| // Split output by whitespace to parse df fields | |
| fields := strings.Fields(rootEntry) | |
| Expect(len(fields)).To(BeNumerically(">=", 4), "df output should have at least 4 fields") | |
| // Parse fields[3] which is the available space in bytes | |
| availableBytes, err := strconv.ParseInt(fields[3], 10, 64) | |
| Expect(err).ToNot(HaveOccurred(), "failed to parse available bytes from df output") | |
| // Convert bytes to gigabytes (1 GB = 1024^3 bytes) | |
| availableGB := availableBytes / (1024 * 1024 * 1024) | |
| // Split output by whitespace to parse df fields | |
| fields := strings.Fields(rootEntry) | |
| Expect(len(fields)).To(BeNumerically(">=", 4), "df output should have at least 4 fields") | |
| // Parse fields[3] which is the available space in KB (1K-blocks is df default) | |
| availableBytes, err := strconv.ParseInt(fields[3], 10, 64) | |
| Expect(err).ToNot(HaveOccurred(), "failed to parse available bytes from df output") | |
| // Convert KB to gigabytes (1 GB = 1024^2 KB) | |
| availableGB := availableBytes / (1024 * 1024) |
🤖 Prompt for AI Agents
In tests/ginkgo/fixture/fixture.go around lines 66 to 75, the code treats
fields[3] from df as bytes and divides by 1024^3; df outputs 1K-blocks
(kilobytes) by default. Parse fields[3] as kilobytes (rename variable
accordingly), compute availableGB by dividing by 1024*1024 (KB -> GB), and
update the comment and any variable names/messages to reflect that the input is
in KB rather than bytes.
There was a problem hiding this comment.
It may not be always 1K:
man 1 diff
Display values are in units of the first available SIZE from
--block-size, and the DF_BLOCK_SIZE, BLOCK_SIZE and BLOCKSIZE
environment variables. Otherwise, units default to 1024 bytes
(or 512 if POSIXLY_CORRECT is set).
Better run df -m to always get megabytes.
There was a problem hiding this comment.
@olivergondza, excellent point! You're absolutely right that df output units can vary based on environment variables. Using df -m to explicitly request megabytes is a much more robust solution.
Here's the corrected code that uses df -m:
func waitForRootPartitionToHaveMinimumDiskSpace() {
for {
output, err := osFixture.ExecCommandWithOutputParam(false, "df", "-m")
Expect(err).ToNot(HaveOccurred())
GinkgoWriter.Println("------------------")
var rootEntry string
for line := range strings.SplitSeq(output, "\n") {
GinkgoWriter.Println("-", line)
if strings.Contains(line, "root") {
rootEntry = line
break
}
}
if rootEntry == "" {
GinkgoWriter.Println("No root entry to manage")
return
}
// Split output by whitespace to parse df fields
fields := strings.Fields(rootEntry)
Expect(len(fields)).To(BeNumerically(">=", 4), "df output should have at least 4 fields")
// Parse fields[3] which is the available space in megabytes (df -m)
availableMB, err := strconv.ParseInt(fields[3], 10, 64)
Expect(err).ToNot(HaveOccurred(), "failed to parse available megabytes from df output")
// Convert megabytes to gigabytes (1 GB = 1024 MB)
availableGB := availableMB / 1024
// If less than 6 GB available, sleep and continue the loop
if availableGB < 6 {
GinkgoWriter.Println("Waiting for root partition to have minimum size, current size:", availableGB, "GB")
time.Sleep(time.Second * 10)
} else {
break
}
}
}The key changes:
- Changed command to
df -mto explicitly request megabytes - Renamed
availableBytestoavailableMBfor clarity - Updated the conversion formula from
/ (1024 * 1024 * 1024)to/ 1024(MB → GB) - Updated comments to reflect the correct units
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
4ae1540 to
e60fe08
Compare
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
cd73093 to
40bdac1
Compare
2 participants
What type of PR is this?
/kind chore
What does this PR do / why we need it:
Have you updated the necessary documentation?
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.