temporalio / cli

Command-line interface for running Temporal Server and interacting with Workflows, Activities, Namespaces, and other parts of Temporal
https://docs.temporal.io/cli
MIT License
247 stars 34 forks source link

Switch to a common format for CLI command generation #620

Open josh-berry opened 1 month ago

josh-berry commented 1 month ago

We should switch to a more commonly-understood format for CLI command generation that is easily consumable by "standard" parsers (like a Markdown or YAML or JSON parser). The custom parser thing we have now is quite easy to misuse—for example, there is a semantic difference between Required and Required., and between Default is foo and Default: foo.

After some internal discussion, YAML is probably the preferred approach, with Markdown for item descriptions. For example:

- temporal env set:
    summary: Set environment properties
    description: |
      `temporal env set --env environment -k property -v value`

      Property names match CLI option names, for example '--address' and '--tls-cert-path':

      [...]
    maximum-args: 2
    ignores-missing-env: true
    options:
      - [-k, --key]:
        type: string
        description: The name of the property.
        required: true
      - [-v, --value]:
        type: string
        description: The value to set the property to.
        required: true

Another option might be to embed YAML within Markdown, however this option was discarded as it's still more complex to parse:

### temporal env set: Set environment properties.

`temporal env set --env environment -k property -v value`

Property names match CLI option names, for example '--address' and '--tls-cert-path':

`temporal env set --env prod -k address -v 127.0.0.1:7233`
`temporal env set --env prod -k tls-cert-path -v /home/my-user/certs/cluster.cert`

If the environment is not specified, the `default` environment is used.

```yaml
maximum-args: 2
ignores-missing-env: true
options:
  - [-k, --key]:
    type: string
    description: The name of the property.
    required: true
  - [-v, --value]:
    type: string
    description: The value to set the property to.
    required: true

(Note: Exact structure of the YAML data is still TBD; the above examples show one possibility but there are others, to be decided at implementation time.)

I also considered but discarded XML because it's significantly harder to work with in code, and therefore much less commonly-used nowadays. I also considered but discarded JSON because it's not flexible enough in its plain-text representation; we expect to have a lot of multi-line string literals, and JSON does not do well with those.

YAML does have some caveats, but we still think it's the best choice despite those caveats:

Further comments and opinions are of course welcome. :)

josh-berry commented 1 month ago

Not sure if we want to do this together or separately, but it would also be nice to use a proper Markdown formatter for producing help output, especially for the long descriptions. This would allow us to re-wrap text nicely, apply bold/underline, handle links, etc.

josh-berry commented 1 month ago

A quick thought on how to allow for flag reuse:

commands:
- temporal env set:
    summary: Set environment properties
    description: |
      `temporal env set --env environment -k property -v value`

      Property names match CLI option names, for example '--address' and '--tls-cert-path':

      [...]
    maximum-args: 2
    ignores-missing-env: true
    options:
    - --env
    - key-value-pair

option-defs:
  --env:
    type: string
    description: The name of the environment to use.
  key-value-pair:
  - [-k, --key]:
    type: string
    description: The name of the property.
    required: true
  - [-v, --value]:
    type: string
    description: The value to set the property to.
    required: true 
cretz commented 1 month ago

I think that's divorcing it a bit too much

commands:
  temporal env set:
    summary: Set environment properties
    description: |
      `temporal env set --env environment -k property -v value`

      Property names match CLI option names, for example '--address' and '--tls-cert-path':

      [...]
    maximum-args: 2
    ignores-missing-env: true
    options:
      env:
        type: string
        description: The name of the environment to use.
    option-refs: [key-value-pair]

option-defs:
  key-value-pair:
    key:
      shorthand: k
      type: string
      description: The name of the property.
      required: true
    value:
      shorthand: v
      type: string
      description: The value to set the property to.
      required: true 

Or inline but I think I like this less:

commands:
  temporal env set:
    summary: Set environment properties
    description: |
      `temporal env set --env environment -k property -v value`

      Property names match CLI option names, for example '--address' and '--tls-cert-path':

      [...]
    maximum-args: 2
    ignores-missing-env: true
    options:
      env:
        type: string
        description: The name of the environment to use.
      key:
        shorthand: k
        type: string
        description: The name of the property.
        required: true
        tag-ref: [key-value-pair]
      value:
        shorthand: v
        type: string
        description: The value to set the property to.
        required: true
        tag-ref: [key-value-pair]
  temporal something else:
    other-stuff: other-stuff
    options:
      some-option-specific-to-me:
        type: string
    include-tag-refs: [key-value-pair]

But in general, I don't think we should move all options definitions away from their use, potentially only shared ones

josh-berry commented 1 month ago

But in general, I don't think we should move all options definitions away from their use, potentially only shared ones

Completely agree, I did so in my example only for illustrative purposes.