Skip to content

VersionStatus for WorkerDeploymentVersions#595

Merged
Shivs11 merged 7 commits into
masterfrom
ss/version_status
May 28, 2025
Merged

VersionStatus for WorkerDeploymentVersions#595
Shivs11 merged 7 commits into
masterfrom
ss/version_status

Conversation

@Shivs11
Copy link
Copy Markdown
Member

@Shivs11 Shivs11 commented May 24, 2025

READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.

What changed?

  • add VersionStatus inside WorkerDeploymentVersionInfo + WorkerDeploymentVersionSummary

Why?

  • right now, the UI code deduces the status of a version by checking CurrentSince, RampingSince and other attributes. This can just be simplified by adding this enum and the server doing the heavy lifting instead.

Breaking changes

  • None

Server PR

@Shivs11 Shivs11 marked this pull request as ready for review May 24, 2025 21:33
@Shivs11 Shivs11 requested review from a team as code owners May 24, 2025 21:33
Copy link
Copy Markdown
Contributor

@ShahabT ShahabT left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

Comment thread temporal/api/enums/v1/deployment.proto Outdated
Comment thread temporal/api/enums/v1/deployment.proto Outdated
Copy link
Copy Markdown
Contributor

@cretz cretz left a comment

Choose a reason for hiding this comment

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

(deferring to @Sushisource who has more background here than I do, nothing syntactically wrong from my POV)

Copy link
Copy Markdown
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

LGTM including comment on other PR about docstrings in the list msg

@Shivs11 Shivs11 merged commit e11b55a into master May 28, 2025
7 checks passed
@Shivs11 Shivs11 deleted the ss/version_status branch May 28, 2025 16:36
stephanos pushed a commit that referenced this pull request Mar 10, 2026
_**READ BEFORE MERGING:** All PRs require approval by both Server AND
SDK teams before merging! This is why the number of required approvals
is "2" and not "1"--two reviewers from the same team is NOT sufficient.
If your PR is not approved by someone in BOTH teams, it may be summarily
reverted._

<!-- Describe what has changed in this PR -->
**What changed?**
- add VersionStatus inside `WorkerDeploymentVersionInfo` +
`WorkerDeploymentVersionSummary`

<!-- Tell your future self why have you made these changes -->
**Why?**
- right now, the UI code deduces the status of a version by checking
`CurrentSince`, `RampingSince` and other attributes. This can just be
simplified by adding this enum and the server doing the heavy lifting
instead.

<!-- Are there any breaking changes on binary or code level? -->
**Breaking changes**
- None

<!-- If this breaks the Server, please provide the Server PR to merge
right after this PR was merged. -->
**Server PR**
- [741011](temporalio/temporal#7804)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants