Update Deployment Workflow ID's and accept string version values #7198
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ss/update-deployment-wf-ids
## What changed? Made deployment workflow ids based on the version id (deploymeny name / build id). Standardized internal variable naming so that `version string` always refers to the fully-qualified version identifier, whereas `build_id string` always refers to build id. ## Why? <!-- Tell your future self why have you made these changes --> ## 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) -->
…emporal into ss/update-deployment-wf-ids
carlydf
approved these changes
Feb 2, 2025
Shivs11
commented
Feb 2, 2025
| @@ -246,6 +246,7 @@ message SetWorkerDeploymentRampingVersionArgs { | |||
| // used as Worker Deployment activity input: | |||
| message SyncVersionStateActivityArgs { | |||
| string deployment_name = 1; | |||
| // <deployment_name>/<build_id> or possibly just <version_id> in the future | |||
There was a problem hiding this comment.
I think once we are inside the entity workflow realm, we should probably not be doing the business of string concatenation/separation and pass them as separate values right?
| if s == "__unversioned__" { | ||
| return nil, nil | ||
| } | ||
| before, after, found := strings.Cut(s, WorkerDeploymentVersionIdDelimiter) |
There was a problem hiding this comment.
nit: should rename this to deploymentName, buildID, found
| @@ -344,7 +344,8 @@ func (c *physicalTaskQueueManagerImpl) PollTask( | |||
| } | |||
|
|
|||
| // [cleanup-wv-pre-release] | |||
| if c.partitionMgr.engine.config.EnableDeployments(namespaceEntry.Name().String()) { | |||
| if c.partitionMgr.engine.config.EnableDeployments(namespaceEntry.Name().String()) && | |||
| pollMetadata.deploymentOptions.GetWorkflowVersioningMode() == enumspb.WORKFLOW_VERSIONING_MODE_UNSPECIFIED { | |||
Comment on lines
+358
to
+360
| if versionObj.GetDeploymentName() != deploymentName { | ||
| return nil, serviceerror.NewInvalidArgument(fmt.Sprintf("invalid version string '%s' does not match deployment name '%s'", version, deploymentName)) | ||
| } |
There was a problem hiding this comment.
there's a chance, right now atleast, that versionObj could be nil (in the event that we deal with unversioned workers)
Either we change the code in physical_task_queue_manager to only deal with VERSIONING_BEHAVIORS workers and then this won't be a problem or safeguard against this
wdyt?
Comment on lines
+212
to
+213
| deploymentName := workerDeploymentVersion.GetDeploymentName() | ||
| buildID := workerDeploymentVersion.GetBuildId() |
There was a problem hiding this comment.
we may need to have nil checks here (right now) just to safeguard but I don't think we do in the future since "unversioned" would just be present in a new field (id) inside workerDeploymentVersion - might be conservative programming rn but better to be safe than sorry
Comment on lines
+419
to
+421
| versionObj = &deploymentpb.WorkerDeploymentVersion{ | ||
| DeploymentName: deploymentName, | ||
| BuildId: "", |
There was a problem hiding this comment.
nit: took me a min to realize that this was unsetting the set ramp - maybe a comment would be nice
| deploymentName, version string, | ||
| deploymentName, buildId string, |
There was a problem hiding this comment.
if you notice, this is the only method which accepts a build_id rather than other methods now accepting version which will be the concatenation of deploymentName/build_id - is there any way we can make a change here for consistency purposes?
More than consistency, it's so hard to follow now what to pass in where and almost always think if you are passing in the wrong version/build_id value - Maybe keeping things consistent could help us here?
ShahabT
pushed a commit
that referenced
this pull request
Feb 4, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
pushed a commit
that referenced
this pull request
Feb 4, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
pushed a commit
that referenced
this pull request
Feb 4, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
pushed a commit
that referenced
this pull request
Feb 5, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
pushed a commit
that referenced
this pull request
Feb 5, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
pushed a commit
that referenced
this pull request
Feb 6, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
pushed a commit
that referenced
this pull request
Feb 6, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
pushed a commit
that referenced
this pull request
Feb 6, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
pushed a commit
that referenced
this pull request
Feb 6, 2025
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?