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.

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

Implement workflow terminate command #436

Merged
merged 8 commits into from
Feb 7, 2024

Conversation

tdeebswihart
Copy link
Contributor

What was changed

I implemented temporal workflow terminate, which should behave the same as in the existing CLI except for the same caveats/improvements as in #432.

I extended the existing SingleWorkflowOrBatch functions to allow a command to opt-in to supplying a reason alongside a workflow id as terminate allows for this.

@tdeebswihart tdeebswihart requested a review from cretz February 5, 2024 23:37
Also ensure terminate with a workflow id works without a reason specified
Also use an options struct for workflowExecOrBatch
@tdeebswihart tdeebswihart requested a review from bergundy February 6, 2024 17:23
@@ -338,7 +338,7 @@ Includes options set for [payload input](#options-set-for-payload-input).
* `--run-id`, `-r` (string) - Run Id. Cannot be set when query is set.
* `--query`, `-q` (string) - Start a batch to Signal Workflow Executions with given List Filter. Either this or
Workflow Id must be set.
* `--reason` (string) - Reason to perform batch. Only allowed if query is present unless the command specifies otherwise.
* `--reason` (string) - Reason to perform batch. Only allowed if query is present unless the command specifies otherwise. Defaults to message with the current user's name.
Copy link
Member

Choose a reason for hiding this comment

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

This still mentions batch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're looking at the help for workflow signal rather than workflow terminate. That command only allows a reason to be specified when invoked in batch mode. If you check the options for workflow terminate you'll see I updated it there already:

* `--reason` (string) - Reason to terminate this workflow. Defaults to message with the current user's name.

temporalcli/commandsmd/commands.md Outdated Show resolved Hide resolved
@tdeebswihart tdeebswihart merged commit 9934738 into cli-rewrite Feb 7, 2024
5 checks passed
@tdeebswihart tdeebswihart deleted the tds/workflow-terminate branch February 7, 2024 22:17
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Nice! Just a couple of notes (though I know it's already merged, just stuff to discuss)

Comment on lines +406 to +410
* `--workflow-id`, `-w` (string) - Workflow Id. Either this or query must be set.
* `--run-id`, `-r` (string) - Run Id. Cannot be set when query is set.
* `--query`, `-q` (string) - Start a batch to terminate Workflow Executions with given List Filter. Either this or Workflow Id must be set.
* `--reason` (string) - Reason for termination. Defaults to message with the current user's name.
* `--yes`, `-y` (bool) - Confirm prompt to perform batch. Only allowed if query is present.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `--workflow-id`, `-w` (string) - Workflow Id. Either this or query must be set.
* `--run-id`, `-r` (string) - Run Id. Cannot be set when query is set.
* `--query`, `-q` (string) - Start a batch to terminate Workflow Executions with given List Filter. Either this or Workflow Id must be set.
* `--reason` (string) - Reason for termination. Defaults to message with the current user's name.
* `--yes`, `-y` (bool) - Confirm prompt to perform batch. Only allowed if query is present.
Includes options set for [single workflow or batch](#options-set-for-single-workflow-or-batch).

Or are you making this a completely separate options set just because of the English description of reason?

Copy link
Member

Choose a reason for hiding this comment

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

Note that #options-set-for-single-workflow-or-batch mentions signal and reason is applied only to batch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the discussion Roey and I had about this @cretz: #436 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

We can make docs more generic if we need to, but I have no problem copy/pasting just for docs-only reasons if we have to like here.

Comment on lines 158 to +160
namespace string,
cl client.Client,
overrides singleOrBatchOverrides,
Copy link
Member

Choose a reason for hiding this comment

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

Probably can toss all of this in a single struct instead of keeping overrides separate, but no biggie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants