Make argument and flag use consistent across commands

[josh-berry]

Some commands have required flags—we should take a pass at every command and see if there are required flags that should instead be positional arguments.

Exceptions might be situations where it's not obvious which order arguments should be provided in, or what the arguments actually are when reading a particular CLI invocation (since script writers will probably appreciate having named flags for documentation purposes).

[lorensr]

I'm not aware of any required args that are currently positional. Adding to 1.0.0 milestone because it's such a large change.

So this current command:

temporal workflow reset --workflow-id 3998111b-b11e-4155-bc1e-4aaed545cc65 --event-id 13 --reason "fixed the bug, need to go back" --reapply-type None

with workflow ID, reason, and either event id or type required might become:

temporal workflow reset 3998111b-b11e-4155-bc1e-4aaed545cc65 "fixed the bug, need to go back" --event-id 13 --reapply-type None

[josh-berry]

@lorensr Possibly yeah. If a command has lots of required arguments (e.g. more than 2 or maaaaybe 3), I'd want to evaluate more closely whether going positional is actually a good idea, so it's not a given that this will change everywhere. But I think it would be good to change where it makes sense / is more ergonomic—that's the primary concern.

Opinions welcome. :)

[cretz]

Note, the poor library we use for CLI won't be able to extract positional args if a flag arg is after it I don't believe. We made a huge mistake not going with cobra, and I suspect this is a consequence of that too. See temporalio/documentation#2345.

[bergundy]

I agree we should have gone with Cobra, we inherited this problem when the project was ported from "tctl v2".
I'm sure an experienced Go dev could transition in a day or two of work.
If we do transition though, we'd need to understand what to do with the docs generation tool, that's based on urfave, that may need to be rewritten if we care about auto generated docs.

[karelbilek]

Also see #347

[josh-berry]

Here are three options for us to consider, in order of my preference:

Option 1 (preferred): Use positional arguments for direct objects

We change our arguments/flags to follow two principles:

• More than one or two required positional arguments quickly becomes ambiguous (unless we’re talking about, say, a list of things to operate on), and should be avoided.
• Most commands operate on one or more direct object(s). Direct objects are generally required, and what the direct object is is already named in the command itself (e.g. the word workflow in temporal workflow create). Therefore, it is both shorter and clearer to pass direct objects positionally. For example, it’s clear that the foo in temporal workflow terminate foo is the workflow ID. Adding a-w (as in temporal workflow terminate -w foo) is redundant.

Specifically, I propose we make the following changes:

• Make all temporal workflow ... take the workflow ID as the first positional argument instead of passing thru -w, and keep any remaining required items as flags
• Same for temporal task-queue ... and the task queue ID
• Same for temporal schedule ... and the schedule ID
• Same for temporal batch ... and the job ID
• Same for temporal operator namespace ... and the namespace ID
• Same for temporal operator search-attribute and the attribute name
• Same for temporal operator cluster and the frontend address (which we seem to refer to inconsistently as --frontend-address and --name)

No changes are proposed to the other subcommands (server, activity, env). server start-dev has no required arguments, and env already follows my desired convention (explained further below).

For activity, one needs to specify both the workflow ID and activity ID, so there’s not one single ID that can be used to refer to a particular activity. We could require passing both of the IDs as positional arguments, but this might be ambiguous, since the order of arguments wouldn’t be immediately obvious from reading a temporal activity foo command in a script, command history, etc.

Here are a couple contrived but real-world-shaped examples (I pulled these from my own shell history when playing around with toy workflows):

Executing a workflow:

• Before: temporal workflow execute -t sso --type createUser -w bar -i '"bar"' -i '{"name": "Name", "ssn": "123-45-6789"}'
• After: temporal workflow execute bar -t sso --type createUser -i '"bar"' -i '{"name": "Name", "ssn": "123-45-6789"}'

Describing a workflow:

• Before: temporal workflow describe -w bar
• After: temporal workflow describe bar

(list and similar commands would not change, since they have no required arguments / no direct objects.)

If we decide to go forward with this, we can have a transition period where both the old and new forms are accepted (with a warning) before removing the old forms, to allow users time to update their scripts.

Option 2 (alternative): Make everything use flags

If we choose not to go forward with option 1, we can get rid of positional arguments entirely, and at minimum do another pass at the flags to make sure they are consistent between commands (e.g. temporal operator cluster upsert --frontend-address vs temporal operator cluster remove --name). (Honestly, we should probably do this anyway, though that's outside the scope of this ticket.)

This would mainly affect the env commands, which are the only ones that use positional arguments currently. We might, for example, use -e for the environment name, -k or -p for the property name, and -v for the value, so an env command change would look like:

• Before: temporal env set default.address 127.0.0.1:7233
• After: temporal env set -e default -k address -v 127.0.0.1:7233

Option 3: Do nothing

We could also decide to keep commands, flags and arguments as-is, because changing them isn't worth the cost of breaking backward compatibility. This is my least-favorite option because we have an opportunity to polish things now that we won't post-GA (when backward compatibility becomes mandatory).

[dandavison]

@josh-berry I like this proposal: I agree that we should at least consider bringing the interface in line with Unix CLI tradition in the way you're suggesting.

I'm imagining your idea looking something like

temporal workflow execute CreateOrder -w ord-123
temporal workflow update AddItem -w ord-123 --update-id upd-456 --input '{"key": "val"}'
temporal workflow signal AddItem -w ord-123 --input '{"key": "val"}'
temporal operator namespace create MyNamespace

temporal activity complete --activity-id act-789 -w ord-123 --result '{"key": "val"}'
temporal workflow terminate -w ord-123

So, positionals are typically meaningful names. I'm thinking that opaque identifiers (e.g. workflow ID, activity ID) should never be positionals: so we'd always use -w for workflow ID. In the case of workflow execute that's because I believe the direct object is the type of workflow you want to execute, not the workflow ID. But also it's because the commands are more readable when opaque identifiers are tagged with what they are (--activity-id, -w), and it helps with backwards-compatibility consistency with tctl and temporal 0.x.

[josh-berry]

(FYI for anyone following along both inside and outside of Temporal: I'd like to make a decision on this in the next few days, so we can move forward with whatever changes are needed, if any.)

[cretz]

I vote option 2 or 3. Reasons:

• To @josh-berry's option 1 proposal, workflow IDs sometimes as positional and sometimes as not is confusing
• To @dandavison's proposal, that it's unclear/ambiguous to us what should be the "direct object" of a workflow start/execute/signal/etc should be, you can expect it'll be even more confusing to users. Is the "direct object" what you're operating on or just the primary parameter to the operation? It is a bit confusing to say the workflow isn't the "direct object" of a workflow describe call.
• The flags as they exist today seem to have worked well for people, so we may be solving a problem that doesn't exist while also making a new problem that we very well know will exist (breaking compatibility).

Overall, taking into account past and future use, I think we would be hurting users more than helping by making this unnecessary change.

[josh-berry]

Notes from team discussion:

• Option 1 represents a lot of churn that we don't think provides any real value; it would break a bunch of user scripts for minimal gain.
• Option 2 seems like the most likely path, but we need to do a more detailed assessment of what would actually change. (We realized in the meeting that more than just env uses positionals, so I need to take another pass at the list of commands/arguments.)

[cretz]

Can you clarify the opposition to option 3? Are we trying to be so consistent between env and workflow that the simple env use today will not be the way forward? I think we can be practical here and say positionals where it makes a lot of sense (which is basically nowhere but env IMO today). I don't mind option 2, but it sounds like we want to break env commands due to some dogmatic view of 100% consistency instead of pragmatic concern for users today.

[josh-berry]

@cretz A couple folks in the meeting mentioned confusion around how env works today; the env.variable notation is not intuitive (and I agree). So we think there is value in changing env in particular. And if we change env, that might leave other things like operator namespace with inconsistent design that might be confusing for new users.

Ultimately it's quite subjective which things make sense as positionals and which don't. Just in the course of this discussion, we've had different folks proposing different approaches (e.g. my option 1, Dan's mod to option 1, your desire to keep things the same) and none of the options are obviously more intuitive than the others. That leads me to think that eliminating them entirely strikes the right balance between minimizing churn for our existing users and maximizing consistency and ease-of-understanding for new users.

[cretz]

I don't think there ever was a consistency and ease-of-understanding issue with env for new users with regards to its positionals and I believe we are solving for a problem that doesn't really exist with users. We just kinda made up to ourselves that this is a problem. We are also definitely causing a new problem with users familiar with this.

We can't yet count future issues this will cause, but we can count past issues we've had with confusion on env calls' use of positionals (zero I would guess). At some point in the future when after we've released the env break we can then compare issues we're causing vs issues that exist with the current approach.

I believe this will now be our largest compatibility break to date on the CLI. I hope all the benefits are worth it.

[josh-berry]

Option 2 - specific changes proposed:

• temporal operator namespace create foo => create -n foo
  • NOTE: create is today inconsistent with the other operator namespace commands in not requiring a flag.
• temporal operator namespace describe --namespace-id=foo => describe -n foo [The namespace ID is different from the namespace name, oops.]
  • To be consistent with namespace update and non-namespace commands, which use --namespace and -n
• temporal operator namespace delete foo => delete -n foo
• temporal operator cluster remove --name => remove --frontend-address
  • NOTE: This is inconsistent with cluster create today, which uses --frontend-address. I'm also not thrilled with --frontend-address as a name; maybe --remote-address? (Since --address is already the local cluster.)
  • Turns out --frontend-address and --name are actually different.
• temporal env get env.key => get -e env -k key
• temporal env set env.key value => set -e env -k key -v value
• temporal env delete env => delete -e env
• temporal env delete env.key => delete -e env -k key

I think I have a stronger opinion about fixing the inconsistencies than I do about env (which is at least consistent with itself)—at minimum, I want us to use consistent flags for things that mean the same across commands. Otherwise we are just making it harder for both new AND existing users to remember how to use the CLI.

[josh-berry]

Discussed in our team meeting and decided to adopt option 2 with a deprecation period for all breaking changes.

So we will accept both old and new forms for a while, warn on the old forms being used, and then eventually the old forms will stop working. We can figure out the exact details of deprecation later, but for now, we should at minimum print a warning to stderr.

[dnr]

Sorry for late comments:

I do find the -w for workflow, -t for task queue, etc. to be pretty annoying in some contexts (when copying/pasting ids, I have to remember to type the -w first). At the same time, in scripts I like to see the flags for clarity. tctl had tctl wf showid and similar, but duplicating the commands seems messy. So how about this backwards-compatible option:

Option 1½: Accept all the current flags, but also:

• For commands operating on workflows as direct objects:
  • If there is no -w and there are 1 or 2 positional args, the first positional arg is treated as the workflow id.
  • If there is no -r and there are 2 positional args, the second position arg is treated as the run id.
  • If > 2 positional args, or mix of -w/-r and positional args, error.
  • For this purpose start/execute are excluded.
• For commands operating on task queues as direct objects:
  • If there is no -t and there is 1 positional arg, the first positional arg is treated as the task queue id.
  • If > 1 positional arg, or mix of -t and positional args, error.
• For commands operating on schedules as direct objects:
  • If there is no -s and there is 1 positional arg, the first positional arg is treated as the schedule id.
  • If > 1 positional arg, or mix of -s and positional args, error.

That would be enough to make me happy, since those are the most commonly used interactive commands and the most obvious "direct objects". (I have no opinions on env)

[cretz]

I fear that multiple ways to express the same thing is unnecessarily confusing/ambiguous to solve this "problem" (which I'd argue isn't really a "problem" today, we just kinda told ourselves it was).

[dnr]

I'm not making anything up, I'm saying this has annoyed me in the past when using the cli to debug issues on cloud, and I believe I would be able to work (slightly) faster if I could leave out the -w and -r and -t and -s in these unambiguous situations.

I don't know if it rises to the level of "problem", obviously it's not a huge deal. Yes, multiple ways to say the same thing are potentially confusing. But it's a common thing to do in shells and clis because it also improves efficiency of using them. That's why shells have a much "looser" syntax than proper programming languages, and why we still use them for interactive use instead of "better" options. They're allowed to be a little more like natural language.

[metavee]

I ran into this with --grpc-meta. I'm using a server that requires auth headers, and depending on the subcommand, I have to place the argument in a specific spot for it to work. This make the CLI pretty unfriendly to use.

In the shorter commands with no arguments you can put it at the end and it works:

# works
temporal workflow list --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN"

But e.g. temporal operator where it needs to be in between the subcommand and argument

# works
temporal operator namespace create --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN" mynewnamespace

# none of these work
temporal --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN" operator namespace create mynewnamespace
temporal operator --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN" namespace create mynewnamespace
temporal operator namespace --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN" create mynewnamespace
temporal operator namespace create mynewnamespace --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN"

Or temporal env where it needs to be between the subcommand and the two arguments:

# works
temporal env set --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN" local.x-a 1

# none of these work
temporal --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN" local.x-a env set 1
temporal env --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN" set local.x-a 1
temporal env set local.x-a --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN" 1
temporal env set local.x-a 1 --grpc-meta "Authorization=Bearer $TEMPORAL_TOKEN"

[cretz]

FWIW, the upcoming RC release will solve two of these issues: 1) there will not be a required flag order, and 2) there is an --api-key option to set bearer token (with associated env var).

[metavee]

That is great news! assuming it works for those of us not on Temporal cloud :D

[cretz]

@metavee - it will, it just is a shortcut for setting the header manually.

[metavee]

Wonderful, thank you

[josh-berry]

I'm realizing we have a small problem with temporal env, which is that -e is also used for --event-id in temporal workflow reset. So I'm going to make --env accept -E as a shorthand and use -E instead.

I am also going to drop the changes for cluster describe / cluster remove because I'm realizing the name may not actually be a synonym for frontend-address like I was thinking before. So I should have a PR ready for review shortly.

[cretz]

I'm realizing we have a small problem

Not a problem if we don't make new unnecessary aliases