-
Notifications
You must be signed in to change notification settings - Fork 145
Introduce ExecuteWithVersion and ExecuteWithMinVersion to GetVersion #1427
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.
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
Introduce ExecuteWithVersion and ExecuteWithMinVersion to GetVersion #1427
Conversation
Codecov ReportAttention: Patch coverage is
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
3vilhamster
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, and the implementation is rather small.
But I remember that we wanted to introduce x/ folder for experiments, and this API could be experimental for now until we are fully committed on the final design and merge it to the main.
|
|
||
| // UseMinVersion is used to force GetVersion to return minSupported version | ||
| // instead of maxSupported version. Set up via ExecuteWithMinVersion option. | ||
| UseMinVersion bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, it will allow avoiding very wordy:
GetVersion(ctx, workflow.DefaultVerion, 1, workflow.WithVersion(workflow.DefaultVerion))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small stuff, some not-so-small stuff, and I think I need to spend some more time to think through the implementation's details... but I think it's on the right path, and it's a low-level semantic that users can't really build on their own so it seems worth having. though it is kinda fiddly (but I think that's unavoidable).
And, generally speaking, docs need a fair bit of work I think. They refer to things that don't exist, leave gaps in some places, and tbh I kinda doubt that anyone new to versioning will know what to do from just reading the docs :\ I do think we need to get that into a relatively good state before merging, since users will roughly immediately be exposed to it.
this is kinda making me wonder if we should make an entirely new API for this, to just switch to functional options everywhere. like maybe
v := GetVersionV2(
ctx, "name", // unchanged
3, // desired version (equal to ExecuteWithVersion)
workflow.MaxVersion(4), // default..4 range
workflow.VersionRange(-1, 4), // or an explicit min/max
// if omitted, no range allowed, same as `GetVersion(ctx, 3, 3)`
)since I think we can't avoid an explicit "desired" version.
but we can also do that later. adding varargs like you've got here is probably the least-effort way to add features "in place" with no real migration needed - technically a breaking change, but not in a way most code will notice.
568b897 to
1f2e2ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think the behavior's correct, I was over-thinking it earlier. Let's do this 👍
I think comments summarize as:
- some minor cleanups
- can't expose
internal.GetVersionOptionslike that, probably just make an empty interface with a private method and alias it.- technically you can also cast to a same-shaped struct and re-implement or something. which is what should have been done for ~all of the things in these public packages. but that ship has sailed, more aliases is fine IMO since we can't remove the existing ones.
- like one more test-suite test, to show that it behaves like the integration tests
otherwise looks good :)
…rkflowEnvironmentImpl
Co-authored-by: Steven L <[email protected]>
055ac3d to
b5257fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go, thank you for the idea and all the effort! It's a good low-level-primitive to have, this was definitely a gap in GetVersion.
we'll need to add some details to the changelog / release eventually, but that'll be another PR.
| // customVersionOption forces GetVersion to return a specific version | ||
| type customVersionOption Version | ||
|
|
||
| func (c customVersionOption) apply(config *getVersionConfig) { | ||
| version := Version(c) | ||
| config.CustomVersion = &version | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea. verbose and kinda annoying, but I really don't think we have much of an alternative ¯\_(ツ)_/¯
| // GetVersion returns maxSupported version when is executed for the first time. This version is recorded into the | ||
| // workflow history as a marker event. Even if maxSupported version is changed the version that was recorded is | ||
| // returned on replay. DefaultVersion constant contains version of code that wasn't versioned before. | ||
| // Check documentation for ExecuteWithVersion and ExecuteWithMinVersion to make your changes forward and backward compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, good enough for now anyway :) we can consider a bigger rewrite some other time.
What changed?
GetVersionOptioninterface and related optionsExecuteWithVersionoption to use specific version instead of max supported versionExecuteWithMinVersionoption to use specific version instead of min supported versionGetVersionfunction to supportGetVersionOptionsoptional parametersDefaultVersionGetVersionoftestWorkflowEnvironmentImplgot support of new optionsExecuteWithVersionandExecuteWithMinVersionWhy?
This change improves the workflow versioning system by providing more granular control over version execution. The new options allow for safer rollouts of workflow changes by separating code deployment from feature activation
Testing framework must emulate Cadence worker to enable customer writing unit tests. Without the change, customers will not be able to start using new functions in their unit tests.
How did you test it?
ExecuteWithVersionfunction to reproduce a safe rollout.ExecuteWithVersionfunction to reproduce a safe rollout.Potential risks
GetVersionwithout options will continue to work as beforeDetailed Description
This PR introduces a new version control system for workflows by adding
GetVersionOptioninterface and two new options (ExecuteWithVersionandExecuteWithMinVersion) to the existingGetVersionfunction, allowing for more granular control over workflow version execution and safer deployment strategies. The changes maintain backward compatibility while enabling a three-step deployment process that separates code deployment from feature activation, ensuring safe rollback capabilities and better version management.Interface Changes
1. WorkflowInterceptor, WorkflowEnvironment, TestWorkflowEnvironment Interfaces
opts ...GetVersionOptionto support version control options2. New Types
GetVersionOption Type
3. New Option Functions
ExecuteWithVersion
ExecuteWithMinVersion
4. Version Selection Logic Changes
New Implementation
Impact Analysis
Testing Plan
GetVersionOptionthat makes the changes compatible with the previous signature. At the same time,testify.Mocksupports acceptingGetVersionwithout a variadic argument, so there is no need to change the mock.Rollout Plan