Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

I just signed the CLA.

/ok-to-test

What created an invalid version which caused this to be a problem and require a patch?

Is this related to #57420 ?

This PR doesn't tweak the previous stanza to pull out the minor and major versions like in #57420. In addition, the regex doesn't match on v1.111.0+me.0.

@eparis The K8s Conformance test will fail if the version string contains two + symbol, such as v1.8.3+identifer1+identifer2.

@rphillips It does match the version string v1.111.0+me.0, see here: https://regexr.com/3ia5j

As I understand, the v1.8.3+identifier is a stable release and v1.8.3-identifier is not, since v1.8.3-identifier will cause the Minor version to 8+.

I know this is not a perfect one, but covers more cases.

@chenzhiwei I tried this regex in bash, and it failed to parse. see an issue with the script?

#!/usr/bin/env bash

if [[ "v1.111.0+me.0" =~ ^v([0-9]+)\.([0-9]+)(\.[0-9]+)?([-][0-9A-Za-z+-.]*|[+][0-9A-Za-z.]+)?$ ]] ; then
  echo "matched"
fi

@rphillips here is my result, I just copied your code:

zhiwei@macbook:~$ cat aa.sh
#!/usr/bin/env bash

if [[ "v1.111.0+me.0" =~ ^v([0-9]+)\.([0-9]+)(\.[0-9]+)?([-][0-9A-Za-z+-.]*|[+][0-9A-Za-z.]+)?$ ]] ; then
  echo "matched"
fi
zhiwei@macbook:~$ bash aa.sh
matched

By the way, seems v1.111.0+me.0 is not a valid Semantic Version string, the identifiers after + should be [0-9A-Za-z-].

I just submitted another patch which covers more cases.

^v([0-9]+)\.([0-9]+)(\.[0-9]+)?([-][0-9A-Za-z.-]+|[-][0-9A-Za-z.-]+[+][0-9A-Za-z.-]+|[+][0-9A-Za-z.-]+)?$

Let me describe the last element in ().

[-][0-9A-Za-z.-]+   ==> v1.8.3-alpha.1-rc0
[-][0-9A-Za-z.-]+[+][0-9A-Za-z.-]+   ==> v1.8.3-alpha.1-rc0+identifiers
[+][0-9A-Za-z.-]+  ==>> v1.8.3+identifiers

As said in https://semver.org/, the identifiers after + sign should be [0-9A-Za-z-], but it also says 1.0.0-beta+exp.sha.5114f85 is a valid version string. So, I made a change to identifiers to support . sign.

the tweak to the regex fixed the parse issue for me. Thanks!

/retest

My question is 'where did the 'illegal' version come from that caused you to write this PR? You say:

The K8s Conformance test will fail if the version string contains two + symbol, such as v1.8.3+identifer1+identifer2.

What is creating something with 2 +'s? It seems to me like the whole point of this PR is to break whatever is doing that, not fix that thing and prevent it from coming back. Show/explain to me how/why this is not a regression.

This is to restrict the KUBE_GIT_VERSION to fully match the Semantic Version rules.

Sometimes, when a user build a Kubernetes binary and he may specify a wrong version such as v1.8.3+id1+id2 by exporting KUBE_GIT_VERSION=v1.8.3+id1+id2.

And Conformance tests will fail when the version string is v1.8.3+id1+id2.

This patch is used to fail early in build steps, not in running the Conformance tests cases. Catch the issue early and fix it early.

Another thing is, if users do not export the KUBE_GIT_VERSION environment variable, the build script will automatically get the version string from git repo. If users on a detached/special branch, the KUBE_GIT_VERSION will be a strange string such as branch-xxx+xxx-dirty.

I want to avoid such cases at the very beginning of the build process, so users can fix it early.

This seems reasonable, but I'm not an OWNER that can approve.
/lgtm

/lgtm
/retest

unit test failure looks like a flake. cross failure is not you.

/test pull-kubernetes-cross

/test pull-kubernetes-e2e-kubeadm-gce-canary

/test pull-kubernetes-cross

tracking issue for crossbuild failure is #59121

/retest

/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chenzhiwei, david-mcmahon, ixdy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

/test pull-kubernetes-e2e-kubeadm-gce-canary

/test all [submit-queue is verifying that this PR is safe to merge]

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@chenzhiwei: The following tests failed, say /retest to rerun them all:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.