ManageIQ / floe

Floe is a runner for Amazon States Language workflows
Apache License 2.0
0 stars 6 forks source link

Add support for pluggable schemes #169

Closed Fryguy closed 7 months ago

Fryguy commented 7 months ago

The idea behind this PR is to move the docker runner (renamed here to container runner for clarity) into a separate directory in a way to show what a plugin would look like and what the registration of schemes would look like.

For now, this PR:

TODO:

Here's what the --help looks like after this:

$ be exe/floe --help
Usage: floe [options] workflow input [workflow2 input2]

v0.9.0

Options:
  -w, --workflow=<s>                     Path to your workflow json (legacy)
  -i, --input=<s>                        JSON payload to input to the workflow (legacy)
  -c, --context=<s>                      JSON payload of the Context
  -e, --credentials=<s>                  JSON payload with credentials
  -d, --credentials-file=<s>             Path to a file with credentials

Container runner options:
  -r, --container-runner=<s>             Type of runner for docker container images (docker, podman, or kubernetes)
  -o, --container-runner-options=<s+>    Options to pass to the container runner
  -k, --docker                           Use docker to run container images     (short for --container-runner=docker)
  -p, --podman                           Use podman to run container images     (short for --container-runner=podman)
  -u, --kubernetes                       Use kubernetes to run container images (short for --container-runner=kubernetes)

General options:
  -v, --version                          Print version and exit
  -h, --help                             Show this message

Some future thoughts:

@agrare @kbrock Please review.

Closes #45

Fryguy commented 7 months ago

I know this changes the interface to exe/floe, and I'm not sure we wanted that. I did it more to clarify that the runner is around running containers. However, since the scheme is docker, we may want to continue to call it docker_runner for consistency. That being said, I don't think we need to enforce that a plugin's name match the scheme, because that would preclude plugins that support multiple schemes.

kbrock commented 7 months ago

I actually wanted Floe::ContainerRunner directly, but didn't want to deal with the un-indenting of 1 level of namespace. I still may do this anyway, because then this would mirror what a real "plugin" would look like allowing for things like require 'floe/container_runner' and living in a floe-container_runner gem.

I like the unindent idea (and agree, not necessary to do it at this time). That said, I still feel that the runner's parents should be Floe::Runner and not

While the exe/floe hardcodes the reference to the container runner, I think it's cleaner to loop over all of the registered schemes, and then call cli_options/resolve_cli_options on each. However, I thought it would make this PR less clear and didn't do that for now.

I mentioned above, I feel we need to register a plugin separate from registering the schemes.

Fryguy commented 7 months ago

I mentioned above, I feel we need to register a plugin separate from registering the schemes.

Not sure I understand this distinction. The only purpose of a plugin is to bring schema runners.

Fryguy commented 7 months ago

Added to the OP that I think this closes #45

kbrock commented 7 months ago

LGTM @Fryguy are you all set here?

Fryguy commented 7 months ago

yup :+1:

Fryguy commented 7 months ago

After this PR is merged I'll do some of the indent changes as discussed.