Implement update/get worker build id commands #166
Conversation
Sushisource
force-pushed
the
update-build-id-sets
branch
from
2334871 to
ae08584
Compare
Sushisource
force-pushed
the
update-build-id-sets
branch
from
5f8e594 to
4be2220
Compare
There was a problem hiding this comment.
@Sushisource can you please add tests?
We can use the built-in dev server for that.
Other than than the command structure LGTM.
I mostly focused on that, not on the implementation.
Sushisource
force-pushed
the
update-build-id-sets
branch
2 times, most recently
from
8509498 to
8ff72e6
Compare
app/build_id_compat_cli_test.go
Outdated
| portProvider := sconfig.NewPortProvider() | ||
| port := portProvider.MustGetFreePort() | ||
| portProvider.Close() | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| s.stopServerCancel = cancel | ||
|
|
||
| args, clientOpts := newServerAndClientOpts(port) | ||
|
|
||
| go func() { | ||
| if err := s.app.RunContext(ctx, args); err != nil { | ||
| fmt.Println("Server closed with error:", err) | ||
| } | ||
| }() | ||
|
|
||
| s.client = assertServerHealth(s.T(), ctx, clientOpts) |
There was a problem hiding this comment.
@feedmeapples we can replace all of this boilerplate with https://pkg.go.dev/go.temporal.io/sdk@v1.21.1/testsuite#StartDevServer
There was a problem hiding this comment.
It would require a compiled binary though.
There was a problem hiding this comment.
|
|
||
| // Update build id subcommand definitions | ||
| AddNewDefaultBuildIDDefinition = "Add a new default (incompatible) build ID to the Task Queue version sets." | ||
| AddNewDefaultBuildIDDefinitionUsage = "Creates a new build id set which will become the new overall default for the queue with the provided build id as its only member. This new set is incompatible with all previous sets/versions." |
There was a problem hiding this comment.
nit:
| AddNewDefaultBuildIDDefinitionUsage = "Creates a new build id set which will become the new overall default for the queue with the provided build id as its only member. This new set is incompatible with all previous sets/versions." | |
| AddNewDefaultBuildIDDefinitionUsage = "Creates a new build ID set which will become the new overall default for the queue with the provided build ID as its only member. This new set is incompatible with all previous sets/versions." |
I would also see how this renders and consider adding line breaks.
There was a problem hiding this comment.
Any semi-competent CLI flags library should be able to do this automatically (which this one maybe barely qualifies as lol). It looks like the one we're using has some kind of support for that: https://github.com/urfave/cli/pull/1119/files#diff-4d3f36d62cad94424544bb84fc93b33b73ffe3491a4ddca81a7f825539afc19cR1161-R1169
So we might consider doing that in another PR rather than manually inserting line breaks in a few places.
There was a problem hiding this comment.
Some usage examples would be great too @Sushisource but it already LGTM
|
We should avoid merging it though until the functionality exists on the server. |
Sushisource
force-pushed
the
update-build-id-sets
branch
2 times, most recently
from
10b034b to
264a56d
Compare
Sushisource
force-pushed
the
update-build-id-sets
branch
3 times, most recently
from
d1eb403 to
ee019ab
Compare
| @@ -105,3 +105,11 @@ func (s *buildIdCompatSuite) TestPromoteBuildIdInSet() { | |||
| "--build-id", "foo")) | |||
| s.Nil(err) | |||
| } | |||
|
|
|||
| func (s *buildIdCompatSuite) TestReachability() { | |||
There was a problem hiding this comment.
Can we test the effect of this and not just that it doesn't return an error?
Remove unneeded ldflag
Sushisource
force-pushed
the
update-build-id-sets
branch
from
da2c64f to
6fb70d3
Compare
Sushisource
force-pushed
the
update-build-id-sets
branch
from
8c6f518 to
8e2e5f0
Compare
Draft until temporalio/temporal#4086 is merged and I can get rid of my go.mod replace
What was changed
Added commands for updating and fetching worker-build-id compatibility sets on task queues.
Why?
New API!
Checklist
Closes Implement CLI command for updating version sets #165
How was this tested:
Manually verified against new server
Any docs updates needed?
Yes. I'll be doing a docs PR as well.