-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add workflow update result
, update describe
and result
commands
#658
Conversation
temporalcli/commandsmd/commands.md
Outdated
An Update is a synchronous call to a Workflow Execution that can change its | ||
state, control its flow, and return a result. | ||
|
||
Experimental. | ||
|
||
#### Options set for update targeting |
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.
The named options set should be under the first command that uses it instead of this command. This now shows that this command accepts these args. Can move and replace the #### Options
section for temporal workflow update describe
with this section.
temporalcli/commandsmd/commands.md
Outdated
* `--workflow-id`, `-w` (string) - | ||
Workflow ID. | ||
Required. | ||
* `--update-id` (string) - |
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 should be required for describe
(and therefore reused for result
and make start
be the unnamed set that has a UUID default)
temporalcli/structured_errors.go
Outdated
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.
Meh, just toss this at the bottom of commands.go
IMO
57bf705
to
38bb8ef
Compare
temporalcli/commands.go
Outdated
return &structuredError{ | ||
Message: err.Error(), | ||
Type: err.Type(), | ||
Details: err.Details(), |
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.
.Details
expects a pointer to deserialize into and returns an error if it can't. You could have var details any
and then err.Details(&details)
(checking error) and then set details
to in this field
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.
Ah thanks good catch
temporalcli/commands.go
Outdated
@@ -5,6 +5,7 @@ import ( | |||
"context" | |||
"encoding/json" | |||
"fmt" | |||
"go.temporal.io/sdk/temporal" |
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 go fmt will get these non-stdlib imports separated from the stdlib ones (here and elsewhere in PR)
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 doesn't for me, for whatever reason.
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.
Hrmm, maybe it was goimports
or similar that I just assumed was in go fmt
by 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.
LGTM, but would like someone that has an eye for CLI output to take a peek (@dandavison?)
1a605b7
to
d781eb9
Compare
Filed #667 for an unrelated failed test on Windows that happened to show up in this PR. |
Took a quick look and it seems reasonable to me. Merging so this can be included in the CLI release on Monday. |
This is functionally complete but certainly open to discussion on the details or output.
What was changed
Added these new commands
Why?
More consistent experience for related concepts
Checklist
Closes [Feature Request] Add
workflow result
,workflow update result
, andworkflow update describe
#647How was this tested:
Added tests
Any docs updates needed?
Here are some examples of what output looks like from the tests:
For
workflow result
:For
update describe
:For
update result
: