Conversation
…es, and enhanced `workflow describe` (temporalio#735)
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM, nothing blocking. I admit I didn't review the models too in depth, so long as we're reasonably comfortable with the JSON that is emitted will be stable (no problem with pre-GA incompatible changes, I just mean that we're happy in general with the shape/approach).
| ) | ||
| s.NoError(res.Err) | ||
|
|
||
| // TODO(antlai-temporal): Delete when a server caching bug in 1.26.2 is fixed |
There was a problem hiding this comment.
Can you link to the issue (ideally in the code comment, but here is fine too)? And/or can you just make a CLI issue that we try not to lose that removes these sleeps once whatever upstream issue is fixed?
temporalcli/commands.workflow.go
Outdated
| var workflowExecutionOptions *workflowpb.WorkflowExecutionOptions | ||
| protoMask, err := fieldmaskpb.New(workflowExecutionOptions, "versioning_override") | ||
| if err != nil { | ||
| panic("invalid field mask") |
There was a problem hiding this comment.
While I know this thing shouldn't happen, I think we can still use regular errors here
josh-berry
left a comment
josh-berry
left a comment
There was a problem hiding this comment.
Part one of my review, just the API/YAML. Dropping quickly so you have more time to consider/respond. Will look at the actual code next.
| @@ -0,0 +1,377 @@ | |||
| package temporalcli | |||
There was a problem hiding this comment.
I have a nit about the file name: if this is temporal worker deployment ..., we should probably name this file commands.worker.deployment.go. I know it's a mouthful but it's at least consistent.
There was a problem hiding this comment.
Renaming...
| - name: temporal worker | ||
| summary: Read or update Worker state | ||
| description: | | ||
| Modify or read state associated with a Worker, for example, | ||
| using Worker Deployments commands: | ||
|
|
||
| ``` | ||
| temporal worker deployment | ||
| ``` |
There was a problem hiding this comment.
Let's also mark this as experimental; no need to stabilize something unnecessarily. :)
| - worker | ||
| - worker deployment | ||
|
|
||
| - name: temporal worker deployment |
There was a problem hiding this comment.
Should we also deprecate the old task-queue versioning etc. commands? Or are those still relevant?
There was a problem hiding this comment.
Not yet, we have not deprecated in the go sdk, the consensus was to wait for public preview...
temporalcli/commandsgen/commands.yml
Outdated
| - name: report-reachability | ||
| type: bool | ||
| description: | | ||
| Flag to include reachability information of a Worker Deployment. |
There was a problem hiding this comment.
You don't need to say it's a flag. That's obvious in context:
| Flag to include reachability information of a Worker Deployment. | |
| Include reachability information of a Worker Deployment. |
There was a problem hiding this comment.
Thanks, much better
| type: int | ||
| description: Maximum number of Workflow Executions to display. | ||
|
|
||
| - name: temporal workflow update-options |
There was a problem hiding this comment.
[non-blocking/feel free to ignore] Would it be better to call this set-options so as not to confuse it with Workflow Update? Or maybe even options set if we think we'll add options list, options show etc. later?
[edit: I see there was extensive discussion about this in the PR merging this command into the versioning-3 branch. I missed that discussion, but I doubly don't want to hold up the bus in light of the fact it's already been thought about extensively. :)]
There was a problem hiding this comment.
I don't want to open the can of worms again... It took hours...If we decide to change things it should start with the API and align with activity options...
| summary: Change Workflow Execution Options | ||
| description: | | ||
| Modify properties of Workflow Executions: | ||
|
|
||
| ``` | ||
| temporal workflow update-options [options] | ||
| ``` |
There was a problem hiding this comment.
Good catch, thanks
temporalcli/commandsgen/commands.yml
Outdated
| - name: versioning-override-behavior | ||
| type: string-enum | ||
| description: | | ||
| Flag to override the versioning behavior of a Workflow. |
There was a problem hiding this comment.
| Flag to override the versioning behavior of a Workflow. | |
| Override the versioning behavior of a Workflow. |
Also: I'm a little unclear on what "override" means here—it might be good to explain what each of the enum values mean.
There was a problem hiding this comment.
Thanks, I modified the description to give examples of the enum values.
josh-berry
left a comment
josh-berry
left a comment
There was a problem hiding this comment.
Took a quick pass at the actual code and the accompanying tests and I have no more comments, the implementation LGTM. 👍
3 participants
What was changed
This is a merge of the branch
versioning-3into main.It adds support for the new Deployment API, extends Describe Workflow with versioning info, and
provides a versioning override API.
See the PR #735 for further details.
Why?
Checklist
[Feature Request] versioning override and move API #720 [Feature Request] DescribeWorkflow should show versioning info #719 [Feature Request] Support the deployment API #718