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
📝 WalkthroughWalkthroughAdds an automated dependency-upgrade flow: GitHub Actions job + Make target that run a new Go-based script to bump Argo CD version, sync CRDs, update image digests and go.mod replace blocks, regenerate manifests/bundle, and verify no unexpected repo diffs. Changes
Sequence Diagram(s)sequenceDiagram
actor GHA as GitHub Actions
participant WF as Workflow Job
participant Make as Make Target
participant Script as Go Script
participant ArgoCD as Argo CD Repo
participant Operator as Operator Repo
GHA->>WF: trigger verify_argo_cd_has_updated_dependencies
WF->>Make: run `make update-dependencies`
Make->>Script: execute `hack/update-dependencies-script/run.sh`
Script->>Script: read ARGO_CD_TARGET_VERSION from Makefile
Script->>ArgoCD: clone Argo CD at target version
ArgoCD-->>Script: return CRDs, go.mod, image metadata
Script->>Operator: update image digests (defaults, Dockerfile, tests)
Script->>Operator: copy CRDs into config/crd bases
Script->>Operator: propagate go.mod replace blocks, tidy, regenerate manifests/bundle
Script-->>Make: exit status
Make-->>WF: return status
WF->>WF: diff repo (ignore `createdAt`), fail on unexpected changes
WF-->>GHA: job result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 }
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).
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)
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.
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.
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.
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
2 times, most recently
from
cd73093 to
40bdac1
Compare
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
b0310e1 to
a2db378
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
hack/update-dependencies-script/run.sh (1)
5-5: Add error handling for directory change.The
cdcommand should handle failures to prevent unexpected behavior if the directory doesn't exist. Also, quote the variable to handle paths with spaces.Apply this diff:
-cd $SCRIPT_DIR +cd "$SCRIPT_DIR" || exit 1Makefile (1)
374-377: Remove quotes from script path in the make recipe.The script path is quoted, which causes the shell to interpret it as a literal filename rather than executing the script.
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.shhack/update-dependencies-script/main.go (5)
121-133: Prevent index overrun when matching dex image block.
lines[idx+1]is accessed without confirming thatidx+1is in bounds. When the loop reaches the final index, this will panic.Apply this diff:
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:") {
164-174: Apply bounds check when updating build/util/Dockerfile.This loop also accesses
lines[idx+1]without verifying bounds. Add a guard before accessing the next line.Apply this diff:
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") {
379-380: Copy the entire replace block (include the last line).
readReplaceBlockFromGoModreturns indices wherereplaceBlockEndpoints to the last line of content. Slicing with[replaceBlockStart:replaceBlockEnd]excludes the final replace entry. UsereplaceBlockEnd+1to include it.Apply this diff:
- replaceBlockFromArgoCD := replaceBlockFromArgoCDLines[replaceBlockStart:replaceBlockEnd] + replaceBlockFromArgoCD := replaceBlockFromArgoCDLines[replaceBlockStart : replaceBlockEnd+1]
532-541: Defensively check the Digest field before type assertion.If
skopeoomits theDigestkey or returns a non-string value,digestVal.(string)will panic. Guard both presence and type.Apply this diff:
- 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, }
225-229: Add missing return statement after exitWithError.Unlike other similar error handling blocks in this file, this block is missing a
returnstatement afterexitWithError. This could lead to a nil pointer dereference on line 229 if the error condition is met.Apply this diff:
targetArgoCDImageLine := removeDuplicateLines(grepForString(installYAMLContents, "quay.io/argoproj/argocd:")) if len(targetArgoCDImageLine) != 1 { exitWithError(fmt.Errorf("unexpected target argo cd image value: %v", targetArgoCDImageLine)) + return nil, nil } targetArgoCDImage := stripImagePrefix(targetArgoCDImageLine[0])
🧹 Nitpick comments (2)
hack/update-dependencies-script/README.md (1)
10-17: Add language specifiers to fenced code blocks.The fenced code blocks are missing language specifiers for better syntax highlighting and linting compliance.
Apply this diff:
Example: -``` +```makefile ARGO_CD_TARGET_VERSION ?= 3.1.8 ``` Then run the script: -``` +```bash make update-dependencies ```hack/update-dependencies-script/main.go (1)
481-504: Consider cleaning up the temporary directory.The cloned Argo CD repository in the temp directory is never cleaned up. This could leave large temporary directories on disk after repeated runs.
Consider returning a cleanup function or adding a
deferinmain()to remove the temp directory:// In main(), after cloning: defer os.RemoveAll(filepath.Dir(argoCDRepoRoot))Or return a cleanup function from
cloneArgoCDRepoIntoTempDir:func cloneArgoCDRepoIntoTempDir(latestReleaseVersionTag string) (string, func(), error) { tmpDir, err := os.MkdirTemp("", "argo-cd-src") if err != nil { return "", nil, err } cleanup := func() { os.RemoveAll(tmpDir) } // ... rest of function return newWorkDir, cleanup, nil }
📜 Review details
Configuration used: defaults
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 (7)
.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 (2)
- hack/update-dependencies-script/utils.go
- hack/update-dependencies-script/go.mod
🧰 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). (4)
- GitHub Check: build
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build-operator
🔇 Additional comments (1)
Makefile (1)
8-13: LGTM!The
ARGO_CD_TARGET_VERSIONvariable is well-documented with clear instructions about the version format (novprefix) and the follow-up action (make update-dependencies).
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
a2db378 to
f008878
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.github/workflows/lint.yaml:
- Around line 35-43: The workflow lacks a Go setup step before running make
update-dependencies; add an actions/setup-go step (with go-version set to
1.25.0) before the "Call update dependencies Makefile target" step so the
hack/update-dependencies-script/run.sh (invoked by make update-dependencies and
which runs go run .) uses the correct Go toolchain; ensure the new step is
placed immediately before the existing run: make update-dependencies step and
uses actions/setup-go@v4 (or appropriate stable version).
In @hack/update-dependencies-script/main.go:
- Around line 372-375: The comment above the function
copyGoModReplaceBlockFromArgoCDToArgoCDOperator contains a typo ("go.mold");
update the comment text to read "go.mod" (and ensure the rest of the sentence
still refers to argocd-operator go.mod) so the documentation accurately
references the Go module file; locate the block comment immediately preceding
the copyGoModReplaceBlockFromArgoCDToArgoCDOperator function and replace
"go.mold" with "go.mod".
- Around line 164-174: The loop that checks lines[idx+1] can panic at the end of
the slice; fix by ensuring you never read past the end of lines: either change
the loop to iterate while idx < len(lines)-1 and handle the final line after the
loop, or add an explicit bounds check (if idx+1 < len(lines) &&
strings.HasPrefix(lines[idx], "# Argo CD v") && strings.HasPrefix(lines[idx+1],
"FROM quay.io/argoproj/argocd@sha256") { ... }) before accessing lines[idx+1];
update the handling around idx++/match so the last line is still appended when
no match occurs.
- Around line 225-229: The code calls exitWithError(...) when
targetArgoCDImageLine has unexpected length but doesn't return, so execution can
continue into stripImagePrefix with invalid data; update the block handling
targetArgoCDImageLine (the call to exitWithError in the same conditional) to
immediately return after calling exitWithError (or change exitWithError to
terminate), ensuring no further use of targetArgoCDImageLine before it's valid;
references: targetArgoCDImageLine, exitWithError, targetArgoCDImage,
grepForString, removeDuplicateLines, stripImagePrefix.
- Around line 121-133: The loop currently reads lines[idx+1] without ensuring a
next element exists, which can panic if Name: "dex" is the last line; update the
condition in the for loop (or the if) to first check idx+1 < len(lines) before
evaluating strings.HasPrefix(lines[idx+1], ...) so you only access lines[idx+1]
when safe, and preserve the existing behavior around incrementing idx and
setting match when the two-line pattern is found (references: variables idx,
lines, match and dexContainerImage.sha256Digest).
In @Makefile:
- Around line 373-377: The Makefile target update-dependencies currently invokes
a quoted command name "hack/update-dependencies-script/run.sh" which makes Make
look for a command including the quotes; remove the surrounding quotes so the
recipe runs the script directly (invoke hack/update-dependencies-script/run.sh
from the update-dependencies target) ensuring the shell can find and execute the
script.
🧹 Nitpick comments (5)
hack/update-dependencies-script/README.md (1)
10-17: Add language specifiers to fenced code blocks.Per markdownlint, fenced code blocks should have a language specified for proper syntax highlighting and accessibility.
📝 Suggested fix
Example: -``` +```makefile ARGO_CD_TARGET_VERSION ?= 3.1.8Then run the script:
-+shell
make update-dependencies.github/workflows/lint.yaml (1)
36-37: Inconsistentactions/checkoutversion.The
lint_codejob usesactions/checkout@v6(line 17), but this new job usesactions/checkout@v5. For consistency and to benefit from the latest features/fixes, consider aligning versions.📝 Suggested fix
- name: Checkout code - uses: actions/checkout@v5 + uses: actions/checkout@v6hack/update-dependencies-script/main.go (3)
11-68: Consider cleaning up the temporary Argo CD clone directory.The script clones Argo CD into a temporary directory (line 33) but never cleans it up. This could leave large Git repositories in the temp directory after each run.
Suggested approach
Add cleanup using
deferafter the clone succeeds:argoCDRepoRoot, err := cloneArgoCDRepoIntoTempDir(targetArgoCDVersion) if err != nil { exitWithError(fmt.Errorf("unable to checkout Argo CD: %v", err)) return } defer os.RemoveAll(filepath.Dir(argoCDRepoRoot)) // Clean up parent temp dirAlternatively, add a flag to skip cleanup for debugging purposes.
42-45: Error message may obscure the actual failure reason.When
fileExistsreturns an error (e.g., permission denied), the error message incorrectly suggests running from the wrong directory. Consider distinguishing between "file doesn't exist" and "failed to check file existence."Suggested fix
- if rootGoModExists, err := fileExists(filepath.Join(argocdOperatorRoot, "go.mod")); err != nil || !rootGoModExists { - exitWithError(fmt.Errorf("script should be run from 'hack/update-dependencies-script' directory: %v", err)) + goModPath := filepath.Join(argocdOperatorRoot, "go.mod") + rootGoModExists, err := fileExists(goModPath) + if err != nil { + exitWithError(fmt.Errorf("failed to check for go.mod at %s: %v", goModPath, err)) + return + } + if !rootGoModExists { + exitWithError(fmt.Errorf("script should be run from 'hack/update-dependencies-script' directory (go.mod not found at %s)", goModPath)) return }
424-434: Error message is not specific to the failed operation.The error message on line 433 is identical to line 418, making it difficult to identify which step failed. Consider making the error message more specific.
Suggested fix
err = runCommandListWithWorkDir(argocdOperatorRoot, [][]string{ {"go", "mod", "tidy"}, {"rm", "-f", argocdOperatorRoot + "/bin/controller-gen"}, // Erase the controller-gen binary to ensure it is re-downloaded to the correct version {"make", "generate", "manifests"}, {"make", "bundle"}, {"make", "fmt"}, }) if err != nil { - exitWithError(fmt.Errorf("unable to update argocd-operator go.mod")) + exitWithError(fmt.Errorf("unable to regenerate manifests/bundle: %v", err)) }
📜 Review details
Configuration used: defaults
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 (10)
.github/workflows/lint.yamlMakefilebuild/util/Dockerfilecommon/defaults.gogo.modhack/update-dependencies-script/README.mdhack/update-dependencies-script/go.modhack/update-dependencies-script/main.gohack/update-dependencies-script/run.shhack/update-dependencies-script/utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- hack/update-dependencies-script/go.mod
- hack/update-dependencies-script/utils.go
- hack/update-dependencies-script/run.sh
🧰 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)
⏰ 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-operator
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build
- GitHub Check: Run golangci-lint and gosec
🔇 Additional comments (12)
go.mod (2)
183-184: LGTM - Replace block comment updated to v3.2.3.The comment accurately reflects the source of the replace directives.
226-226: LGTM - k8s.io/sample-controller version aligned with other k8s.io modules.Bumping to v0.34.0 maintains consistency with all other k8s.io replacements in this block.
build/util/Dockerfile (1)
1-2: LGTM - Dockerfile base image updated to v3.2.3.The digest is pinned for reproducibility and matches
common/defaults.go. The version comment aids future maintainers.common/defaults.go (1)
72-73: LGTM - Version bump to v3.2.3 is correct.The digest matches the official Argo CD v3.2.3 release and is consistently applied across
common/defaults.goandbuild/util/Dockerfile. The inline version comment aids maintainability.Makefile (1)
8-13: LGTM!The
ARGO_CD_TARGET_VERSIONvariable is well-documented with clear guidance about thevprefix and the recommended workflow for updates.hack/update-dependencies-script/main.go (7)
70-106: LGTM!The version parsing logic correctly handles the
?=operator and normalizes the version string with avprefix when needed.
271-317: LGTM!The CRD copy logic includes a reasonable sanity check to detect unexpected changes in the upstream CRD structure.
336-370: LGTM!The replace block parsing logic is straightforward. The assumption of a single replace block is documented and enforced.
438-479: LGTM!Good use of returning errors rather than calling
exitWithErrordirectly, allowing the caller to provide context-specific error messages.
481-504: LGTM!The clone and checkout logic is straightforward. Note that the returned path is the inner
argo-cddirectory, so cleanup should targetfilepath.Dir(argoCDRepoRoot).
506-549: LGTM!The skopeo integration and JSON parsing are correct. The version extraction (line 546) assumes a colon-separated format, which is standard for container image URLs.
551-555: LGTM!Clear documentation for the struct fields.
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
f008878 to
8592bbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.github/workflows/lint.yaml:
- Around line 36-37: The workflow uses inconsistent actions/checkout versions:
change the checkout step that currently references actions/checkout@v5 to match
the lint_code job (actions/checkout@v6) so all jobs use the same version; locate
the checkout step by the job name and the step title "Checkout code" and update
the version string used in the step to @v6.
In @hack/update-dependencies-script/go.mod:
- Line 3: Update the Go version directive in the go.mod file from "go 1.20" to
match the main project (e.g., "go 1.25") by replacing the existing go directive;
after updating the go directive, run go mod tidy and build/test the
hack/update-dependencies-script to ensure there are no incompatibilities.
- Around line 5-9: Update the module versions in go.mod: bump
github.com/google/go-github from v58.0.0 to v81.0.0 and replace gopkg.in/yaml.v2
v2.4.0 with gopkg.in/yaml.v3 (the v3 module path), then remove the explicit
indirect entry for github.com/google/go-querystring (do not keep indirect
requires by hand); after editing the require block run go mod tidy to let the
tool resolve and remove indirect dependencies and vendor any needed updates.
In @hack/update-dependencies-script/main.go:
- Around line 164-174: The loop that replaces Argo CD image references reads
lines[idx+1] without ensuring idx+1 < len(lines), causing an out-of-bounds
panic; modify the iteration or add a guard so the check uses a safe condition
(e.g., ensure idx+1 < len(lines) before calling strings.HasPrefix on
lines[idx+1]) when matching the "# Argo CD v" and "FROM
quay.io/argoproj/argocd@sha256" pair; keep the existing behavior of appending
the two replacement lines to newContent, increment idx to skip the consumed
line, and set match = true, similar to the bounds-safe approach used in
replaceDexImageReferenceInDexTest, but without changing other logic around
lines, idx, newContent, match, or argocdContainerImage.
- Around line 225-229: The branch that checks len(targetArgoCDImageLine) != 1
calls exitWithError(...) but does not return afterward, causing control to
continue; update the block in which targetArgoCDImageLine is validated to call
exitWithError(...) and then immediately return (e.g., add "return nil, nil"
after exitWithError) so the function exits consistently like the other error
paths before proceeding to stripImagePrefix and using targetArgoCDImage.
- Around line 545-548: The version extraction uses url[strings.Index(url,
":")+1:] which returns the whole URL when no ':' exists; in the function
returning &processedContainerImage, first compute the separator index (e.g., idx
:= strings.LastIndex(url, ":")) and validate idx > -1 before slicing; if idx <=
-1, set a safe fallback for the version (empty string or an explicit
error/placeholder) or return an error upstream instead of slicing blindly;
update the returned &processedContainerImage to use the validated index when
filling the version field.
🧹 Nitpick comments (7)
hack/update-dependencies-script/run.sh (1)
1-11: Add safer shell options and propagate exit codes.The script lacks
set -e(exit on error) which could mask failures. Also, the exit code fromgo run .is not explicitly handled, though it will propagate by default since it's the last command.Consider also passing
ARGO_CD_TARGET_VERSIONto the Go program if it needs to consume it (based on README instructions).Suggested improvement
#!/bin/bash +set -euo pipefail SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) if ! cd "$SCRIPT_DIR"; then echo "Error: Failed to change directory to $SCRIPT_DIR" >&2 exit 1 fi # Run the upgrade code -go run . +go run . "$@"hack/update-dependencies-script/README.md (1)
10-17: Add language specifiers to fenced code blocks.Per markdown best practices (and the static analysis hint), fenced code blocks should have a language specified for proper syntax highlighting.
Suggested fix
Example: -``` +```makefile ARGO_CD_TARGET_VERSION ?= 3.1.8 ``` Then run the script: -``` +```bash make update-dependencies ```hack/update-dependencies-script/main.go (5)
19-23: Usefilepath.Joinfor path construction.Using string concatenation with
/../..is less portable. Consider usingfilepath.Joinandfilepath.Dirfor cleaner path manipulation.Suggested improvement
- argocdOperatorRoot, err := filepath.Abs(wd + "/../..") + argocdOperatorRoot, err := filepath.Abs(filepath.Join(wd, "..", ".."))
32-39: Consider cleaning up the temporary directory.The cloned Argo CD repository in
argoCDRepoRootis never removed after the script completes. This could lead to disk space accumulation over repeated runs.Suggested improvement
// Clone Argo CD into a temporary directory argoCDRepoRoot, err := cloneArgoCDRepoIntoTempDir(targetArgoCDVersion) if err != nil { exitWithError(fmt.Errorf("unable to checkout Argo CD: %v", err)) return } + defer os.RemoveAll(filepath.Dir(argoCDRepoRoot)) // Clean up temp directory fmt.Println("Using Argo CD temporary directory:", argoCDRepoRoot)
354-356: Consider using exact match for closing parenthesis.The current check
strings.HasPrefix(line, ")")could potentially match unintended lines (though unlikely in well-formed go.mod files). A more precise match would be safer.Suggested improvement
- if replaceBlockStart != -1 && strings.HasPrefix(line, ")") { + if replaceBlockStart != -1 && (line == ")" || strings.HasPrefix(line, ") ")) { replaceBlockEnd = x - 1 }
424-434: Error message doesn't reflect the actual failing command.The error message "unable to update argocd-operator go.mod" is misleading since the failure could be from any of the five commands (
go mod tidy,rm,make generate manifests,make bundle, ormake fmt). Also, usefilepath.Joinfor the path on line 427.Suggested improvements
err = runCommandListWithWorkDir(argocdOperatorRoot, [][]string{ {"go", "mod", "tidy"}, - {"rm", "-f", argocdOperatorRoot + "/bin/controller-gen"}, // Erase the controller-gen binary to ensure it is re-downloaded to the correct version + {"rm", "-f", filepath.Join(argocdOperatorRoot, "bin", "controller-gen")}, // Erase the controller-gen binary to ensure it is re-downloaded to the correct version {"make", "generate", "manifests"}, {"make", "bundle"}, {"make", "fmt"}, }) if err != nil { - exitWithError(fmt.Errorf("unable to update argocd-operator go.mod")) + exitWithError(fmt.Errorf("unable to regenerate manifests/bundle: %v", err)) }
489-497: Consider using shallow clone for faster execution.A full clone of the Argo CD repository can be time-consuming. Since only a specific tag is needed, a shallow clone would significantly improve performance.
Suggested improvement
- if _, _, err := runCommandWithWorkDir(tmpDir, "git", "clone", "https://github.com/argoproj/argo-cd"); err != nil { + if _, _, err := runCommandWithWorkDir(tmpDir, "git", "clone", "--depth", "1", "--branch", latestReleaseVersionTag, "https://github.com/argoproj/argo-cd"); err != nil { return "", err } newWorkDir := filepath.Join(tmpDir, "argo-cd") - commands := [][]string{ - {"git", "checkout", latestReleaseVersionTag}, - } - - if err := runCommandListWithWorkDir(newWorkDir, commands); err != nil { - return "", err - } - return newWorkDir, nil
📜 Review details
Configuration used: defaults
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 (10)
.github/workflows/lint.yamlMakefilebuild/util/Dockerfilecommon/defaults.gogo.modhack/update-dependencies-script/README.mdhack/update-dependencies-script/go.modhack/update-dependencies-script/main.gohack/update-dependencies-script/run.shhack/update-dependencies-script/utils.go
🚧 Files skipped from review as they are similar to previous changes (4)
- common/defaults.go
- go.mod
- hack/update-dependencies-script/utils.go
- build/util/Dockerfile
🧰 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)
⏰ 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 end-to-end tests (v1.27.1)
- GitHub Check: build
- GitHub Check: Verify Argo CD has updated dependencies, for the given target version
- GitHub Check: build-operator
- GitHub Check: Run golangci-lint and gosec
🔇 Additional comments (8)
.github/workflows/lint.yaml (1)
32-47: LGTM on the new verification job structure.The job correctly:
- Sets up Go with the project's
go.mod- Runs the dependency update target
- Verifies no unexpected changes via
git diffThis ensures PRs maintain consistent dependency state.
Makefile (2)
8-13: Well-documented version variable.Clear comments explaining the purpose, usage, and format constraints (no 'v' prefix). This helps maintainability.
374-377: This review comment is based on an incorrect assumption about how the script accessesARGO_CD_TARGET_VERSION. The Go script intentionally reads the value directly from the Makefile viareadTargetVersionFromMakefile(), which parses the file looking for the patternARGO_CD_TARGET_VERSION ?=. No environment variable passing or explicit exports are needed—this is the designed behavior documented in the script's README.Likely an incorrect or invalid review comment.
hack/update-dependencies-script/main.go (5)
70-106: LGTM!The version extraction logic is sound. It properly handles the "v" prefix normalization and validates that no comments are present on the version line.
271-317: LGTM!The CRD copy logic includes a sensible sanity check that validates the expected number of CRD files, which will catch unexpected additions or removals upstream.
372-409: LGTM!The replace block copying logic correctly handles the injection of the source comment and preserves the rest of the go.mod file structure.
438-479: LGTM!The replacement logic correctly identifies variable declarations using tab prefix matching and constructs properly formatted replacement lines with version comments.
551-555: LGTM!The struct is well-documented and appropriately scoped for internal use.
Signed-off-by: Jonathan West <jgwest@gmail.com>
jgwest
force-pushed
the
upgrade-dependencies-aug-2025
branch
from
8592bbc to
7cb3694
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @hack/update-dependencies-script/main.go:
- Around line 32-39: The temp directory created by cloneArgoCDRepoIntoTempDir is
never removed; wrap the cleanup with defer immediately after successful clone by
calling os.RemoveAll on filepath.Dir(argoCDRepoRoot) (since argoCDRepoRoot is
filepath.Join(tmpDir, "argo-cd")) so the temp dir is removed when the function
exits; place this defer right after the cloneArgoCDRepoIntoTempDir call (before
any early returns such as exitWithError) to ensure cleanup runs on normal or
error paths.
🧹 Nitpick comments (5)
hack/update-dependencies-script/README.md (1)
10-17: Add language specifiers to fenced code blocks.The fenced code blocks are missing language specifiers, which helps with syntax highlighting and accessibility.
📝 Suggested fix
Example: -``` +```makefile ARGO_CD_TARGET_VERSION ?= 3.1.8Then run the script:
-+bash
make update-dependencieshack/update-dependencies-script/main.go (4)
121-133: Brittle pattern matching.The hardcoded whitespace pattern (
" Name: \"dex\","with 6 tabs) is tightly coupled to the current file formatting. If the test file is reformatted (e.g., different indentation), the match will fail. This is acceptable for an internal automation tool, but consider a more flexible regex-based approach if this becomes an issue.
354-358: Potential parsing bug: premature block end detection.The condition
strings.HasPrefix(line, ")")will match the closing parenthesis of ANY block, not just thereplaceblock. If the go.mod has additional blocks after the replace block starts being parsed, this could incorrectly identify the end.However, since
replaceBlockStartmust be set first (checked on line 355), and the replace block typically appears last in go.mod files, this may work in practice. Consider tightening the condition to avoid surprises.🔧 Suggested fix
- if replaceBlockStart != -1 && strings.HasPrefix(line, ")") { + if replaceBlockStart != -1 && replaceBlockEnd == -1 && strings.TrimSpace(line) == ")" { replaceBlockEnd = x - 1 }This ensures:
- We only look for the end if we haven't found it yet (
replaceBlockEnd == -1)- The line is exactly
)(with optional whitespace), not just starting with)
418-435: Duplicate error messages reduce debuggability.Both error paths (lines 419 and 434) use the same message
"unable to update argocd-operator go.mod", making it harder to identify which command sequence failed.📝 Suggested fix
err := runCommandListWithWorkDir(argocdOperatorRoot, [][]string{ {"go", "get", "github.com/argoproj/argo-cd/v3@" + targetArgoCDVersion}, }) if err != nil { - exitWithError(fmt.Errorf("unable to update argocd-operator go.mod")) + exitWithError(fmt.Errorf("unable to run 'go get' for argo-cd: %v", err)) return } // ... err = runCommandListWithWorkDir(argocdOperatorRoot, [][]string{ {"go", "mod", "tidy"}, // ... }) if err != nil { - exitWithError(fmt.Errorf("unable to update argocd-operator go.mod")) + exitWithError(fmt.Errorf("unable to regenerate manifests/bundle: %v", err)) }
490-498: Consider shallow clone for faster execution.A full clone of the Argo CD repository can be slow. Since only a specific tag is needed, a shallow clone would be faster.
⚡ Suggested optimization
- if _, _, err := runCommandWithWorkDir(tmpDir, "git", "clone", "https://github.com/argoproj/argo-cd"); err != nil { + if _, _, err := runCommandWithWorkDir(tmpDir, "git", "clone", "--depth", "1", "--branch", latestReleaseVersionTag, "https://github.com/argoproj/argo-cd"); err != nil { return "", err } newWorkDir := filepath.Join(tmpDir, "argo-cd") - commands := [][]string{ - {"git", "checkout", latestReleaseVersionTag}, - } - - if err := runCommandListWithWorkDir(newWorkDir, commands); err != nil { - return "", err - } - return newWorkDir, nilThis eliminates the separate checkout step and significantly reduces clone time.
📜 Review details
Configuration used: defaults
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 (10)
.github/workflows/lint.yamlMakefilebuild/util/Dockerfilecommon/defaults.gogo.modhack/update-dependencies-script/README.mdhack/update-dependencies-script/go.modhack/update-dependencies-script/main.gohack/update-dependencies-script/run.shhack/update-dependencies-script/utils.go
🚧 Files skipped from review as they are similar to previous changes (5)
- hack/update-dependencies-script/utils.go
- hack/update-dependencies-script/run.sh
- .github/workflows/lint.yaml
- go.mod
- hack/update-dependencies-script/go.mod
🧰 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)
⏰ 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: Verify Argo CD has updated dependencies, for the given target version
- GitHub Check: build-operator
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build
🔇 Additional comments (12)
build/util/Dockerfile (1)
1-2: LGTM!The Dockerfile correctly updates the Argo CD base image to v3.2.3 with digest pinning. The digest matches the value in
common/defaults.go, ensuring consistency.Makefile (2)
8-13: LGTM! Well-documented version variable.The
ARGO_CD_TARGET_VERSIONvariable is clearly documented with usage instructions. The note about not using the 'v' prefix is helpful for consistency.
373-377: LGTM!The
update-dependenciestarget is properly declared as.PHONYand delegates to the shell script.hack/update-dependencies-script/main.go (8)
70-106: LGTM!The function correctly parses the version from the Makefile and normalizes the version string by adding the 'v' prefix when needed, aligning with the Makefile's documented convention.
149-188: LGTM!The Dockerfile update logic correctly identifies the Argo CD image section by matching the comment and FROM line pattern, then replaces both with updated values.
190-270: LGTM!The function correctly parses the Argo CD install manifest to extract image references, retrieves their SHA256 digests via skopeo, and updates the defaults file. The dependency on
skopeoshould be documented in the README.Consider documenting the
skopeodependency in the README since the script will fail without it installed.
272-318: LGTM!The CRD update logic includes a useful sanity check that will fail if Argo CD changes the number of CRDs, prompting manual review of the mapping.
373-410: LGTM!The function correctly transfers the replace block between go.mod files, adding a helpful comment about the source version.
439-480: LGTM!The function correctly identifies and replaces version constants in the defaults file with appropriate formatting.
507-550: LGTM!The skopeo integration correctly retrieves image digests with appropriate error handling. The
--no-tagsflag is a good choice for performance.
552-556: LGTM!The struct is well-documented with clear field descriptions.
common/defaults.go (1)
72-73: Version update looks consistent.The updated Argo CD image digest for v3.2.3 aligns with the
ARGO_CD_TARGET_VERSIONin the Makefile and the Dockerfile base image. The format follows the existing pattern used for other image versions in this file.
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
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.