CLI Extensions #98
CLI Extensions #98
Conversation
| And that can do anything programmatically and also get the benefit of having access to the Temporal client. So it could | ||
| access some company resource (e.g. a database) and send an update to a workflow with some value. | ||
|
|
||
| ### Contributions |
There was a problem hiding this comment.
Will all contributions/extensions be in their own repos and have an extension discovery platform, or will we have some extensions live in the CLI repo (either temporal authored or external that meet a certain bar/requirement)?
There was a problem hiding this comment.
All extensions will be external, even Temporal authored ones (unless we decide to make them built-in commands). We have things like https://temporal.io/code-exchange to help build a marketplace of these as needed. I need to change the title of this section to "Extension Ecosystem" since they are not contributions in the traditional sense.
| `temporal-foo-bar_baz` executable is called for both `temporal foo bar-baz qux` and `temporal foo bar_baz qux` though | ||
| help text only shows the former. | ||
|
|
||
| Built-in commands cannot be overridden by extensions, but subcommands can be added to existing commands. |
There was a problem hiding this comment.
I'm maybe sliiiightly concerned about the possibility of this being an attack vector, where someone could sit on
temporal workflow strt or something, and hijack a mistype of workflow start and because it can load up profile info it might be able to do something bad to your instance.
This applies to any command, too (and I wouldn't want to sacrifice profile loading), but the chance is higher with a typo like this.
Maybe this means we should plan to (at some point) build in something like this is a new extension from XXX signed by ZZZ do you want to enable running this extension? y/n
There was a problem hiding this comment.
I'm not too concerned about this as this is something a user installs. Being concerned that a user put temporal-workflow-strt PATH would be like being concerned that a user put temporl on their PATH and temporl workflow start could be hijacked. A user putting a binary on their path slightly misnamed from what they intend I think has the same concerns for top-level as sub-command level. I doubt similar concerns exist for git-commt executables, so not sure we need to take extra precautions here.
| `temporal workflow --address foo start`, and `temporal workflow start --address foo` are all the same. It is still a | ||
| subject of active research on whether flags of _parent_ commands can still be placed before the extension command in the | ||
| CLI. Regardless, flags of the extension command _must_ come after the extension command because CLI does not know |
There was a problem hiding this comment.
I would want the answer to whether parent flags can come before children to be yes. Extension flags coming after commands being a must is a good thing I think, regardless of the technical reason behind it.
There was a problem hiding this comment.
The reason I mentioned the parent-flag-before-command (I should have called it built-in-flag-before-command) is under active research was more about how hard our library is going to make performing partial flag parsing on not-found commands. But I think it will work.
| There is not a plan to expose a printer at this time, so extensions will have to handle the possible `output` enumerates | ||
| of `text`, `json`, `jsonl`, and `none` themselves. This is because the current CLI printer has too many quirks and is |
There was a problem hiding this comment.
My feeling is it's very likely they just won't
That's not really the end of the world or anything but I just want to acknowledge that's the likely outcome if we don't start with a helper.
Alternatively we could require them to always do jsonl, and just transform that into the desired format for them. I don't have a strong feeling on it, but it's an option.
There was a problem hiding this comment.
That's not really the end of the world or anything but I just want to acknowledge that's the likely outcome if we don't start with a helper.
We don't have to block the whole project by starting with a helper if we can followup shortly thereafter. I don't want to hold up this project on designing a quality printer abstraction. So maybe I can change this to "we may expose a printer shortly after initial implementation"?
There was a problem hiding this comment.
Yeah, I didn't say to hold it up. Just wanted to bring it up.
There was a problem hiding this comment.
👍 Is anything holding up the proposal (and subsequent implementation) from your POV?
| ### Helper Library | ||
|
|
||
| Built-in commands leverage several helpers to be consistent with the CLI ecosystem. Extension commands should be able to | ||
| do the same, within reason, so a helper library will be made available. Originally it was thought that such a helper |
There was a problem hiding this comment.
Can you outline how users who don't want to use Go will proceed? I expect Python will be a common language to want to implement extensions in.
There was a problem hiding this comment.
Like kubectl (https://kubernetes.io/docs/tasks/extend-kubectl/kubectl-plugins/#using-the-command-line-runtime-package), they will not get a helper library at this time. I can clarify here if needed that there are no plans for any helper libraries for CLI extensions beyond the language the CLI is written in. I actually don't expect Python will be a common language to implement extensions in (but Python may be a common language to write general CLIs, just not as extensions to the Temporal CLI in Go I don't think anymore than it is for kubectl).
There was a problem hiding this comment.
Python's a bit too slow to be a good language to write general CLIs in, but in this context where you know you're making a network call that disadvantage doesn't apply. It's fine if Go is the only offering initially but I don't think plugin authors should have to care which language the CLI is written in. And since plugins are often written by people to "scratch an itch" (often for their own development environments), I think it will be common to choose a more convenient language than Go.
There was a problem hiding this comment.
You are more than welcome to write it in any language. If you care about convenience, you may choose bash. But if you want a helper library, it's only reasonable for us to maintain it in Go at this time (and for our use, the temporal-cloud CLI, we'll of course only use Go). Maybe we can revisit if the desire is large enough for Temporal CLI extensions in Python, though I suspect like similar Go CLIs there won't be much desire.
There was a problem hiding this comment.
Personally I am fine only providing helpers for Go at this time.
|
|
||
| It is an area of active research whether the CLI's YAML-based code generation functionality will be made available to | ||
| extensions. | ||
|
|
There was a problem hiding this comment.
How will errors from extensions be presented? (Will there be any effort to make this have a consistent appearance, e.g. will the real CLI intercept and re-present stderr and/or exit codes from user executables?)
There was a problem hiding this comment.
There is no effort to make any of the output consistent at this time. Extensions are free to do with stdin, stdout, stderr, and exit codes whatever they want. Parent CLI will just tunnel the subprocess stuff through.
| Extensions on built-in commands are not shown as part of the parent command's help. | ||
|
|
||
| Running `temporal help <command sequence>` is treated as `temporal <command sequence> --help`. So the same style of | ||
| lookup is performed for extensions. |
There was a problem hiding this comment.
Having a temporal plugin subcommand could be worth considering for discoverability (e.g. temporal plugin list)
There was a problem hiding this comment.
I don't think we should call these "plugins", but regardless, I found the git approach a bit simpler than a full, separate command for listing.
There was a problem hiding this comment.
The advantage of a subcommand is that when a user does temporal --help, they'll see a subcommand with a name like "extension", alerting them to the fact that extensions are a possibilty. Not blocking, but worth considering for discoverability.
There was a problem hiding this comment.
We can add a "To see all extensions, pass --all" to the end of --help text if it's important that default help shows that extensions are thing, but I am hoping to avoid a new sub command at least not until there is more to do than just list commands.
|
|
||
| Extensions are any executables on the `PATH` with `temporal-` prefix. Extension executable names use `-` for each | ||
| subcommand and `_` for each dash in the command (but still can be called with an underscore). This is similar to | ||
| `kubectl` behavior. |
There was a problem hiding this comment.
While there is precedent with kubectl and git, I wonder whether we should choose a slightly less common prefix such as temporalcli- or temporal-cli- or -temporal-. I've surveyed one temporal user, and they already have the following executables named temporal- (found by issuing temporal-<TAB>):
cli(main) temporal-
temporal-cancel-all
temporal-delete-all
temporal-generate-help-output
temporal-github-actions-delete-runs
temporal-otel-collector
temporal-python-release.sh
temporal-python-test-failures
temporal-samples-traffic
temporal-server
temporal-server-test-failures
temporal-slack-search
temporal-terminate-all
temporal-typescript-link
temporal-workflow-from-url
temporal-workflow-list-ids
temporal-workflow-start
There was a problem hiding this comment.
I expect the dashes to be like spaces on a command line and I think others would expect similarly having seen this pattern before. Looking at that list, I'd expect those to be CLI extensions same as I would for other extensible CLIs. All of those all now magically work with spaces instead of dashes with the exception of temporal-server and temporal-workflow-start. I don't think we should alter the simplicity of extension names based on this user's list. But if it is enough of a struggle for this user, maybe we can make the prefix configurable via env var.
| help text only shows the former. | ||
|
|
||
| Built-in commands cannot be overridden by extensions, but subcommands can be added to existing commands. | ||
|
|
There was a problem hiding this comment.
It might make sense to reserve some names as unavailable for extensions, for example temporal nexus, temporal config etc?
Beyond that, are you going to impose any restrictions on valid names? (File systems are very permissive) We obviously want to support user's local language; I think that typically means allowing non-utf8 even (?).
There was a problem hiding this comment.
It might make sense to reserve some names as unavailable for extensions
Yes we can reserve some, though we won't predict them all. I will make a blanket statement like "Built-in commands always take precedence so if a newer Temporal CLI has a built-in command that once was an extension, the command will be chosen and the extension name will have to be changed". This is a normal state of affairs for CLI extensions, and in many ways it can be a good thing because some extensions could be contributions upstreamed to become built-in.
|
|
||
| Invocation of the extension is done as a subprocess. Stdin, stdout, stderr, exit code, etc are all handled by the | ||
| extension and just relayed through the `temporal` process as is. It is an area of active research on whether interrupt | ||
| signals can be ignored by the parent `temporal` process to be handled by the subprocess, though that is the goal. |
There was a problem hiding this comment.
exec is an option on non-Windows platforms -- that way signals, exit code etc all work exactly as expected without any work. But of course subprocess has to be supported for Windows, so there may not be a justification for the two paths.
There was a problem hiding this comment.
Yes, we may take this approach as this is what kubectl does: https://github.com/kubernetes/kubectl/blob/285ed6ce48d8ba81d430845306c70533170cab58/pkg/cmd/cmd.go#L239-L259. The problem is that I don't think we can do anything as a wrapping process anymore, like enforce command-timeout, if we use this form of process replacement. But it still may be worth it.
Summary
This is a proposal for supporting extensions in the CLI.
RENDERED
Please leave comments/feedback. If the comment list grows unwieldy and/or we've resolved most but still have more to say, this PR can be closed and another will be opened. All alternative options/suggestions welcome. Requests for clarifications or justification on anything welcome.
Note, some things are missing by intention because they require more research, but we didn't want to hold up the ability to review.
As part of this, all legacy CLI proposals for
tctlwere moved to apreviousfolder since they aren't weren't for what is currently termed the "CLI".