Add support for native connector packaging #30
Conversation
daniel-chambers
force-pushed
the
daniel/native-packaging
branch
from
1c04ba8 to
260709c
Compare
There was a problem hiding this comment.
Looks good @daniel-chambers
I'd have the node version constraints defined somewhere common rather than hard-coded in multiple scripts, but not a big deal.
It would be a good goal to minimise the amount of platform specific script logic, moving as much logic prior and post script invocation as possible. I guess that is what you've done but just keeping an eye out for oportunities to take this further would be great. My fear is that other authors might actually bifurcate a lot of their connector logic into complex platform specific scripts and this could serve as a good example of how to do it right.
Approved. Feel free to discuss comments if you like.
| script_files := $(wildcard scripts/*) | ||
| dist_script_files := $(patsubst scripts/%,dist/.hasura-connector/%,$(script_files)) | ||
|
|
||
| $(dist_script_files): $(script_files) | ||
| cp -f $(script_files) dist/.hasura-connector/ |
| nativeToolchainDefinition: | ||
| commands: | ||
| start: | ||
| type: ShellScript | ||
| bash: ./start.sh | ||
| powershell: ./start.ps1 | ||
| watch: | ||
| type: ShellScript | ||
| bash: ./watch.sh | ||
| powershell: ./watch.ps1 |
There was a problem hiding this comment.
Yeah I quite like that this specifies exactly which shell it's using rather than the platform.
| $nodeVersion = & node --version | ||
| if ($nodeVersion -match "^v(\d+)\.") { | ||
| $majorVersion = $Matches[1] | ||
| if ($majorVersion -lt $minimumNodeVersion) { | ||
| Write-Host "Detected Node.js version $nodeVersion on the PATH. The minimum required version is v$minimumNodeVersion." | ||
| exit 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Should this be specified as a constant somewhere so that is can be shared between scripts, etc?
There was a problem hiding this comment.
Yeah potentially, but unfortunately the node version (or something linked to the node version) is used in a lot of disparate places across the codebase that can't be unified into a single configurable value (eg. package.json, node ts types version, .nvm, Dockerfiles, tsconfig templates, tsconfig defaults inside the code, etc).
I could unify it in the scripts, but it would mean having to read the value from a file, which seems like more script complexity to simply reduce two tiny definitions into one. It doesn't feel worth it, given the load of other places that cannot use this value and still have to be corrected by hand. If you feel strongly that it is worth it for these two places, I can change it very easily.
| if ((Test-Path "./node_modules") -eq $false) { | ||
| Write-Host "node_modules not found, please ensure you have run 'npm install'." | ||
| exit 1 | ||
| } |
| #!/usr/bin/env bash | ||
| set -eu -o pipefail | ||
|
|
||
| minimum_node_version="20" |
| // This script reads from the package.json file in the current working directory | ||
| // and prints out the text in the "scripts.<name>" entry. This is used to bypass | ||
| // npm, which does not handle signals correctly, and execute the command directly |
There was a problem hiding this comment.
Are there any open issues about this upstream? Seems so dumb.
There was a problem hiding this comment.
Last time I looked there were people complaining about this, and maybe it had been fixed in a very recent version of npm, but then someone else reported that it was still broken in an even newer version...
I just tested it again on the latest available version and it still appears broken. 🤷♂️
|
OK it’s not a big deal.
…On Wed, 17 Apr 2024 at 10:22 AM, Daniel Chambers ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In connector-definition/scripts/check-reqs.ps1
<#30 (comment)>
:
> +$nodeVersion = & node --version
+if ($nodeVersion -match "^v(\d+)\.") {
+ $majorVersion = $Matches[1]
+ if ($majorVersion -lt $minimumNodeVersion) {
+ Write-Host "Detected Node.js version $nodeVersion on the PATH. The minimum required version is v$minimumNodeVersion."
+ exit 1
+ }
+}
Yeah potentially, but unfortunately the node version (or something linked
to the node version) is used in a lot of disparate places across the
codebase that can't be unified into a single configurable value (eg.
package.json, node ts types version, .nvm, Dockerfiles, tsconfig templates,
tsconfig defaults inside the code, etc).
I could unify it in the scripts, but it would mean having to read the
value from a file, which seems like more script complexity to simply reduce
two tiny definitions into one. It doesn't feel worth it, given the load of
other places that cannot not use this value and still have to be corrected
by hand. If you feel strongly that it *is* worth it for these two places,
I can change it very easily.
—
Reply to this email directly, view it on GitHub
<#30 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAWRC6I36DL6R5ERBNFKFTY5W6EDAVCNFSM6AAAAABGBXXDMGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMBUG44DENZQGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
This PR adds support for Native Connector Packaging, allowing
ddnto run the connector using NodeJS installed on the end-user's machine, rather than running it in a docker container.There are three scripts created, in both bash and PowerShell:
check-reqs: This checks the user's local environment to ensure nodejs is installed, ensure they have the correct version installed, and a smoke test ofnode_modulesto make sure they've initialized their project correctly withnpm install(standard Node project practice)start: Checks requirements usingcheck-reqsand then starts the connector using thestartscriptwatch: Checks requirements usingcheck-reqsand then starts the connector using thewatchscriptThere is also a
read-package-script.jsfile. This is used to read thescriptsdefined in the user'spackage.jsonand extract them.startandwatchuse this to get the command to use to start/watch from thepackage.json. This is done using this NodeJS script file in order to avoid requiring the user to installjq.startandwatchdon't just runnpm run <command>because npm does not handle signals correctly. TheddnCLI will sendSIGTERMto the script (on *nix systems) when it wants the connector to shut down.npmswallows this. So the scriptsexecthe appropriate command directly, ensuring that the node process itself is the one to receive theSIGTERMandnpmisn't involved.These scripts are copied into the
.hasura-connectorfolder in the connector definition tarball, and are referenced in the newnativeToolchainDefinitionin theconnector-metadata.yaml.