Skip to content
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.

Sign up for GitHub

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

Dockerized Commands in the packaging spec #129

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Dockerized Commands in the packaging spec #129

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions rfcs/0001-packaging.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,14 @@ type EnvironmentVariableDefinition = {
}

type Commands = {
update?: string
watch?: string
update?: string | DockerizedCommand
watch?: string | DockerizedCommand
Comment on lines +67 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to allow a string array for args in the Docker command, let's do it for native commands too.

Suggested change
update?: string | DockerizedCommand
watch?: string | DockerizedCommand
update?: string | string[] | DockerizedCommand
watch?: string | string[] | DockerizedCommand

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure about that. For native commands, that is not "args", it's a shell command, so it's one string because it ends up as one arg. It's what get passed to (for example) bash -c.

bash -c "echo \"123\"" is not the same as bash -c "echo" "123".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I want the capability to provide commands + args directly, not just a shell command. There's no reason to invoke a shell for echo 123, after all, if we can represent it as ["echo", "123"].

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. @SandeepSamba do you have the capacity to implement a third methodology here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it to be just a single string command rather than string[]. We have to invoke the commands via a shell because without a shell, all the fancy(coloured output, formatting,etc) logging that the plugins provide are lost. Even if we provide a string[], the CLI wil concat them with spaces and call it with shell. And with the native packaging RFC we will anyway have to bring in a shell. @SamirTalwar Do you have any particular issue due to the choice of single string vs string[]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SandeepSamba the problem is which shell? Isn't that the point of this. To not use a shell, and just have the CLI run the docker command. Bash isn't normally or always available on windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codedmart I think @SandeepSamba's point is that he doesn't want to implement string[] (which bypasses the shell) as an alternative to string (which uses the shell), because bypassing the shell stuffs up formatting. The DockerizedCommand is separate to string and string[]. @SamirTalwar's suggestion of adding string[] was in addition to DockerizedCommand.

}

type DockerizedCommand = {
type: "Dockerized"
dockerImage: string // eg "hasura/postgres-data-connector:1.0.0"
commandArgs: string[]
}

type CliPluginDefinition = {
Expand All @@ -90,7 +96,10 @@ The `connector-metadata.yaml` contains YAML that describes:
- The packaging definition, which can be either `PrebuiltDockerImagePackaging` (a connector that does not require a build step), or `ManagedDockerBuildPackaging` (a connector that requires a build step).
- `PrebuiltDockerImagePackaging` defines the prebuilt `dockerImage` used to run the connector (`dockerImage`)
- If `ManagedDockerBuildPackaging` is used, a Dockerfile must be in the `.hasura3` directory (and optionally, a `.dockerignore`). It will be used to build the connector.
- A `commands` structure that optionally defines what shell commands to run for an "update" (eg. refresh schema introspection details) and "watch" (eh. watch and refresh schema introspection details periodically) scenario.
- A `commands` structure that optionally defines what commands to run for an "update" (eg. refresh schema introspection details) and "watch" (eg. watch and refresh schema introspection details periodically) scenario.
- A string value is a shell command to be executed
- A `DockerizedCommand` object defines a particular Docker image that should be run, passing the specified command arguments to. They can expect to have the build context directory mounted as a volume.
- Both the shell command and Docker container can expect to have the connector's environment variables set, as well as the environment variables defined in the [CLI plugin spec](https://github.com/hasura/ndc-hub/blob/cli-guidelines/rfcs/0002-cli-guidelines.md).
- An optional `CLIPluginDefinition` that describes where to acquire the CLI plugin for this connector that can be used by the `commands` structure. If provided, the CLI plugin executable will be made available on the `PATH` for the commands and some configuration environment variables will be set (see the [CLI plugin RFC](https://github.com/hasura/ndc-hub/blob/cli-guidelines/rfcs/0002-cli-guidelines.md) for more details).
- A `dockerComposeWatch` that defines how to rebuild/restart the container if the user modifies their connector configuration in their project (see below)

Expand Down
Loading