-
Notifications
You must be signed in to change notification settings - Fork 77
change Version to struct, prevent misconfiguration of VersioningOverride #579
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
Conversation
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! Much safer. Having all these deprecated fields is slightly sad, but, that's life. Let's make sure we track actually removing them after things are GA
None of my comments are blocking.
Co-authored-by: Spencer Judge <[email protected]>
Co-authored-by: Spencer Judge <[email protected]>
Co-authored-by: Spencer Judge <[email protected]>
Co-authored-by: Spencer Judge <[email protected]>
…versioning-override
2ebd0b3
to
9ab5def
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.
Great, happy with this and 10x more obvious than the previous iteration
Co-authored-by: Shahab Tajik <[email protected]>
Co-authored-by: Shahab Tajik <[email protected]>
…ersion and deployment options
**What changed?** - Fix a double semicolon introduced in #579. **Why?** - The double semicolon sequence is causing a compilation error in `protobufjs`, even though this is technically valid according to [the official Protobuf 3 spec](https://protobuf.dev/reference/protobuf/proto3-spec/#emptystatement). This is a bug in `protobufjs`, for which there's been [an open issue ticket](protobufjs/protobuf.js#1322) since 2019, but it was never acknowledged, and a PR fixing this has been ignored for more than a year. Given that there's simply no reason to use a double semicolon sequence anyway, it appears preferable to simply avoid that in our proto definitions.
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?
Why?
RoutingConfig
you are reading.Variables are being deprecated, but the content of the code is not changing.
The idea of "Unset Ramp" having a separate meaning from "0% Ramp" is removed. 0% ramp now is the only way to have "no ramp" / 100% of traffic going to Current Version. Nil
ramping_deployment_version
+ non-zeroramping_version_percentage
is the way to ramp traffic to unversioned workers.The SDK interface for this could look like:
Server PR