[CLI Refresh] Add build id commands #469
Conversation
There was a problem hiding this comment.
Would support combining this code and the next code into commands.taskqueue_buildids.go if you'd like since they're pretty small.
There was a problem hiding this comment.
I was wondering about that, but the commands are at different levels, keeping all the sub commands of update_build_id in a separate file makes it a bit easier to find...
There was a problem hiding this comment.
Sure, though we don't over-optimize for discoverability or we'd put every command in its own file (or force every sub command to be in a file with its adjacent ones). But this is fine. You may want to separate the tests too. For discoverability, often you prefer to test foo.go in foo_test.go.
| BuildIds []string | ||
| DefaultForSet string | ||
| IsDefaultSet bool |
There was a problem hiding this comment.
You'll probably want to provide "json:buildIds" type of struct tags for these since we usually want our JSON as camelCase (even if old CLI wasn't as kind/consistent).
| return cctx.Printer.PrintStructured(items, printer.StructuredOptions{Table: &printer.TableOptions{}}) | ||
| } | ||
|
|
||
| // Missing in the SDK? |
There was a problem hiding this comment.
Haven't had a need to convert these to strings yet in the SDK I guess, but would support a PR there to add a String() to this type
There was a problem hiding this comment.
This code is going away with the versioning rewrite, but I will make sure we have something like that after the rewrite...
| if cctx.JSONOutput { | ||
| return cctx.Printer.PrintStructured(items, printer.StructuredOptions{}) | ||
| } |
There was a problem hiding this comment.
Can just get rid of this. Println doesn't print when JSON set, and Table is ignored when JSON set, so the code below will do what you want. So basically PrintStructured is abstracted for reuse for just this kind of use case.
There was a problem hiding this comment.
Great, much cleaner thanks!
| func updateBuildIds(cctx *CommandContext, parent *TemporalTaskQueueCommand, options *client.UpdateWorkerBuildIdCompatibilityOptions) error { | ||
| cl, err := parent.ClientOptions.dialClient(cctx) |
There was a problem hiding this comment.
| func updateBuildIds(cctx *CommandContext, parent *TemporalTaskQueueCommand, options *client.UpdateWorkerBuildIdCompatibilityOptions) error { | |
| cl, err := parent.ClientOptions.dialClient(cctx) | |
| func (c *TemporalTaskQueueUpdateBuildIdsCommand) updateBuildIds(cctx *CommandContext, options *client.UpdateWorkerBuildIdCompatibilityOptions) error { | |
| cl, err := c.Parent.ClientOptions.dialClient(cctx) |
Can make the common piece the receiver if you wanted to just c.Parent.updateBuildIds(cctx, options) in other commands below without having a top-level function. But not required or anything.
There was a problem hiding this comment.
Didn't realize I had a base command built by the parser... That's great thanks!
There was a problem hiding this comment.
One minor thing about test file name at #469 (comment), nothing big, LGTM
What was changed
Implements
./temporal task-queue get-build-id-reachability
./temporal task-queue get-build-ids
./temporal task-queue update-build-ids add-new-compatible
./temporal task-queue update-build-ids add-new-default
./temporal task-queue update-build-ids promote-id-in-set
./temporal task-queue update-build-ids promote-set
Checklist
[CLI Refresh] Implement
temporal task-queuebuild-ID commands (update-andget-) #468