-
Notifications
You must be signed in to change notification settings - Fork 36
#224 - Run In Place and Use Provided Startup Script features #334
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
base: develop
Are you sure you want to change the base?
Conversation
…er from its originally installed location
…e plumbing is done just not running script correctly; fixed bug that caused runInPlace servers to be set as copied servers when closing and reopening VSC
… with catalina_opts and the fact that the startup script spawns another window
…tdown scripts! Just need to fix debugging while using startup scripts
…s using the provided scripts
| export const invalidWebappFolder: string = localize('tomcatExt.invalidWebappFolder', 'The folder is not a valid web app to run on Tomcat Server.'); | ||
| export const invalidWarFile: string = localize('tomcatExt.invalidWarFile', 'Please select a .war file.'); | ||
| export const pickFolderToGenerateWar: string = localize('tomcatExt.pickFolderToGenerateWar', 'Please select the folder(s) you want to generate war package'); | ||
| export const serverAlreadyAdded: string = localize('tomcatExt.serverAlreadyAdded', 'That server has already been added to the list.'); |
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.
New error condition when using the Run In Place feature. It was previously impossible to have two server entries pointing to the same CATALINA_BASE.
| Utility.infoTelemetryStep(operationId, 'construct server name'); | ||
| const existingServerNames: string[] = this._tomcatModel.getServerSet().map((item: TomcatServer) => { return item.getName(); }); | ||
| const serverName: string = await Utility.getServerName(tomcatInstallPath, this._tomcatModel.defaultStoragePath, existingServerNames); | ||
| const catalinaBasePath: string = await Utility.getServerStoragePath(this._tomcatModel.defaultStoragePath, serverName); | ||
| await fse.remove(catalinaBasePath); | ||
| await Utility.trackTelemetryStep(operationId, 'copy files', () => Promise.all([ | ||
| fse.copy(path.join(tomcatInstallPath, 'conf'), path.join(catalinaBasePath, 'conf'), {dereference: true}), | ||
| fse.copy(path.join(this._extensionPath, 'resources', 'jvm.options'), path.join(catalinaBasePath, 'jvm.options')), | ||
| fse.copy(path.join(this._extensionPath, 'resources', 'index.jsp'), path.join(catalinaBasePath, 'webapps', 'ROOT', 'index.jsp')), | ||
| fse.copy(path.join(this._extensionPath, 'resources', 'icon.png'), path.join(catalinaBasePath, 'webapps', 'ROOT', 'icon.png')), | ||
| fse.mkdirs(path.join(catalinaBasePath, 'logs')), | ||
| fse.mkdirs(path.join(catalinaBasePath, 'temp')), | ||
| fse.mkdirs(path.join(catalinaBasePath, 'work')) | ||
| ])); | ||
|
|
||
| await Utility.copyServerConfig(path.join(tomcatInstallPath, 'conf', 'server.xml'), path.join(catalinaBasePath, 'conf', 'server.xml')); | ||
| const tomcatServer: TomcatServer = new TomcatServer(serverName, tomcatInstallPath, catalinaBasePath); | ||
| Utility.trackTelemetryStep(operationId, 'add server', () => this._tomcatModel.addServer(tomcatServer)); | ||
| return tomcatServer; |
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.
All of this code was preserved and moved to a separate addServerToWorkspace function below
| private async addInPlaceServer(operationId: string, tomcatInstallPath: string, runInPlace: boolean) : Promise<TomcatServer> { | ||
| Utility.infoTelemetryStep(operationId, 'construct server name'); | ||
| const existingServerNames: string[] = this._tomcatModel.getServerSet().map((item: TomcatServer) => { return item.getName(); }); | ||
| const serverName: string = await Utility.getServerName(tomcatInstallPath, this._tomcatModel.defaultStoragePath, existingServerNames, runInPlace); | ||
| const tomcatServer: TomcatServer = new TomcatServer(serverName, tomcatInstallPath, tomcatInstallPath, runInPlace); | ||
| Utility.trackTelemetryStep(operationId, 'add server', () => this._tomcatModel.addServer(tomcatServer)); | ||
| return tomcatServer; | ||
| } |
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.
This is new code that handles adding a new in-place server.
| } | ||
| startArguments.push('start'); | ||
| const javaProcess: Promise<void> = Utility.executeCMD(this._outputChannel, serverInfo.getName(), Utility.getJavaExecutable(), { shell: true }, ...startArguments); | ||
| process = Utility.executeCMD(this._outputChannel, serverInfo.getName(), Utility.getStartExecutable(serverInfo.getInstallPath()), { env: await this._tomcatModel.getEnvironmentVars(serverInfo, true), shell: true }, true, ...startArguments); |
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.
Notable thing that changed: explicitly supplying the correct CATALINA_HOME, CATALINA_BASE, and JPDA_OPTS environment variables to the shell.
| try { | ||
| await fse.outputJson(this._serversJsonFile, this._serverList.map((s: TomcatServer) => { | ||
| return { _name: s.getName(), _installPath: s.getInstallPath(), _storagePath: s.getStoragePath() }; | ||
| return { _name: s.getName(), _installPath: s.getInstallPath(), _storagePath: s.getStoragePath(), _runInPlace: s.isRunInPlace() }; |
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.
The "Delete server" behavior had to change based on if the server was in-place or copied to the workspace, so I had to add this new param to the TomcatModel
|
|
||
| public async updateJVMOptions(serverName: string) : Promise<void> { | ||
| const server: TomcatServer = this.getTomcatServer(serverName); | ||
| public async updateJVMOptions(server: TomcatServer) : Promise<void> { |
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.
This method didn't semantically change much. Just sent portions of it to separate functions so that the branching changes I implemented were clearer to maintain.
| function spawnWindowsScript(scriptFile: string, args: string[], options: child_process.SpawnOptions): child_process.ChildProcess { | ||
| args.unshift(scriptFile); | ||
| const quotedArgs: string = '"'.concat(args.reduce((accumulator: string, currentVal: string) => accumulator.concat(" ", currentVal), ""), '"'); | ||
| return child_process.spawn(Constants.WINDOWS_CMD, ['/c', quotedArgs], options); | ||
| } |
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.
Windows can't straight spawn a .bat script file. You have to spawn cmd.exe and pass the .bat file as an arg instead.
|
I ran manual tests on all combinations of in-place/copied-to-workspace servers and starting/debugging/stopping manually/with provided tomcat scripts. Everything seems to be in working order to me. |
Implemented two new features: