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
Signed-off-by: Jonathan West <[email protected]>
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
7842460 to
b4fa01d
Compare
WalkthroughThis PR updates Argo CD dependencies to v3.1.9 and introduces an automated dependency update system. It modifies container image references, Go module versions, CRD definitions, and adds new Make targets and scripts that programmatically manage upstream dependency upgrades across multiple artifact types. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 }
| for idx := 0; idx < len(lines); idx++ { | ||
| if strings.HasPrefix(lines[idx], " Name: \"dex\",") && | ||
| strings.HasPrefix(lines[idx+1], " Image: \"ghcr.io/dexidp/dex@sha256:") { | ||
|
|
||
| newContent += lines[idx] + "\n" // No modification of first line required | ||
| newContent += " Image: \"ghcr.io/dexidp/dex@" + dexContainerImage.sha256Digest + "\", // (" + dexContainerImage.version + ") NOTE: this value is modified by dependency update script\n" | ||
|
|
||
| idx++ // Skip to the line after these 2 | ||
| match = true | ||
| } else { | ||
| newContent += lines[idx] + "\n" | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent index overrun when matching dex image block
lines[idx+1] is evaluated on every iteration without confirming that idx+1 is in bounds; when the loop reaches the final index this will panic and abort the script. Guard the look-ahead before dereferencing.
- for idx := 0; idx < len(lines); idx++ {
- if strings.HasPrefix(lines[idx], " Name: \"dex\",") &&
- strings.HasPrefix(lines[idx+1], " Image: \"ghcr.io/dexidp/dex@sha256:") {
+ for idx := 0; idx < len(lines); idx++ {
+ if idx+1 >= len(lines) {
+ newContent += lines[idx] + "\n"
+ continue
+ }
+ if strings.HasPrefix(lines[idx], " Name: \"dex\",") &&
+ strings.HasPrefix(lines[idx+1], " Image: \"ghcr.io/dexidp/dex@sha256:") {
newContent += lines[idx] + "\n" // No modification of first line required
newContent += " Image: \"ghcr.io/dexidp/dex@" + dexContainerImage.sha256Digest + "\", // (" + dexContainerImage.version + ") NOTE: this value is modified by dependency update script\n"
idx++ // Skip to the line after these 2
match = true
- } else {
- newContent += lines[idx] + "\n"
- }
+ continue
+ }
+ newContent += lines[idx] + "\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.
| for idx := 0; idx < len(lines); idx++ { | |
| if strings.HasPrefix(lines[idx], " Name: \"dex\",") && | |
| strings.HasPrefix(lines[idx+1], " Image: \"ghcr.io/dexidp/dex@sha256:") { | |
| newContent += lines[idx] + "\n" // No modification of first line required | |
| newContent += " Image: \"ghcr.io/dexidp/dex@" + dexContainerImage.sha256Digest + "\", // (" + dexContainerImage.version + ") NOTE: this value is modified by dependency update script\n" | |
| idx++ // Skip to the line after these 2 | |
| match = true | |
| } else { | |
| newContent += lines[idx] + "\n" | |
| } | |
| } | |
| for idx := 0; idx < len(lines); idx++ { | |
| if idx+1 >= len(lines) { | |
| newContent += lines[idx] + "\n" | |
| continue | |
| } | |
| if strings.HasPrefix(lines[idx], " Name: \"dex\",") && | |
| strings.HasPrefix(lines[idx+1], " Image: \"ghcr.io/dexidp/dex@sha256:") { | |
| newContent += lines[idx] + "\n" // No modification of first line required | |
| newContent += " Image: \"ghcr.io/dexidp/dex@" + dexContainerImage.sha256Digest + "\", // (" + dexContainerImage.version + ") NOTE: this value is modified by dependency update script\n" | |
| idx++ // Skip to the line after these 2 | |
| match = true | |
| continue | |
| } | |
| newContent += lines[idx] + "\n" | |
| } |
🤖 Prompt for AI Agents
In hack/update-dependencies-script/main.go around lines 121 to 133, the loop
unconditionally dereferences lines[idx+1] which can cause an out-of-range panic
when idx is the last index; add a bounds check (e.g., ensure idx+1 < len(lines))
before evaluating or using lines[idx+1] in the if condition, preserving the
existing match behavior and idx++ logic so the replacement still occurs and the
loop continues safely.
| // 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.
1 participant
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
Chores