-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support to specify custom environment variables #318
Conversation
can this please get merged because I would love to be able to use VS code to do my back end work as well |
Hi. I would also really like this to be merged. There seems to be absolutely no way to set the JDK version for tomcat servers on VS Code. |
really need this feature! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it LGTM, but with some nits in implementation.
package.json
Outdated
}, | ||
"default": [], | ||
"description": "Specifies an array of environment variable names and values. These environment variable values will be passed to java when tomcat starts.", | ||
"scope": "resource" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scope shoud be "window" based on your implementation, as you don't pass uri of resource when reading configuration. But regardless of implementation, I prefer "resource" for this feature, in case you have multiple workspace folders open in the same window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to support different envs per workspace folder, you should provide a URI when calling vscode.workspace.getConfiguration
src/Utility.ts
Outdated
@@ -214,4 +214,26 @@ export namespace Utility { | |||
} | |||
return javaPath ? javaPath + '/bin/java' : 'java'; | |||
} | |||
|
|||
export function getJavaEnvironment(options?: child_process.SpawnOptions): child_process.SpawnOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to bring options
in.
export function getCustomEnv(): { [key: string]: string }
would be simple and clear.
src/Tomcat/TomcatController.ts
Outdated
@@ -157,7 +157,7 @@ export class TomcatController { | |||
} | |||
server.needRestart = restart; | |||
await Utility.trackTelemetryStep(operationId, restart ? 'restart' : 'stop', () => | |||
Utility.executeCMD(this._outputChannel, server.getName(), Utility.getJavaExecutable(), { shell: true }, ...server.jvmOptions.concat('stop'))); | |||
Utility.executeCMD(this._outputChannel, server.getName(), Utility.getJavaExecutable(), Utility.getJavaEnvironment({ shell: true }), ...server.jvmOptions.concat('stop'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to pass spawnOptions when you calculate custom environment variables. Something like Utility.executeCMD(.... {shell: true, env: Utility.getCustomEnv()}, ....)
would be clearer.
src/Utility.ts
Outdated
} | ||
|
||
const { env = { ...process.env } } = options || {}; | ||
const customEnv: { [key: string]: string } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, config.foreach
might be more human-readable. But anyway reduce
also works and that's not a big problem.
@jppurcell9 do you have time to update this PR? Or I can help to update this PR. |
@Eskibear Thanks for taking a look, but I don't have time to update this now. If you can help out, that would be great. Thanks! |
Signed-off-by: Yan Zhang <[email protected]>
Renamed this setting to If you want to specify the JDK used to launch tomcat server, you can set custom "tomcat.customEnv": [
{
"environmentVariable": "JAVA_HOME",
"value": "C:\\Program Files\\Microsoft\\jdk-17.0.0.35-hotspot"
}
] |
Add a config option to set environment vars when spawning the java process. This lets you set
JAVA_HOME
to point to a different JVM, for example. The structure was copied from the maven plugin and looks like this in yoursettings.json
file:Fixes #317 and #239