Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add versioning API methods to client #920

Merged
merged 19 commits into from
Mar 13, 2023
Merged

Add versioning API methods to client #920

merged 19 commits into from
Mar 13, 2023

Conversation

Sushisource
Copy link
Member

What was changed

Added top-level methods for the versioning API to the client.
Added worker flag to opt in to the feature

Go fmt also seemed to get a little more... aggressive? Some changes b/c of that.

Why?

So we can use them in tctl and eventually release

Checklist

Relates to temporalio/cli#165

  1. Closes

  2. How was this tested:
    Added some tests

  3. Any docs updates needed?

@Sushisource Sushisource requested a review from a team as a code owner September 27, 2022 19:41
@Sushisource Sushisource mentioned this pull request Sep 27, 2022
Closed
@Sushisource Sushisource marked this pull request as draft September 27, 2022 19:41
@Sushisource
Copy link
Member Author

Sushisource commented Sep 27, 2022 •
edited
Loading

Draft until API changes merged

@Sushisource Sushisource force-pushed the versioning-api-refactor branch from eb9aeb6 to 0d67096 Compare September 28, 2022 17:26
@Sushisource Sushisource force-pushed the versioning-api-refactor branch from 0d67096 to 5bcc013 Compare February 22, 2023 23:38
@Sushisource Sushisource force-pushed the versioning-api-refactor branch from 5bcc013 to e655cd3 Compare March 13, 2023 16:45
@Sushisource Sushisource marked this pull request as ready for review March 13, 2023 16:46
Quinn-With-Two-Ns
Quinn-With-Two-Ns reviewed Mar 13, 2023
View reviewed changes
client/client.go Outdated
// And it will immediately terminating the current execution instance.
// RequestId is used to deduplicate requests. It will be autogenerated if not set.
ResetWorkflowExecution(ctx context.Context, request *workflowservice.ResetWorkflowExecutionRequest) (*workflowservice.ResetWorkflowExecutionResponse, error)

// UpdateWorkerBuildIDCompatability **IN DEVELOPMENT**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For schedules we tagged things as // NOTE: Experimental. Might want to be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the struct like UpdateWorkerBuildIDCompatabilityOptions done or are they still in development? If they are still in development we should mark them as such

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured that's sort of implied by their 1:1 tie with the in-dev client functions, but in any case added the note there too 👍

Quinn-With-Two-Ns
Quinn-With-Two-Ns reviewed Mar 13, 2023
View reviewed changes
go.mod Outdated
@@ -21,3 +21,5 @@ require (
google.golang.org/protobuf v1.28.1
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect
)

replace go.temporal.io/api => /mnt/chonky/dev/temporal/api-go
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chonky

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 (of course will go away once API change is merged)

Quinn-With-Two-Ns
Quinn-With-Two-Ns reviewed Mar 13, 2023
View reviewed changes
internal/client.go Outdated
@@ -336,6 +336,14 @@ type (
// RequestId is used to deduplicate requests. It will be autogenerated if not set.
ResetWorkflowExecution(ctx context.Context, request *workflowservice.ResetWorkflowExecutionRequest) (*workflowservice.ResetWorkflowExecutionResponse, error)

// UpdateWorkerBuildIDCompatability allows you to update the worker-build-id based version sets for a particular
// task queue. This is used in conjunction with workers who specify their build id and thus opt into the
// feature. For more, see: <doc link>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have docs links? otherwise I would remove or leave a not saying they are coming.

Copy link
Member Author

@Sushisource Sushisource Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, makes sense. (removed everywhere)

Quinn-With-Two-Ns
Quinn-With-Two-Ns reviewed Mar 13, 2023
View reviewed changes
internal/internal_workflow_client.go Outdated
@@ -938,6 +938,51 @@ func (wc *WorkflowClient) ResetWorkflowExecution(ctx context.Context, request *w
return resp, nil
}

// UpdateWorkerBuildIDCompatability allows you to update the worker-build-id based version sets for a particular
// task queue. This is used in conjunction with workers who specify their build id and thus opt into the
// feature. For more, see: <doc link>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing docs link

Quinn-With-Two-Ns
Quinn-With-Two-Ns reviewed Mar 13, 2023
View reviewed changes
test/worker_versioning_test.go
ts.NoError(handle21.Get(ctx, nil))
ts.NoError(handle22.Get(ctx, nil))

// TODO: Actually assert they ran on the appropriate workers, once David's changes are ready
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have anything to track this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

temporalio/temporal#3127

Quinn-With-Two-Ns
Quinn-With-Two-Ns approved these changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming the API links are updated once the API changes are merged.

@Sushisource Sushisource force-pushed the versioning-api-refactor branch from 3f3d092 to cee67ec Compare March 13, 2023 19:37
@Sushisource Sushisource force-pushed the versioning-api-refactor branch 3 times, most recently from 220e5f0 to 7b1cabb Compare March 13, 2023 19:47
@Sushisource Sushisource force-pushed the versioning-api-refactor branch from 7b1cabb to 6f092ad Compare March 13, 2023 19:50
@Sushisource Sushisource merged commit 3d009e0 into temporalio:master Mar 13, 2023
@Sushisource Sushisource deleted the versioning-api-refactor branch March 13, 2023 21:22
@cretz cretz mentioned this pull request Sep 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Quinn-With-Two-Ns Quinn-With-Two-Ns Quinn-With-Two-Ns approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Sushisource @Quinn-With-Two-Ns