🐛 Add cmd for gradle version #911
Conversation
Signed-off-by: Emily McMullan <[email protected]>
WalkthroughThe Gradle version detection now retries on failure by re-running the command with JAVA_HOME (instead of JAVA8_HOME) while preserving the working directory. This introduces an alternative environment configuration before returning an error. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ServiceClient
participant Gradle
Caller->>ServiceClient: GetGradleVersion()
activate ServiceClient
note over ServiceClient: Attempt 1 (env uses JAVA8_HOME)
ServiceClient->>Gradle: gradle -v
alt Success
Gradle-->>ServiceClient: Version output
ServiceClient-->>Caller: Parsed version
else Failure
note over ServiceClient: Retry with JAVA_HOME, same args & cwd
ServiceClient->>Gradle: gradle -v (env uses JAVA_HOME)
alt Success
Gradle-->>ServiceClient: Version output
ServiceClient-->>Caller: Parsed version
else Failure
ServiceClient-->>Caller: Error
end
end
deactivate ServiceClient
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
eemcmullan
added
the
cherry-pick/release-0.8
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (3)
479-481: Fix the comment: Gradle 8.x requires Java 17; Java 8 works for 7.x, not 8.14.The current comment is misleading and may confuse future maintenance.
Apply this minimal change:
-// up to 8.14 is compatible with Java 8, so let's first try to run with that +// Gradle 7.x is compatible with Java 8; Gradle 8+ requires Java 17. +// Try with Java 8 first, then fall back to Java 17.
511-512: Don’t return nil error when version parsing fails.Returning a zero value version with nil error can mis-route later logic (e.g., defaulting to Java 8). Return an explicit error.
Apply this change:
- return version.Version{}, nil + return version.Version{}, fmt.Errorf("unable to parse Gradle version from output")
519-527: Correct the compatibility cutoff: Gradle 8+ requires Java 17.Using 8.14 as “last version for Java 8” is incorrect and will select Java 8 for Gradle 8.x (which requires Java 17). This can cause repeated failures.
Suggested fix:
- lastVersionForJava8, _ := version.NewVersion("8.14") - if v.LessThanOrEqual(lastVersionForJava8) { + minGradleRequiringJava17, _ := version.NewVersion("8.0.0") + if v.LessThan(minGradleRequiringJava17) { java8home := os.Getenv("JAVA8_HOME") if java8home == "" { return "", fmt.Errorf("couldn't get JAVA8_HOME environment variable") } return java8home, nil } - return os.Getenv("JAVA_HOME"), nil + javaHome := os.Getenv("JAVA_HOME") + if javaHome == "" { + return "", fmt.Errorf("couldn't get JAVA_HOME environment variable") + } + return javaHome, nil
🧹 Nitpick comments (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
479-496: Guard against unset JAVA8_HOME/JAVA_HOME and avoid a doomed first exec.If the env var is empty, we currently execute with JAVA_HOME="", which is noisy and wastes time. Guard and use a deterministic fallback.
Proposed inline refactor:
args := []string{ "--version", } cmd := exec.CommandContext(ctx, exe, args...) cmd.Dir = s.config.Location - cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", os.Getenv("JAVA8_HOME"))) - output, err := cmd.CombinedOutput() + var output []byte + j8 := os.Getenv("JAVA8_HOME") + if j8 != "" { + cmd.Env = []string{fmt.Sprintf("JAVA_HOME=%s", j8)} + output, err = cmd.CombinedOutput() + } else { + err = fmt.Errorf("JAVA8_HOME is not set") + } if err != nil { // if executing with 8 we get an error, try with 17 - cmd = exec.CommandContext(ctx, exe, args...) - cmd.Dir = s.config.Location - cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", os.Getenv("JAVA_HOME"))) - output, err = cmd.CombinedOutput() + cmd = exec.CommandContext(ctx, exe, args...) + cmd.Dir = s.config.Location + j17 := os.Getenv("JAVA_HOME") + if j17 == "" { + return version.Version{}, fmt.Errorf("JAVA_HOME is not set") + } + cmd.Env = []string{fmt.Sprintf("JAVA_HOME=%s", j17)} + output, err = cmd.CombinedOutput() if err != nil { return version.Version{}, fmt.Errorf("error trying to get Gradle version: %w - Gradle output: %s", err, string(output)) } }Note: This keeps the maintainers’ preference to not preserve the ambient environment (no os.Environ()).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jmle
PR: konveyor/analyzer-lsp#888
File: external-providers/java-external-provider/pkg/java_external_provider/service_client.go:507-521
Timestamp: 2025-08-21T12:42:54.499Z
Learning: In the konveyor/analyzer-lsp Java external provider (external-providers/java-external-provider/pkg/java_external_provider/service_client.go), the maintainers prefer to keep the current JAVA_HOME selection logic in GetJavaHomeForGradle method simple, as they operate in a controlled environment where both Java 8 and Java 17+ are guaranteed to be available, with Java 17+ being a system requirement for running the analyzer.
📚 Learning: 2025-08-21T12:42:54.499Z
Learnt from: jmle
PR: konveyor/analyzer-lsp#888
File: external-providers/java-external-provider/pkg/java_external_provider/service_client.go:507-521
Timestamp: 2025-08-21T12:42:54.499Z
Learning: In the konveyor/analyzer-lsp Java external provider (external-providers/java-external-provider/pkg/java_external_provider/service_client.go), the maintainers prefer to keep the current JAVA_HOME selection logic in GetJavaHomeForGradle method simple, as they operate in a controlled environment where both Java 8 and Java 17+ are guaranteed to be available, with Java 17+ being a system requirement for running the analyzer.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
📚 Learning: 2025-08-21T12:39:46.778Z
Learnt from: jmle
PR: konveyor/analyzer-lsp#888
File: external-providers/java-external-provider/pkg/java_external_provider/provider.go:662-663
Timestamp: 2025-08-21T12:39:46.778Z
Learning: In the konveyor/analyzer-lsp Java provider, when setting JAVA_HOME for Gradle commands using exec.CommandContext, the maintainers prefer not to preserve the existing environment variables (including PATH) and use `cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome))` instead of `cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))`.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
⏰ 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: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
490-493: Good fallback retry with JAVA_HOME; working directory preserved.Nice touch re‑initializing the command and re‑setting Dir before retrying. This should handle Gradle 8+ requiring Java 17.
Please confirm that both JAVA8_HOME and JAVA_HOME are guaranteed to be set in the target runtime (containers/agents). If either can be unset, see my follow‑up suggestion to guard against empty values.
jmle
removed
the
cherry-pick/release-0.8
2 participants
Summary by CodeRabbit