CLI Refresh: `workflow signal` command #432

cretz · 2024-01-30T22:43:32Z

What was changed

Add support for workflow signal command. The functionality is mostly unchanged from current CLI, but the output may be different.

Improvements over current CLI:

JSON output support (empty for single-workflow, contains batch job ID for batch)
Use non-success exit code if prompt confirmation fails on batch
Clarify batch count is approximate (due to race between count request and actual batch)
--reason not required for batch (same default format as UI)

…write-workflow-signal # Conflicts: # go.mod # go.sum # temporalcli/commands.gen.go # temporalcli/commands.workflow.go # temporalcli/commands.workflow_exec.go # temporalcli/commandsmd/commands.md

temporalcli/commands.workflow.go

cretz · 2024-01-30T23:11:19Z

temporalcli/commands_test.go

+		lastFoundPieceIndex := -1
 		for _, piece := range pieces {
-			if !strings.Contains(line, piece) {
+			if index := strings.Index(line, piece); index > lastFoundPieceIndex {
+				lastFoundPieceIndex = index
+			} else {


@dandavison - here is update for forcing order

Great, thanks for adding that! This looks like it will allow the pieces to match in an overlapping way -- I think we need to set lastFoundPieceIndex to the index of the end of piece.

I think it's ok to be overlapping, it's still in order by start of piece

No, it's clear to me that when users make a call like s.ContainsOnSameLine(out, "Namespace", "default") they mean

there exists a line which contains 'Namespace' and 'default', in that order (not overlapping).

I've never seen a test framework that allows them to be overlapping or out of order. I do think that your s.ContainsOnSameLine(out, "Namespace", "default") call is nice and readable -- nicer to read than a regexp assertion for this common case.

cretz · 2024-01-30T23:12:22Z

temporalcli/commands.workflow.go

+	// Default the reason if not set
+	reason := s.Reason
+	if reason == "" {
+		username := "<unknown-user>"
+		if u, err := user.Current(); err != nil && u.Username != "" {
+			username = u.Username
+		}
+		reason = "Requested from CLI by " + username
+	}


Current CLI requires reason to be set by user. UI defaults to something like Terminated from Web UI by foo@bar.com, so we follow a similar format, but we just do "Requested" instead of specific verb (batch job carries its own timestamp info).

dandavison · 2024-01-31T17:32:53Z

temporalcli/commands_test.go

-			if !strings.Contains(line, piece) {
+			if index := strings.Index(line, piece); index > lastEndIndex {
+				lastEndIndex = index + len(piece)
+			} else {


Ha, algorithms like this are hard to get right first time without unit tests! I suggest we use a regular-expression based implementation, i.e. form a regex by joining pieces with whatever separator gives the separator semantics that we think callers will want in the common case. That way the behavior of the utility is clear to anyone who looks at the code, and they can use their own regex assertion if they want different behavior. I'd also be OK with deleting this utility and requiring callers to use a regex explicitly.

As it is it still has bugs. The most serious I'm seeing is that ContainsOnSameLine("a b a", "b", "a") will fail, when it should pass.

But also ContainsOnSameLine("ab", "a", "b") will fail (no gap between them) and yet ContainsOnSameLine("axb", "a", "b") will pass.

And related to the first bug, ContainsOnSameLine("a a", "a", "a") will fail.

This is why I wanted it simple and ask users to use regex if they needed more instead of increase the rules of this little helper. I regret not sticking to the original simple/looser requirements. At this point I think we should just document the limits and tell people to use regex if they want.

The original version allowed the pieces to be in any order and overlapping, which isn't appropriate for a test harness assertion utility.

What do you see as the downside of implementing ContainsOnSameLine as a regex search for pieces joined by something like .* or .+ or .*\s.*? However it's implemented, the resulting function will have some behavior, which we need to make clear to callers. A regex is the best way to make that clear to callers as I see it.

Or, as I mentioned above, we could just delete the utility and let callers decide on their regex.

What do you see as the downside of implementing ContainsOnSameLine as a regex search for pieces joined by something like .* or .+ or .\s.?

The continual requirements. "Can you strip colors too?", "Can you make it case insensitive?", "Can you make it only allow whole words instead of partial match?", "Can you allow this to work on adjacent lines instead of just one?", etc etc. "But it isn't appropriate for an stdout test harness utility to not strip out ANSI codes", "But it isn't appropriate for this test harness utility to not check whole words", etc. Each has their own opinion and it was just meant to be a little code reuse thing, but I didn't want to name it ContainsAnywhereOnSameLineInAnyOrderMayOverlap.

Or, as I mentioned above, we could just delete the utility and let callers decide on their regex.

This may be best, but I still want some code reuse and I accept the limitations of the little helper.

The continual requirements. "Can you strip colors too?", "Can you make it case insensitive?", "Can you make it only allow whole words instead of partial match?", "Can you allow this to work on adjacent lines instead of just one?", etc etc. "But it isn't appropriate for an stdout test harness utility to not strip out ANSI codes", "But it isn't appropriate for this test harness utility to not check whole words", etc.

My suggestions that a string matching function should enforce order of the query terms and not allow overlapping matches are perfectly reasonable and uncontroversial. I'm not proposing them as optional extra enhancements; I'm saying that these are basic requirements of a utility that searches for a line containing certain substrings.

In any case, it's not appropriate to attempt to ridicule reviewer suggestions like that. If you think that the suggestions are reasonable but not how you want the function to behave, then that’s of course fine – in that case please just ask the other team members for their opinion.

Updated to support regex-based match

Thanks, that looks great to me.

bergundy

LGTM.

Not much to add.

Only thing I'm unsure of is if we want to print the success on non-json output via the logger to stderr.
Seems like the rest of the output for this command goes to stdout and if I want to grep for the job ID (yes some prefer that over jq), I have to pipe stderr which is a bit awkward and non-standard.

dandavison

As discussed, there are bugs in the test helper function that need fixing.

cretz · 2024-02-01T18:42:07Z

Only thing I'm unsure of is if we want to print the success on non-json output via the logger to stderr.

I will update the started-batch statement to be a stdout line instead of a log line

EDIT: Updated

# 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.

dandavison · 2024-02-10T12:23:25Z

temporalcli/commands.workflow.go

+		return err
+	}
+
+	exec, batchReq, err := c.workflowExecOrBatch(cctx, c.Parent.Namespace, cl)


Did this just happen to be the first command implemented for which batch was an option?

dandavison · 2024-02-10T12:25:25Z

temporalcli/commands.workflow.go

+		if err := startBatchJob(cctx, cl, batchReq); err != nil {
+			return err
+		}
+	}


I believe that the contract of workflowExecOrBatch is such that one or other of the above conditions must be true, so I'd like the code not to appear to allow neither to be executed.

Yes, this is leftover from previous code where it could return all nils when you declined the prompt, but I decided to change that to be an error and did not change this.

dandavison · 2024-02-10T12:26:25Z

temporalcli/commands.workflow.go

@@ -41,3 +95,77 @@ func (*TemporalWorkflowTraceCommand) run(*CommandContext, []string) error {
 func (*TemporalWorkflowUpdateCommand) run(*CommandContext, []string) error {
 	return fmt.Errorf("TODO")
 }
+


Although it's a private function, it would be good to document this something like

This function guarantees that exactly one of the 3 returned values will be non-nil.

OK great, I can make these minor changes.

# What was changed I implemented `temporal workflow cancel`, which should behave the same as in the existing CLI except for the same caveats/improvements as in #432

cretz added 6 commits December 15, 2023 14:46

Work on CLI

4e34c90

More implementations and reworking

c4b1ff6

README, CONTRIBUTING, CI, and other minor things

fcc6137

PR-suggested updates

a4fcf9b

Single-workflow signal command

01ed375

Merge remote-tracking branch 'remotes/origin/cli-rewrite' into cli-re…

2d07efc

…write-workflow-signal # Conflicts: # go.mod # go.sum # temporalcli/commands.gen.go # temporalcli/commands.workflow.go # temporalcli/commands.workflow_exec.go # temporalcli/commandsmd/commands.md

cretz requested a review from a team January 30, 2024 22:43

Add batch support to signal

c27034f

cretz force-pushed the cli-rewrite-workflow-signal branch from 0819721 to c27034f Compare January 30, 2024 22:45

cretz commented Jan 30, 2024

View reviewed changes

temporalcli/commands.workflow.go Outdated Show resolved Hide resolved

Don't require reason and fix eventually assertion

16d1e00

cretz commented Jan 30, 2024

View reviewed changes

Update harness helper

2f91ff0

dandavison reviewed Jan 31, 2024

View reviewed changes

bergundy approved these changes Feb 1, 2024

View reviewed changes

dandavison requested changes Feb 1, 2024

View reviewed changes

PR suggested updates

3a7b209

dandavison approved these changes Feb 1, 2024

View reviewed changes

cretz merged commit 7ca8679 into cli-rewrite Feb 1, 2024
5 checks passed

cretz deleted the cli-rewrite-workflow-signal branch February 1, 2024 21:47

This was referenced Feb 5, 2024

Implement workflow cancel command #435

Merged

Implement workflow terminate command #436

Merged

dandavison reviewed Feb 10, 2024

View reviewed changes

tdeebswihart added a commit that referenced this pull request Feb 12, 2024

Implement workflow cancel command (#435)

b98627b

# What was changed I implemented `temporal workflow cancel`, which should behave the same as in the existing CLI except for the same caveats/improvements as in #432

josh-berry mentioned this pull request Apr 29, 2024

temporal 0.12.0 Homebrew/homebrew-core#170352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI Refresh: `workflow signal` command #432

CLI Refresh: `workflow signal` command #432

cretz commented Jan 30, 2024 •

edited

Loading

cretz Jan 30, 2024

dandavison Jan 30, 2024

cretz Jan 30, 2024

dandavison Jan 30, 2024 •

edited

Loading

cretz Jan 31, 2024

cretz Jan 30, 2024 •

edited

Loading

dandavison Jan 31, 2024 •

edited

Loading

cretz Jan 31, 2024 •

edited

Loading

dandavison Jan 31, 2024

cretz Jan 31, 2024

dandavison Feb 1, 2024 •

edited

Loading

cretz Feb 1, 2024

dandavison Feb 1, 2024

bergundy left a comment

dandavison left a comment

cretz commented Feb 1, 2024 •

edited

Loading

dandavison Feb 10, 2024

cretz Feb 12, 2024

dandavison Feb 10, 2024

cretz Feb 12, 2024

dandavison Feb 10, 2024

cretz Feb 12, 2024

dandavison Feb 12, 2024

CLI Refresh: workflow signal command #432

CLI Refresh: workflow signal command #432

Conversation

cretz commented Jan 30, 2024 • edited Loading

What was changed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

dandavison Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

cretz Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy left a comment

Choose a reason for hiding this comment

dandavison left a comment

Choose a reason for hiding this comment

cretz commented Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLI Refresh: `workflow signal` command #432

CLI Refresh: `workflow signal` command #432

cretz commented Jan 30, 2024 •

edited

Loading

dandavison Jan 30, 2024 •

edited

Loading

cretz Jan 30, 2024 •

edited

Loading

dandavison Jan 31, 2024 •

edited

Loading

cretz Jan 31, 2024 •

edited

Loading

dandavison Feb 1, 2024 •

edited

Loading

cretz commented Feb 1, 2024 •

edited

Loading