Implement activity commands by dandavison · Pull Request #445 · temporalio/cli

dandavison · 2024-02-12T22:58:27Z

What was changed

Implement temporal activity fail and temporal activity complete

How was this tested

PR includes end-to-end tests
I modified samples-python:hello/hello_activity.py to sleep for a long time in the activity, issued CLI commands (examples below), and confirmed results in web UI.

Details

commit 5519021cac3a1d0b9818d903882e0e053ad3df8b (HEAD -> update-reapply)
Author: Dan Davison <dan.davison@temporal.io>
Date:   Mon Feb 12 18:02:34 2024 -0500

    Test cli activity commands

diff --git a/hello/hello_activity.py b/hello/hello_activity.py
index 6615b55..a8a8e54 100644
--- a/hello/hello_activity.py
+++ b/hello/hello_activity.py
@@ -21,6 +21,7 @@ class ComposeGreetingInput:
 @activity.defn
 async def compose_greeting(input: ComposeGreetingInput) -> str:
     activity.logger.info("Running activity with parameter %s" % input)
+    await asyncio.sleep(0xFFFFFFFF)
     return f"{input.greeting}, {input.name}!"
 
 
@@ -33,7 +34,8 @@ class GreetingWorkflow:
         return await workflow.execute_activity(
             compose_greeting,
             ComposeGreetingInput("Hello", name),
-            start_to_close_timeout=timedelta(seconds=10),
+            start_to_close_timeout=timedelta(seconds=0xFFFFFFFF),
+            activity_id="MyActivityId",
         )

Example commands tested

temporal-new activity complete --activity-id MyActivityId --workflow-id MyWorkflowId --result '{"resultkey": "resultval"}' --identity 'MyIdentity' --run-id e592b7a1-a15d-4de1-83b4-32ec070706fa
temporal-new activity fail --activity-id MyActivityId --workflow-id MyWorkflowId --detail '{"detailkey": "detailval"}' --run-id d9049fc2-ad6c-4f3e-9f04-52b287a7e913 --reason 'top-level-reason' --identity 'MyIdentity'

cretz

Some notes for now and future, but nothing blocking. The existing CLI sure was inconsistent/unclean with these two commands.

cretz · 2024-02-12T23:02:51Z

 * `--no-json-shorthand-payloads` (bool) - Always all payloads as raw payloads even if they are JSON.

+### temporal activity: Complete or fail an activity.
+


Suggested change

Activity commands operate on [Activity Executions](/concepts/what-is-an-activity-execution).

Activity commands follow this syntax:

`temporal activity [command] [command options]`

Just stole it from old CLI, but I guess not required (we hope to come back and clean these)

Yes, I'm not in love with that text from the old CLI. I think the help output is pretty clear as-is:

$ temporal activity -h Complete or fail an activity. Usage: temporal activity [command] Available Commands: complete Complete an activity. fail Fail an activity.

and these Markdown links [Activity Executions](/concepts/what-is-an-activity-execution) don't link to a valid target (unless I'm missing something?)

Not missing anything, I too am not a fan, but I was just leaving many docs things as is to come back and be cleaned up later. Can just leave off too.

cretz · 2024-02-12T23:04:11Z

+#### Options
+
+* `--activity-id` (string) - The Activity to be completed. Required.
+* `--identity` (string) - Identity of operator.


Previous CLI required this, but I am ok not requiring it

cretz · 2024-02-12T23:07:14Z

+
+* `--activity-id` (string) - The Activity to be completed. Required.
+* `--identity` (string) - Identity of operator.
+* `--result` (string) - The result with which to complete the Activity (JSON). Required.


This works for me to meet parity. There are a few things we may want in the future to be as robust as --input for workflows:

--result as a string[] to accept multiple results

--result-file to accept files instead of strings

--result-meta to support payload metadata

--result-base64 bool to support base64 binary strings

I wonder if we should create an issue here (same for --detail on failure)

cretz · 2024-02-12T23:09:28Z

Not sure this deserves its own file but meh

Yeah, I wasn't sure how people would like the code arranged. Where would you put it out of interest?

Probably client, but it doesn't matter. As the utils grow we can move/consolidate as needed

cretz · 2024-02-12T23:09:52Z

+	"go.temporal.io/api/common/v1"
+)
+
+func CreatePayloads(data [][]byte, metadata map[string][]byte, isBase64 bool) (*common.Payloads, error) {


Suggested change

func CreatePayloads(data [][]byte, metadata map[string][]byte, isBase64 bool) (*common.Payloads, error) {

func createPayloads(data [][]byte, metadata map[string][]byte, isBase64 bool) (*common.Payloads, error) {

Probably don't need to export (though it doesn't really matter in the context of an application vs a library so there's no rules here, so nothing required to change)

OK thanks, my Go style is a work-in-progress... when I uppercased it I was thinking that I was working in a separate "util" package from the main commands, but as you point out that's not the case.

cretz · 2024-02-12T23:14:43Z

+#### Options
+
+* `--activity-id` (string) - The Activity to be completed. Required.
+* `--identity` (string) - Identity of operator.


It was a bit inconsistent for us to have our CLI accept identity here but not on other commands like start workflow. I wonder if we should accept --identity higher up.

Agreed; I'd like consistency across all protos that have an identity field. I was just going for parity here.

cretz · 2024-02-12T23:15:37Z

+		return err
+	}
+
+	_, err = cl.WorkflowService().RespondActivityTaskCompletedById(cctx, &workflowservice.RespondActivityTaskCompletedByIdRequest{


While this is the approach of the old CLI, cl.CompleteActivityByID would have worked here too (you'd wrap the payload in rawValue). Arguably it'd be cleaner/clearer to use the high-level SDK client, but don't need to change now, but we might consider using the high-level API in the future.

OK I'm happy to make that change and it would be easy, however, it looks like it doesn't support identity so I'm going to leave it for now.

Usually smart clients set identity at the client level not the request level. That's what makes the --identity here so strange.

cretz · 2024-02-12T23:16:47Z

+	metadata := map[string][]byte{"encoding": []byte("json/plain")}
+	var detailPayloads *common.Payloads
+	detailPayloads = nil
+	if len(c.Detail) > 0 {
+		detailPayloads, err = CreatePayloads([][]byte{[]byte(c.Detail)}, metadata, false)


Suggested change

metadata := map[string][]byte{"encoding": []byte("json/plain")}

var detailPayloads *common.Payloads

detailPayloads = nil

if len(c.Detail) > 0 {

detailPayloads, err = CreatePayloads([][]byte{[]byte(c.Detail)}, metadata, false)

var detailPayloads *common.Payloads

if len(c.Detail) > 0 {

metadata := map[string][]byte{"encoding": []byte("json/plain")}

detailPayloads, err = CreatePayloads([][]byte{[]byte(c.Detail)}, metadata, false)

Little cleanup

dandavison · 2024-02-12T23:55:30Z

Thanks @cretz! Merged to keep the velocity up but there are some threads there we can follow-up in future PRs.

Implement activity complete and activity fail commands

5971ef3

dandavison requested a review from cretz February 12, 2024 22:58

cretz approved these changes Feb 12, 2024

View reviewed changes

Edit help text

94d36f1

dandavison mentioned this pull request Feb 12, 2024

[CLI Refresh] Consistent payload input options across all commands #446

Open

Code clean-up from @cretz

8d1cd8d

dandavison merged commit 678bd0b into temporalio:cli-rewrite Feb 12, 2024

josh-berry mentioned this pull request Apr 29, 2024

temporal 0.12.0 Homebrew/homebrew-core#170352

Merged

		* `--no-json-shorthand-payloads` (bool) - Always all payloads as raw payloads even if they are JSON.

		### temporal activity: Complete or fail an activity.

+Activity commands operate on [Activity Executions](/concepts/what-is-an-activity-execution).
+Activity commands follow this syntax:
+`temporal activity [command] [command options]`

	func CreatePayloads(data [][]byte, metadata map[string][]byte, isBase64 bool) (*common.Payloads, error) {
	func createPayloads(data [][]byte, metadata map[string][]byte, isBase64 bool) (*common.Payloads, error) {

Conversation

dandavison commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

How was this tested

Example commands tested

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

cretz Feb 12, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dandavison Feb 12, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Feb 12, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

cretz Feb 13, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dandavison commented Feb 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dandavison commented Feb 12, 2024 •

edited

Loading

cretz Feb 12, 2024 •

edited

Loading

dandavison Feb 12, 2024 •

edited

Loading

cretz Feb 12, 2024 •

edited

Loading

cretz Feb 13, 2024 •

edited

Loading