Skip to content

Conversation

@jgwest
Copy link
Member

@jgwest jgwest commented Sep 19, 2025

What type of PR is this?
/kind chore

What does this PR do / why we need it:

  • This script is an experimental utility that will automatically upgrade the various downstream dependencies that are referenced in the repository, including go.mod and various container images.
  • I've added it as a PR check, but if we find it's breaking other PRs then we can disable it.

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Summary by CodeRabbit

  • New Features

    • Added make update-dependencies command to update Argo CD dependencies and related artifacts.
  • Chores

    • Introduced automated dependency update script with supporting utilities.
    • Added ARGO_CD_TARGET_VERSION configuration variable to Makefile.
    • Added CI/CD verification job to validate dependency updates.
    • Renamed workflow to "Validation, verifications, and lints".

✏️ Tip: You can customize this high-level summary in your review settings.

@jgwest jgwest force-pushed the upgrade-dependencies-aug-2025 branch 2 times, most recently from 37bc3ba to 82325f7 Compare October 10, 2025 16:50
@jgwest jgwest marked this pull request as ready for review October 10, 2025 16:50
@jgwest jgwest force-pushed the upgrade-dependencies-aug-2025 branch 2 times, most recently from 770c447 to 7842460 Compare October 15, 2025 15:35
@jgwest jgwest force-pushed the upgrade-dependencies-aug-2025 branch from 7842460 to b4fa01d Compare November 10, 2025 19:14
@coderabbitai
Copy link

coderabbitai bot commented Nov 10, 2025

Walkthrough

This PR introduces a comprehensive dependency update automation system for the operator. It adds a GitHub Actions workflow job to verify dependencies remain current, a Makefile target to execute updates, and a Go-based script that upgrades Argo CD artifacts including container images, CRDs, Go modules, manifests, and test references.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow
.github/workflows/lint.yaml
Renames workflow to "Validation, verifications, and lints"; adds verify_argo_cd_has_updated_dependencies job to checkout repository and run make update-dependencies, verifying no unexpected diff in artifacts (ignoring createdAt lines).
Makefile Configuration
Makefile
Adds ARGO_CD_TARGET_VERSION variable (default 3.1.9) with upgrade guidance; introduces runtime check for container runtime (Docker/Podman) with warning instead of failure; adds .PHONY declaration and public update-dependencies target wiring to hack/update-dependencies-script/run.sh.
Dependency Script Documentation
hack/update-dependencies-script/README.md
Documents the Go-based script for upgrading upstream Argo CD dependencies; provides instructions for setting ARGO_CD_TARGET_VERSION and running via make update-dependencies.
Dependency Script Go Module
hack/update-dependencies-script/go.mod
Defines Go module with dependencies on github.com/google/go-github/v58, gopkg.in/yaml.v2, and transitive go-querystring; Go version 1.20.
Dependency Script Implementation
hack/update-dependencies-script/main.go
hack/update-dependencies-script/utils.go
hack/update-dependencies-script/run.sh
Main Go program that automates Argo CD dependency upgrades by reading target version from Makefile, cloning Argo CD repository, and updating container images in common/defaults.go, CRD YAMLs, go.mod, manifests, bundle, Dockerfile, and test references. Includes utilities for command execution, file operations, and image digest handling; shell wrapper invokes Go program.

Sequence Diagram

sequenceDiagram
    actor GHA as GitHub Actions
    participant WF as Workflow Job
    participant Make as make target
    participant Script as Go Script
    participant ArgoCDRepo as Argo CD Repo
    participant OperatorRepo as Operator Repo Files

    GHA->>WF: Trigger verify_argo_cd_has_updated_dependencies
    WF->>Make: Execute make update-dependencies
    Make->>Script: Execute hack/update-dependencies-script/run.sh
    Script->>Script: Read ARGO_CD_TARGET_VERSION from Makefile
    Script->>ArgoCDRepo: Clone Argo CD at target version
    ArgoCDRepo-->>Script: Cloned repository
    Script->>OperatorRepo: Update container images in common/defaults.go
    Script->>OperatorRepo: Copy CRD YAMLs from cloned Argo CD
    Script->>OperatorRepo: Update go.mod and sync versions
    Script->>OperatorRepo: Regenerate manifests and bundle
    Script->>OperatorRepo: Update Dockerfile image reference
    Script->>OperatorRepo: Update dex_test.go image reference
    Script-->>Make: Return control
    Make-->>WF: Script completes
    WF->>WF: Verify no unexpected diffs (ignore createdAt)
    WF-->>GHA: Job result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

  • main.go logic: Multiple responsibilities including version parsing, Argo CD repository cloning, and coordinated updates across container images, CRDs, Go modules, manifests, and test files—each requiring verification of correctness
  • Error handling and edge cases: Script includes guidance messages and error handling that should be validated for robustness
  • Container image digest handling: Implementation details around digest inspection and propagation through the internal processedContainerImage type warrant careful review
  • File operations and transformations: Replace and update routines for various artifact sources need validation to ensure data integrity

Possibly related PRs

  • chore: Upgrade to Argo CD v3.1.9 #1935: Applies Argo CD v3.1.9 dependency updates to multiple artifacts (Dockerfile, defaults, go.mod, tests), which directly reflects the output expected from running this new automation script.

Poem

🐰 Dependencies dance, kept fresh and keen,
Scripts hop from file to file with routine,
Versions aligned, no surprises unseen—
Argo CD blessed, our garden stays green!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a script to update ArgoCD dependencies and images, which aligns with the primary objective of the PR.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 status as 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 specifies go 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.8

Then run the script:
- +bash
make update-dependencies

Also 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.sh

Alternatively, if you prefer the quoted form, ensure it's properly quoted:

-	"hack/update-dependencies-script/run.sh"
+	./hack/update-dependencies-script/run.sh

Note: 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 exitWithError calls os.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

📥 Commits

Reviewing files that changed from the base of the PR and between 2a10f87 and b4fa01d.

⛔ Files ignored due to path filters (2)
  • go.sum is excluded by !**/*.sum
  • hack/update-dependencies-script/go.sum is 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-lines for createdAt is 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.ArgoCDDefaultArgoVersion instead 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_VERSION variable 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
}

Comment on lines +379 to +398
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 {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

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

Comment on lines +526 to +542
// 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),
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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 1

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

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

Comment on lines +98 to +115
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
}
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

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

Comment on lines +133 to +142
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")

}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

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

@jgwest jgwest force-pushed the upgrade-dependencies-aug-2025 branch from b4fa01d to fc41142 Compare November 25, 2025 11:45
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 cd without error handling. This issue was previously flagged and should be fixed before merge.

Apply this diff:

-cd $SCRIPT_DIR
+cd "$SCRIPT_DIR" || exit 1

This 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

📥 Commits

Reviewing files that changed from the base of the PR and between b4fa01d and fc41142.

⛔ Files ignored due to path filters (1)
  • hack/update-dependencies-script/go.sum is 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.sh and 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., trap to kill the background process on exit).

Comment on lines +374 to +377
# Updates upstream dependencies throughout the repository
.PHONY: update-dependencies
update-dependencies:
"hack/update-dependencies-script/run.sh"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.sh

Alternatively, 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.sh

However, 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.

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc41142 and 96e21a5.

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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.sh
hack/df-loop.sh (1)

1-10: Add signal handling for graceful CI process termination (previously noted, still unaddressed).

The script lacks signal handlers for SIGTERM and SIGINT, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 96e21a5 and 374026b.

📒 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-dependencies target 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 374026b and cce303b.

📒 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 sum ad21c0d2be495a8f6cfc0821e592ae61afd191ebd453156d9b204e2524fceaf2 matches the official KUTTL v0.10.0 kubectl-kuttl_0.10.0_linux_x86_64 binary. Version and checksum are consistent.


func main() {

wd, err := os.Getwd()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@jgwest jgwest marked this pull request as draft November 26, 2025 15:17
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93d5530 and 03da1e9.

📒 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 strconv import 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 in waitForRootPartitionToHaveMinimumDiskSpace() are resolved first.

@jgwest jgwest force-pushed the upgrade-dependencies-aug-2025 branch 2 times, most recently from cd73093 to 40bdac1 Compare November 27, 2025 15:20
@jgwest jgwest force-pushed the upgrade-dependencies-aug-2025 branch from b0310e1 to a2db378 Compare December 17, 2025 12:32
@jgwest jgwest marked this pull request as ready for review December 17, 2025 12:34
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

♻️ Duplicate comments (7)
hack/update-dependencies-script/run.sh (1)

5-5: Add error handling for directory change.

The cd command 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 1
Makefile (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.sh
hack/update-dependencies-script/main.go (5)

121-133: Prevent index overrun when matching dex image block.

lines[idx+1] is accessed without confirming that idx+1 is 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).

readReplaceBlockFromGoMod returns indices where replaceBlockEnd points to the last line of content. Slicing with [replaceBlockStart:replaceBlockEnd] excludes the final replace entry. Use replaceBlockEnd+1 to 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 skopeo omits the Digest key 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 return statement after exitWithError. 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 defer in main() 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

📥 Commits

Reviewing files that changed from the base of the PR and between 03da1e9 and a2db378.

⛔ Files ignored due to path filters (1)
  • hack/update-dependencies-script/go.sum is 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_VERSION variable is well-documented with clear instructions about the version format (no v prefix) and the follow-up action (make update-dependencies).

Comment on lines +36 to +40
- name: Checkout code
uses: actions/checkout@v5
- name: "Call update dependencies Makefile target"
run: |
make update-dependencies
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add Go setup step and use consistent checkout version.

The new job is missing the actions/setup-go step required by the dependency update script (which runs go run .). Also, this job uses actions/checkout@v5 while lint_code uses @v6.

Apply this diff:

   verify_argo_cd_has_updated_dependencies:
     name: Verify Argo CD has updated dependencies, for the given target version
     runs-on: ubuntu-latest
     steps:
       - name: Checkout code
-        uses: actions/checkout@v5
+        uses: actions/checkout@v6
+      - name: Set up Go
+        uses: actions/setup-go@v6
+        with:
+          go-version-file: 'go.mod'
       - name: "Call update dependencies Makefile target"
         run: |
           make update-dependencies
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Checkout code
uses: actions/checkout@v5
- name: "Call update dependencies Makefile target"
run: |
make update-dependencies
- name: Checkout code
uses: actions/checkout@v6
- name: Set up Go
uses: actions/setup-go@v6
with:
go-version-file: 'go.mod'
- name: "Call update dependencies Makefile target"
run: |
make update-dependencies
🤖 Prompt for AI Agents
.github/workflows/lint.yaml around lines 36 to 40: the job currently checks out
the repo with actions/checkout@v5 and runs make update-dependencies without
setting up Go; add an actions/setup-go step before running the Make target and
update the checkout step to actions/checkout@v6 to match lint_code. Ensure
setup-go is configured with the repository's Go version (or a sensible default)
and appears before the make update-dependencies run.

Comment on lines +78 to +79
exitWithError(fmt.Errorf("unable to dex_test.go: %v", err))
return ""
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix incorrect error message.

The error message is truncated/incorrect. It should describe what file failed to read.

Apply this diff:

-		exitWithError(fmt.Errorf("unable to dex_test.go: %v", err))
+		exitWithError(fmt.Errorf("unable to read Makefile: %v", err))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
exitWithError(fmt.Errorf("unable to dex_test.go: %v", err))
return ""
exitWithError(fmt.Errorf("unable to read Makefile: %v", err))
return ""
🤖 Prompt for AI Agents
In hack/update-dependencies-script/main.go around lines 78-79, the error message
passed to exitWithError is incorrect/truncated; change the fmt.Errorf string to
clearly state the file read failure (e.g. "unable to read dex_test.go: %v") so
the log accurately describes which file failed to read, and keep the existing
error value substitution.

Comment on lines +142 to +144
if err := os.WriteFile(fileToUpdate, []byte(newContent), 0600); err != nil {
exitWithError(fmt.Errorf("unable to update build/util/Dockefile: %v", err))
return
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix typo in error message.

The error message has a typo: "Dockefile" should be "Dockerfile". Also, the message references build/util/Dockerfile but the function is updating dex_test.go.

Apply this diff:

 	if err := os.WriteFile(fileToUpdate, []byte(newContent), 0600); err != nil {
-		exitWithError(fmt.Errorf("unable to update build/util/Dockefile: %v", err))
+		exitWithError(fmt.Errorf("unable to update dex_test.go: %v", err))
 		return
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if err := os.WriteFile(fileToUpdate, []byte(newContent), 0600); err != nil {
exitWithError(fmt.Errorf("unable to update build/util/Dockefile: %v", err))
return
if err := os.WriteFile(fileToUpdate, []byte(newContent), 0600); err != nil {
exitWithError(fmt.Errorf("unable to update dex_test.go: %v", err))
return
🤖 Prompt for AI Agents
In hack/update-dependencies-script/main.go around lines 142-144, the error
string contains a typo ("Dockefile") and references the wrong hard-coded path;
change the fmt.Errorf call to use the actual target filename instead of
"build/util/Dockerfile" and fix the typo by formatting the message with the
fileToUpdate variable (e.g. "unable to update %s: %v", fileToUpdate, err) so the
error reports the correct file name and error details.

make update-dependencies
```

See `hack/update-dependencies-script/main.go` for list dependencies that are updated. No newline at end of file
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Minor grammar fix.

Missing word "of" in the sentence.

Apply this diff:

-See `hack/update-dependencies-script/main.go` for list dependencies that are updated.
+See `hack/update-dependencies-script/main.go` for list of dependencies that are updated.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
See `hack/update-dependencies-script/main.go` for list dependencies that are updated.
See `hack/update-dependencies-script/main.go` for list of dependencies that are updated.
🤖 Prompt for AI Agents
hack/update-dependencies-script/README.md around line 19: the sentence "See
`hack/update-dependencies-script/main.go` for list dependencies that are
updated." is missing the word "of"; update the line to read "See
`hack/update-dependencies-script/main.go` for list of dependencies that are
updated." to fix the grammar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants