WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@aufi
Copy link
Member

@aufi aufi commented Nov 20, 2025

Proxy setting with -Djava...Proxies=true was placed in args after jar which was most likely ignored by JVM.

Changing its order to ensure its understood correctly by JVM running jdtls.

Replaces konveyor/java-analyzer-bundle#173

Fixes: konveyor/java-analyzer-bundle#172

Summary by CodeRabbit

  • Bug Fixes
    • Fixed Java application launcher argument ordering to ensure system proxy settings are correctly applied before the application runs.

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

Proxy setting with `-Djava...Proxies=true` was placed in args after `jar`
which was most likely ignored by JVM.

Changing its order to ensure its understood correctly by JVM running jdtls.

Replaces konveyor/java-analyzer-bundle#173

Fixes: konveyor#172

Signed-off-by: Marek Aufart <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Nov 20, 2025

Walkthrough

The Java launcher command in the external provider is modified to ensure JVM system properties are positioned before the -jar argument. This change moves -Djava.net.useSystemProxies=true ahead of -jar jarPath, ensuring proxy settings are applied to the JVM rather than passed to the application.

Changes

Cohort / File(s) Change Summary
Java launcher argument reordering
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Reordered Java command arguments to place -Djava.net.useSystemProxies=true before -jar jarPath, ensuring JVM system properties are correctly processed by the launcher.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Single argument reordering in one file
  • Verify that moving -jar after the system property doesn't introduce unintended side effects in argument parsing

Poem

🐰 A little rabbit hops with glee,
Arguments rearranged, so the JVM runs free!
Proxy settings now in place, before the jar takes flight—
External providers configured just right!

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The PR addresses the core requirement from issue #172 by ensuring proxy settings are positioned correctly in JVM arguments so jdtls can access external resources.
Out of Scope Changes check ✅ Passed The change is narrowly scoped to reordering a single argument in the Java launcher command, directly addressing the linked issue without introducing unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing the order of JVM proxy arguments to ensure the proxy setting is correctly recognized by the JVM launcher.
✨ 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.

@aufi aufi requested a review from pranavgaikwad November 20, 2025 14:37
@aufi aufi requested a review from shawn-hurley November 20, 2025 14:38
@aufi aufi changed the title Fix JVM proxy args passing 🐛 Fix JVM proxy args passing Nov 20, 2025
@aufi aufi added the cherry-pick/release-0.8 This PR should be cherry-picked to release-0.8 branch label Nov 20, 2025
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

If this is working in a test environment, then please ignore this.

But I wanted to know whether we also needed to explicitly add the environment to the command here: https://github.com/konveyor/analyzer-lsp/pull/978/files#diff-2430297ef0a9e63ec1a282c9ba2b450043754ad1efc7fe73cd9b54045e5fa724R409

If we don't have a test environment that can test proxied environments easily/automated, can we can add an issue tracking this in the CI repo?

@shawn-hurley
Copy link
Contributor

@aufi go ahead and merge when you are ready!

@aufi aufi merged commit 1397460 into konveyor:main Nov 26, 2025
19 of 21 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 26, 2025
Proxy setting with `-Djava...Proxies=true` was placed in args after
`jar` which was most likely ignored by JVM.

Changing its order to ensure its understood correctly by JVM running
jdtls.

Replaces konveyor/java-analyzer-bundle#173

Fixes: konveyor/java-analyzer-bundle#172

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Fixed Java application launcher argument ordering to ensure system
proxy settings are correctly applied before the application runs.

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

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Marek Aufart <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick/release-0.8 This PR should be cherry-picked to release-0.8 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proxy settings from env need to be passed to JVM for jdtls

2 participants