-
Notifications
You must be signed in to change notification settings - Fork 36
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 activity commands #445
Implement activity commands #445
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes for now and future, but nothing blocking. The existing CLI sure was inconsistent/unclean with these two commands.
@@ -57,6 +57,43 @@ This document has a specific structure used by a parser. Here are the rules: | |||
* `--color` (string-enum) - Set coloring. Options: always, never, auto. Default: auto. | |||
* `--no-json-shorthand-payloads` (bool) - Always all payloads as raw payloads even if they are JSON. | |||
|
|||
### temporal activity: Complete or fail an activity. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
temporalcli/commandsmd/commands.md
Outdated
#### Options | ||
|
||
* `--activity-id` (string) - The Activity to be completed. Required. | ||
* `--identity` (string) - Identity of operator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous CLI required this, but I am ok not requiring it
|
||
* `--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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 astring[]
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this deserves its own file but meh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wasn't sure how people would like the code arranged. Where would you put it out of interest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably client, but it doesn't matter. As the utils grow we can move/consolidate as needed
"go.temporal.io/api/common/v1" | ||
) | ||
|
||
func CreatePayloads(data [][]byte, metadata map[string][]byte, isBase64 bool) (*common.Payloads, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
temporalcli/commandsmd/commands.md
Outdated
#### Options | ||
|
||
* `--activity-id` (string) - The Activity to be completed. Required. | ||
* `--identity` (string) - Identity of operator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed; I'd like consistency across all protos that have an identity
field. I was just going for parity here.
return err | ||
} | ||
|
||
_, err = cl.WorkflowService().RespondActivityTaskCompletedById(cctx, &workflowservice.RespondActivityTaskCompletedByIdRequest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually smart clients set identity at the client level not the request level. That's what makes the --identity
here so strange.
temporalcli/commands.activity.go
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks @cretz! Merged to keep the velocity up but there are some threads there we can follow-up in future PRs. |
What was changed
Implement
temporal activity fail
andtemporal activity complete
How was this tested
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.Example commands tested