-
Notifications
You must be signed in to change notification settings - Fork 5
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
add forked docker process #39
Merged
Merged
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
7a85d69
add forked docker process
Courey 6124093
initiate wait and exit process upon stopping
Courey 6a50c05
correct spelling
Courey 18d932a
creating small launch function to await asyn call
Courey 8db69a9
make esbuild file with both entrypoints
Courey f6cdc96
resolving on process termination instead of docker container exit
Courey 8d88bfb
using json string argv
Courey b40c77f
removing unused import
Courey bd153a2
re-adding message handling to fork
Courey 464c8e4
changing target so we can await at top level
Courey 735c443
remove unused type
Courey 7d51368
handle symlinked and npm installed directory name
Courey 7790ae7
do not worry about symlinked paths
Courey df58b89
make it all one file again
Courey b528592
revert package.json
Courey 2e5a890
add fixme
Courey efa8ed1
revisions post code review
Courey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/*! | ||
* Copyright © 2023 United States Government as represented by the | ||
* Administrator of the National Aeronautics and Space Administration. | ||
* All Rights Reserved. | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import Dockerode from 'dockerode' | ||
|
||
const [, , argv] = process.argv | ||
const { dataDir, logsDir, engine, port, options } = JSON.parse(argv) | ||
|
||
async function launchDocker() { | ||
const Image = | ||
engine === 'elasticsearch' | ||
? 'elastic/elasticsearch:8.6.2' | ||
: 'opensearchproject/opensearch:2.11.0' | ||
console.log('Launching Docker container', Image) | ||
const docker = new Dockerode() | ||
|
||
const container = await docker.createContainer({ | ||
Env: [...options, 'path.data=/var/lib/search', 'path.logs=/var/log/search'], | ||
HostConfig: { | ||
AutoRemove: true, | ||
Mounts: [ | ||
{ Source: dataDir, Target: '/var/lib/search', Type: 'bind' }, | ||
{ Source: logsDir, Target: '/var/log/search', Type: 'bind' }, | ||
], | ||
PortBindings: { | ||
[`${port}/tcp`]: [{ HostIP: '127.0.0.1', HostPort: `${port}` }], | ||
}, | ||
}, | ||
Image, | ||
}) | ||
const stream = await container.attach({ stream: true, stderr: true }) | ||
stream.pipe(process.stderr) | ||
await container.start() | ||
return container | ||
} | ||
|
||
const dockerContainer = await launchDocker() | ||
|
||
const signals = ['message', 'SIGTERM', 'SIGINT'] | ||
signals.forEach((signal) => { | ||
process.on(signal, async () => { | ||
await dockerContainer.kill() | ||
process.exit(0) | ||
}) | ||
}) | ||
|
||
await dockerContainer.wait() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
{ | ||
"extends": "@tsconfig/node14/tsconfig.json", | ||
"compilerOptions": { | ||
"resolveJsonModule": true | ||
"resolveJsonModule": true, | ||
"module": "esnext", | ||
"target": "es2022" | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
And of course add
import { join } from 'node:path'
.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.
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.
ah, I was one version behind. so if your local is not using the proper node version it's undefined.
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.
do you know if most people are using nvm? because we can prevent it from being undefined because of the wrong version if we add an .nvmrc file. That tells nvm what version to use for the project. But that would only be effective if people are using nvm.
Because if I use import.meta.dirname and they are not at the proper version, doing it this way will break whereas the other way would work regardless of node version.
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, I don't think we can expect that everyone is using NVM. What are you trying to do? Is
importlib.meta.dirname
only supported in recent Node.js versions? What is the minimum version that you need?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.
upon further testing, the way that requires a node version of 20.11 doesn't seem to be an option right now.
I just ran into something interesting. As soon as I updated my node version to be 20.11.0, in order to terminate the process I started having to hit control + c twice. I changed my architect-plugin-search branch back to main, re-ran
npm prepare
and ensured that the package had updated in gcn. I then ran GCN at main after annpm install
. I had to control + c twice to exit the process.when I roll my version back to v20.9.0 I can quit out of the process with one control + c.
To be clear, for the import.meta.dirname to work GCN would have to be run using node version >= 20.11 and it is when I am running GCN with version 20.11.0 that I encounter this issue.
I haven't looked into what is making the process stick around in version v20.11.0 yet, so I don't know why it's doing it. Just that it happens with both branches on main if my version is 20.11.0.
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.
Alright.
import.meta.url
may be a better option, then. That has been supported at least as far back as Node.js 18. Please useimport.meta.url
.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.
that brings us back to this part of my other comment (cutting and pasting it here since it's relevant in this thread too):
import.meta.url
isarchitect-plugin-search/index.js
so that can't be used directly. Here is what I'm doing there and why it needs to have multiple steps:file:///Users/courey/dev/architect-plugin-search/index.js
this is getting the name of the file that is currently running. in this case it'sindex.js
. I don't wantindex.js
, I wantlaunchSearch.js
.fileURLToPath
so__filename
would then be:/Users/courey/dev/architect-plugin-search/index.js
. This is preparing it so that we can use path.fileURLToPath
we can usepath.dirname()
to get just the path of the directory instead of the file that is running. that makes__dirname
:/Users/courey/dev/architect-plugin-search
/launchSearch.js
to the end of that path to have the proper path and proper file name.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.
We can make this a lot simpler. There is no need to compute the path. Just bundle a single entry point with esbuild, as we did originally. Then launch the subprocess like this:
At file scope, add this code: