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

Use Deployment Version strings in all user-facing APIs #7219

Merged
merged 4 commits into from
Feb 3, 2025
Merged

Use Deployment Version strings in all user-facing APIs #7219

merged 4 commits into from
Feb 3, 2025

Conversation

ShahabT
Copy link
Collaborator

@ShahabT ShahabT commented Feb 3, 2025

What changed?

Updated code to use latest API changes in which Deployment Versions are represented by string fields rather than structs.
WorkerDeploymentVersion proto message still exists but only for the internal APIs.

Why?

See temporalio/api#547.

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

@ShahabT ShahabT requested review from carlydf and Shivs11 February 3, 2025 09:04
@ShahabT ShahabT requested a review from a team as a code owner February 3, 2025 09:04
carlydf
carlydf reviewed Feb 3, 2025
View reviewed changes
common/worker_versioning/worker_versioning.go Outdated Show resolved Hide resolved
common/worker_versioning/worker_versioning.go Show resolved Hide resolved
common/worker_versioning/worker_versioning.go Outdated Show resolved Hide resolved
service/history/api/updateworkflowoptions/api_test.go
@@ -45,13 +44,13 @@ var (
pinnedOverrideOptionsA = &workflowpb.WorkflowExecutionOptions{
VersioningOverride: &workflowpb.VersioningOverride{
Behavior: enumspb.VERSIONING_BEHAVIOR_PINNED,
PinnedVersion: &deploymentpb.WorkerDeploymentVersion{DeploymentName: "X", BuildId: "A"},
PinnedVersion: "X/A",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if you call this slash worker_versioning.WorkerDeploymentVersionIdDelimiter then we won't have to change the test later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, but there is something good about this: if someone changes the delimiter unexpectedly, this test will catch it. Will leave it as is until the delimiter discussion resolves.

service/history/workflow/mutable_state_impl.go Outdated Show resolved Hide resolved
service/matching/deployment_util.go Show resolved Hide resolved
service/matching/deployment_util.go Show resolved Hide resolved
service/matching/deployment_util.go Show resolved Hide resolved
@ShahabT @carlydf
Apply suggestions from code review
c20d13a 
Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
@ShahabT ShahabT merged commit 63c99b9 into versioning-3.1 Feb 3, 2025
8 of 10 checks passed
@ShahabT ShahabT deleted the shahab/v31-fixes branch February 3, 2025 23:53
carlydf
carlydf reviewed Feb 4, 2025
View reviewed changes
Copy link
Contributor

@carlydf carlydf left a comment

Choose a reason for hiding this comment

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

approved except for one comment I'd love to sync up about since I'm confused about how we're representing unversioned ramp target

service/matching/task_queue_partition_manager.go Show resolved Hide resolved
ShahabT added a commit that referenced this pull request Feb 4, 2025
@ShahabT @carlydf
Use Deployment Version strings in all user-facing APIs (#7219)
aed6b6b 
## What changed?
<!-- Describe what has changed in this PR -->
Updated code to use latest API changes in which Deployment Versions are
represented by string fields rather than structs.
`WorkerDeploymentVersion` proto message still exists but only for the
internal APIs.

## Why?
<!-- Tell your future self why have you made these changes -->
See temporalio/api#547.

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->

---------

Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
ShahabT added a commit that referenced this pull request Feb 4, 2025
@ShahabT @carlydf
Use Deployment Version strings in all user-facing APIs (#7219)
5df8994 
## What changed?
<!-- Describe what has changed in this PR -->
Updated code to use latest API changes in which Deployment Versions are
represented by string fields rather than structs.
`WorkerDeploymentVersion` proto message still exists but only for the
internal APIs.

## Why?
<!-- Tell your future self why have you made these changes -->
See temporalio/api#547.

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->

---------

Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
ShahabT added a commit that referenced this pull request Feb 4, 2025
@ShahabT @carlydf
Use Deployment Version strings in all user-facing APIs (#7219)
881180f 
## What changed?
<!-- Describe what has changed in this PR -->
Updated code to use latest API changes in which Deployment Versions are
represented by string fields rather than structs.
`WorkerDeploymentVersion` proto message still exists but only for the
internal APIs.

## Why?
<!-- Tell your future self why have you made these changes -->
See temporalio/api#547.

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->

---------

Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
ShahabT added a commit that referenced this pull request Feb 5, 2025
@ShahabT @carlydf
Use Deployment Version strings in all user-facing APIs (#7219)
6c43c32 
## What changed?
<!-- Describe what has changed in this PR -->
Updated code to use latest API changes in which Deployment Versions are
represented by string fields rather than structs.
`WorkerDeploymentVersion` proto message still exists but only for the
internal APIs.

## Why?
<!-- Tell your future self why have you made these changes -->
See temporalio/api#547.

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->

---------

Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
ShahabT added a commit that referenced this pull request Feb 5, 2025
@ShahabT @carlydf
Use Deployment Version strings in all user-facing APIs (#7219)
1f555d5 
## What changed?
<!-- Describe what has changed in this PR -->
Updated code to use latest API changes in which Deployment Versions are
represented by string fields rather than structs.
`WorkerDeploymentVersion` proto message still exists but only for the
internal APIs.

## Why?
<!-- Tell your future self why have you made these changes -->
See temporalio/api#547.

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->

---------

Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlydf carlydf carlydf left review comments

@Shivs11 Shivs11 Awaiting requested review from Shivs11

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

Successfully merging this pull request may close these issues.
3 participants
@ShahabT @carlydf @Shivs11