Add support for native connector packaging #30

daniel-chambers · 2024-04-11T07:09:25Z

This PR adds support for Native Connector Packaging, allowing ddn to 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 of node_modules to make sure they've initialized their project correctly with npm install (standard Node project practice)
start: Checks requirements using check-reqs and then starts the connector using the start script
watch: Checks requirements using check-reqs and then starts the connector using the watch script

There is also a read-package-script.js file. This is used to read the scripts defined in the user's package.json and extract them. start and watch use this to get the command to use to start/watch from the package.json. This is done using this NodeJS script file in order to avoid requiring the user to install jq.
start and watch don't just run npm run <command> because npm does not handle signals correctly. The ddn CLI will send SIGTERM to the script (on *nix systems) when it wants the connector to shut down. npm swallows this. So the scripts exec the appropriate command directly, ensuring that the node process itself is the one to receive the SIGTERM and npm isn't involved.

These scripts are copied into the .hasura-connector folder in the connector definition tarball, and are referenced in the new nativeToolchainDefinition in the connector-metadata.yaml.

sordina

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.

sordina · 2024-04-16T23:08:10Z

connector-definition/Makefile

+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/


sordina · 2024-04-16T23:08:51Z

connector-definition/connector-metadata.yaml

+nativeToolchainDefinition:
+  commands:
+    start:
+      type: ShellScript
+      bash: ./start.sh
+      powershell: ./start.ps1
+    watch:
+      type: ShellScript
+      bash: ./watch.sh
+      powershell: ./watch.ps1


Yeah I quite like that this specifies exactly which shell it's using rather than the platform.

sordina · 2024-04-16T23:09:58Z

connector-definition/scripts/check-reqs.ps1

+$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
+  }
+}


Should this be specified as a constant somewhere so that is can be shared between scripts, etc?

$minimumNodeVersion I mean

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.

sordina · 2024-04-16T23:10:26Z

connector-definition/scripts/check-reqs.ps1

+  if ((Test-Path "./node_modules") -eq $false) {
+    Write-Host "node_modules not found, please ensure you have run 'npm install'."
+    exit 1
+  }


sordina · 2024-04-16T23:10:36Z

connector-definition/scripts/check-reqs.sh

+#!/usr/bin/env bash
+set -eu -o pipefail
+
+minimum_node_version="20"


sordina · 2024-04-16T23:11:28Z

connector-definition/scripts/read-package-script.js

+// 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


Are there any open issues about this upstream? Seems so dumb.

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. 🤷‍♂️

sordina · 2024-04-17T00:28:09Z

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: ***@***.***>

daniel-chambers self-assigned this Apr 11, 2024

daniel-chambers added 2 commits April 16, 2024 12:14

Add support for native connector packaging

22690fa

Stop using npm so signal handling works properly

260709c

daniel-chambers force-pushed the daniel/native-packaging branch from 1c04ba8 to 260709c Compare April 16, 2024 02:15

daniel-chambers marked this pull request as ready for review April 16, 2024 03:01

sordina approved these changes Apr 16, 2024

View reviewed changes

Update changelog

390e357

daniel-chambers merged commit d72fe06 into main Apr 17, 2024

daniel-chambers deleted the daniel/native-packaging branch April 17, 2024 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for native connector packaging #30

Add support for native connector packaging #30

Uh oh!

daniel-chambers commented Apr 11, 2024 •

edited

Loading

Uh oh!

sordina left a comment

Uh oh!

sordina Apr 16, 2024

Uh oh!

sordina Apr 16, 2024

Uh oh!

sordina Apr 16, 2024

Uh oh!

sordina Apr 16, 2024

Uh oh!

daniel-chambers Apr 17, 2024 •

edited

Loading

Uh oh!

sordina Apr 16, 2024

Uh oh!

sordina Apr 16, 2024

Uh oh!

sordina Apr 16, 2024

Uh oh!

daniel-chambers Apr 17, 2024

Uh oh!

sordina commented Apr 17, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add support for native connector packaging #30

Add support for native connector packaging #30

Uh oh!

Conversation

daniel-chambers commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sordina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-chambers Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sordina commented Apr 17, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daniel-chambers commented Apr 11, 2024 •

edited

Loading

daniel-chambers Apr 17, 2024 •

edited

Loading