Add `temporal workflow signal-with-start` command by THardy98 · Pull Request #758 · temporalio/cli

THardy98 · 2025-02-20T21:02:05Z

Added temporal workflow signal-with-start command.

Description and sample usage:

  Send an asynchronous notification (Signal) to a Workflow Execution.
  If the Workflow Execution is not running or is not found, it starts the 
  workflow then sends the signal.

  ```
  temporal workflow signal-with-start \
    --signal-name YourSignal \
    --signal-input '{"some-key": "some-value"}' \
    --workflow-id YourWorkflowId \
    --type YourWorkflowType \
    --task-queue YourTaskQueue \
    --input '{"some-key": "some-value"}'
  ```

Closes [Feature Request] SignalWithStartWorkflow support #332
How was this tested:
Added unit tests
Any docs updates needed?
Yes

CLAassistant · 2025-02-20T21:02:11Z

All committers have signed the CLA.

CLAassistant · 2025-02-20T21:02:12Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Description and sample usage: Send an asynchronous notification (Signal) to a Workflow Execution. If the Workflow Execution is not running or is not found, it starts the workflow then sends the signal. ``` temporal workflow signal-with-start \ --signal-name YourSignal \ --signal-input '{"some-key": "some-value"}' \ --workflow-id YourWorkflowId \ --type YourWorkflowType \ --task-queue YourTaskQueue \ --input '{"some-key": "some-value"}' ```

THardy98 · 2025-02-20T22:29:34Z

Note: the command doesn't configure VersioningOverride in the signal-with-start request. It would basically need replicating the logic in Go SDK to convert it to proto. I opted to omit the duplication since it was marked experimental anyway and added a warning.

Let me know if we usually opt for a different approach here.

cretz · 2025-02-20T22:59:13Z

temporalcli/commands.workflow.go

+	if wfStartOpts.Memo != nil {
+		fields, err := encodeMapToPayloads(wfStartOpts.Memo)
+		if err != nil {
+			return err
+		}
+		memo = &common.Memo{Fields: fields}
+	}


I think this kind of logic needs to match temporal workflow start (which uses stringKeysJSONValues for memo). In fact, looking at this in general, is there a reason not to use the Go SDK's high-level SignalWithStartWorkflow call instead of the raw RPC? We use the high-level ExecuteWorkflow call for other workflow start. Then you can just reuse the buildStartOptions helper that exists. Also I would consider moving this into commands.workflow_exec.go with its other start workflow friends.

In fact, looking at this in general, is there a reason not to use the Go SDK's high-level SignalWithStartWorkflow call instead of the raw RPC?

There's a comment above the workflow service call below - SignalWithStartWorkflow only accepts a single arg for signal. The existing signal command on CLI uses the workflow service call because of this. I think it makes sense to provide the flexibility for multiple signal args and stay consistent with the existing CLI signal command

I see, that is unfortunate that Go SDK is that way but I agree this needs to support multiple signal arguments. I fear that people that implement a new start option may not know to come update this. But I cannot think of a better way short of having Go SDK support a signal-with-start that takes multiple signal parameters. @Quinn-With-Two-Ns - thoughts here?

I do think it still may make more sense to move that encodeMapToPayloads utility that used to be for schedules only into commands.go for more generic use, and maybe move this command impl to commands.workflow_exec.go next to the other commands that work with the workflow start options.

signal-with-start moved to commands.workflow_exec.go and encodeMapToPayloads to commands.go

cretz

Only minor suggestions regarding moving code a bit and providing output.

I do wonder if we should do update-with-start at the same time in case we learn something about the approach we take there that can affect signal-with-start. But maybe not since the result types/approaches are so different.

cretz · 2025-02-20T23:15:28Z

temporalcli/commands.workflow.go

+	if wfStartOpts.Memo != nil {
+		fields, err := encodeMapToPayloads(wfStartOpts.Memo)
+		if err != nil {
+			return err
+		}
+		memo = &common.Memo{Fields: fields}
+	}


I see, that is unfortunate that Go SDK is that way but I agree this needs to support multiple signal arguments. I fear that people that implement a new start option may not know to come update this. But I cannot think of a better way short of having Go SDK support a signal-with-start that takes multiple signal parameters. @Quinn-With-Two-Ns - thoughts here?

I do think it still may make more sense to move that encodeMapToPayloads utility that used to be for schedules only into commands.go for more generic use, and maybe move this command impl to commands.workflow_exec.go next to the other commands that work with the workflow start options.

cretz · 2025-02-20T23:27:42Z

temporalcli/commands.workflow.go

+			WorkflowIdConflictPolicy: wfStartOpts.WorkflowIDConflictPolicy,
+		},
+	)
+	return err


The startWorkflow helper when called from workflow start prints a structured output like:

cctx.Printer.Println(color.MagentaString("Running execution:")) err := cctx.Printer.PrintStructured(struct { WorkflowId string `json:"workflowId"` RunId string `json:"runId"` Type string `json:"type"` Namespace string `json:"namespace"` TaskQueue string `json:"taskQueue"` }{ WorkflowId: run.GetID(), RunId: run.GetRunID(), Type: sharedWorkflowOpts.Type, Namespace: c.Namespace, TaskQueue: sharedWorkflowOpts.TaskQueue, }, printer.StructuredOptions{})

I think we should match that output here too (not much code reuse though, so just need to copy the logic probably)

Added (along with a check in test for expected output)

cretz

This LGTM. Only reason I hesitate to mark approved is because I'd like to see at least a high-level discussed/approved design of update with start in CLI just to make sure we're not missing anything.

cretz · 2025-02-21T16:18:01Z

temporalcli/commands.workflow_exec.go

+	err = cctx.Printer.PrintStructured(struct {
+		WorkflowId string `json:"workflowId"`
+		RunId      string `json:"runId"`
+		Type       string `json:"type"`
+		Namespace  string `json:"namespace"`
+		TaskQueue  string `json:"taskQueue"`
+	}{
+		WorkflowId: c.WorkflowId,
+		RunId:      resp.RunId,
+		Type:       c.Type,
+		Namespace:  c.Parent.Namespace,
+		TaskQueue:  c.TaskQueue,
+	}, printer.StructuredOptions{})
+	if err != nil {
+		return fmt.Errorf("failed printing: %w", err)
+	}
+	return nil


Suggested change

err = cctx.Printer.PrintStructured(struct {

WorkflowId string `json:"workflowId"`

RunId string `json:"runId"`

Type string `json:"type"`

Namespace string `json:"namespace"`

TaskQueue string `json:"taskQueue"`

}{

WorkflowId: c.WorkflowId,

RunId: resp.RunId,

Type: c.Type,

Namespace: c.Parent.Namespace,

TaskQueue: c.TaskQueue,

}, printer.StructuredOptions{})

if err != nil {

return fmt.Errorf("failed printing: %w", err)

}

return nil

return cctx.Printer.PrintStructured(struct {

WorkflowId string `json:"workflowId"`

RunId string `json:"runId"`

Type string `json:"type"`

Namespace string `json:"namespace"`

TaskQueue string `json:"taskQueue"`

}{

WorkflowId: c.WorkflowId,

RunId: resp.RunId,

Type: c.Type,

Namespace: c.Parent.Namespace,

TaskQueue: c.TaskQueue,

}, printer.StructuredOptions{})

Pedantic, but don't really need to wrap printing failures I don't think (even though we did in some other places)

THardy98 · 2025-02-21T16:27:11Z

This LGTM. Only reason I hesitate to mark approved is because I'd like to see at least a high-level discussed/approved design of update with start in CLI just to make sure we're not missing anything.

Ok, I can put up a separate update-with-start PR before this gets merged to get a better picture.

cretz

After internal discussion, this is like what we'll do with update-with-start, LGTM

THardy98 requested a review from a team February 20, 2025 21:02

THardy98 force-pushed the signal-with-start branch from 7cd38b6 to b1da783 Compare February 20, 2025 21:04

THardy98 added 2 commits February 20, 2025 17:02

fix tests, add warning

5c0a899

THardy98 force-pushed the signal-with-start branch from 5216d71 to 5c0a899 Compare February 20, 2025 22:02

cretz reviewed Feb 20, 2025

View reviewed changes

THardy98 changed the title ~~Add Workflow commands perform operations on Workflow Executions:~~ Add temporal workflow signal-with-start command Feb 20, 2025

THardy98 changed the title ~~Add temporal workflow signal-with-start command~~ Add temporal workflow signal-with-start command Feb 20, 2025

cretz reviewed Feb 20, 2025

View reviewed changes

Code movement, added workflow execution print

2667307

cretz reviewed Feb 21, 2025

View reviewed changes

remove error wrapping

fa0ea38

cretz approved these changes Feb 21, 2025

View reviewed changes

Merge branch 'main' into signal-with-start

73b2b73

THardy98 merged commit f968fa3 into main Feb 21, 2025
8 checks passed

THardy98 deleted the signal-with-start branch February 21, 2025 18:38

BrewTestBot mentioned this pull request Feb 27, 2025

temporal 1.3.0 Homebrew/homebrew-core#209202

Merged

Conversation

THardy98 commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Feb 20, 2025

Uh oh!

THardy98 commented Feb 20, 2025

Uh oh!

cretz Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

cretz Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

cretz Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

THardy98 Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

THardy98 commented Feb 21, 2025

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

THardy98 commented Feb 20, 2025 •

edited

Loading

CLAassistant commented Feb 20, 2025 •

edited

Loading

cretz Feb 20, 2025 •

edited

Loading

THardy98 Feb 20, 2025 •

edited

Loading

cretz Feb 20, 2025 •

edited

Loading

cretz Feb 20, 2025 •

edited

Loading

cretz Feb 20, 2025 •

edited

Loading