-
Notifications
You must be signed in to change notification settings - Fork 1
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 native toolchain definition to connector metadata #687
base: main
Are you sure you want to change the base?
Conversation
commands: vec![ | ||
("start".to_string(), metadata::CommandDefinition { | ||
command_type: metadata::CommandType::ShellScript, | ||
bash: "#!/usr/bin/env bash\nset -eu -o pipefail\nHASURA_CONFIGURATION_DIRECTORY=\"$HASURA_PLUGIN_CONNECTOR_CONTEXT_PATH\" ndc-postgres serve".to_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.
Are you sure this will work? This is not what I expect to see passed to bash -c
as per the specification for native toolchain commands. This looks like what should be in a script file inside the connector definition's .hasura-connector folder. That's what's done in the nodejs lambda connector.
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.
Either of them works. The bash
field can point to a script file or contain the script itself.
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.
@SandeepSamba Interesting; that's not in the spec afaik. Do you detect this and run it differently? (ie not "bash -c")
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, we just dump bash -c "whatever that is there in the field"
and it works
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.
Hrm. When I try this manually in my terminal it doesn't work, which is why I was questioning it.
For example,
bash -c '#!/usr/bin/env bash\nset -eu -o pipefail\necho really\necho test > test.txt'
neither echoes "really" nor writes "test" into test.txt
.
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.
Oh okay, this is just my bad. If I use single quotes, it's not substituting the \n
s. If I do this:
bash -c '#!/usr/bin/env bash
set -eu -o pipefail
echo really
echo test > test.txt'
It works. OK all good then!
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.
Now this is super weird, same result when I tried the command you tried, but if I put the the following in a file test.sh
and do bash -c "$(cat ./test.sh)"
, it works!!!
#!/usr/bin/env bash
set -eu -o pipefail
echo really
echo test > test.txt
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 doesn't surprise me now. My mistake was using single quotes and then using escape sequences. :)
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.
Great minds think alike :)
("start".to_string(), metadata::CommandDefinition { | ||
command_type: metadata::CommandType::ShellScript, | ||
bash: "#!/usr/bin/env bash\nset -eu -o pipefail\nHASURA_CONFIGURATION_DIRECTORY=\"$HASURA_PLUGIN_CONNECTOR_CONTEXT_PATH\" ndc-postgres serve".to_string(), | ||
powershell: String::new(), |
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.
Please implement the same thing in Powershell. We have Windows customers.
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 native runtime for postgres is targeted to be run inside the Native DDN workspace only and this script assumes the binary is present in so and so location, so this is not a general script that all users in UNIX systems can run (Postgres is setup in workspace like this). Also, I don't think there is a windows binary for the Postgres Connector today (I mean the connector itself, not the plugin). So, lets skip the powershell script for now?
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 spec should not be asking for a PowerShell command then. :/
It's trivial to translate that to PowerShell, regardless: $env:HASURA_CONFIGURATION_DIRECTORY=$env:HASURA_PLUGIN_CONNECTOR_CONTEXT_PATH; & ndc-postgres.exe serve
.
Who knows, next week maybe someone will ask for Native DDN workspace equivalent for a Windows VM or something. Doing something half-way always comes back to bite you later 🙂
What
This PR adds the native toolchain definition to the connector metadata.
It also adds two commands in the initial metadata generated by the CLI:
Click here for the CLI command output
How