fix sending v prefixed semver version to older etcd peer versions that use the coreos/go-semver - #22261
fix sending v prefixed semver version to older etcd peer versions that use the coreos/go-semver#22261upodroid wants to merge 1 commit into
Conversation
…t use the coreos/go-semver Signed-off-by: upodroid <cy@borg.dev>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: upodroid The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @upodroid. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
|
||
| // VersionForPeer returns the version string to use in the X-Server-Version header | ||
| // when communicating with a peer. Peers older than v3.8 use github.com/coreos/go-semver | ||
| // which does not accept a leading "v" prefix and will panic when parsing it. | ||
| // If the peer version is unknown (nil) or less than v3.8, the "v" prefix is stripped. | ||
| func VersionForPeer(peerVer *semver.Version) string { | ||
| if peerVer == nil || peerVer.LessThan(&V3_8) { | ||
| return strings.TrimPrefix(Version, "v") | ||
| } | ||
| return Version | ||
| } |
There was a problem hiding this comment.
Is this the behavior that we want starting 3.8, though? Or should we always keep the version without the "v" prefix?
I think updating to a new semver library makes sense, but I'm not so sure about changing the API behavior.
cc. @ahrtr, @serathius
There was a problem hiding this comment.
Or should we always keep the version without the "v" prefix?
+1
Here is the plan:
The main justification is that you simplify the release process and eliminate the need to commit versions after cutting tags. You can cut the tag, read the tag value and build it. Also, it will enable testing nightly builds like these: |
I am not against this.
Never do this to avoid unnecessary toil on migration, doc stuff. |
|
This may be a naive question, but what's the issue with always returning the following? return strings.TrimPrefix(Version, "v")So we don't add a breaking change. |
|
I'll explain it more at the community meeting. |
3 participants
In #20915, we are building etcd with Version that is prefixed with v.
Sending, for example,
v2.1.0in the X-Server-Version header will cause semver parsing errors, so the v needs to be stripped. This code can be removed from 3.9 onwards.