Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make push-build.sh version regex'ing CI-friendly. #192

Merged
merged 1 commit into from
Nov 5, 2016
Merged

Make push-build.sh version regex'ing CI-friendly. #192

merged 1 commit into from
Nov 5, 2016

Conversation

david-mcmahon
Copy link
Contributor

@david-mcmahon david-mcmahon commented Nov 3, 2016 •
edited
Loading

closes #191
cc @kubernetes/test-infra-admins

ixdy
ixdy reviewed Nov 3, 2016
View reviewed changes
lib/gitlib.sh
@@ -40,6 +40,7 @@ BRANCH_REGEX="master|release-([0-9]{1,})\.([0-9]{1,})(\.([0-9]{1,}))*$"
declare -A VER_REGEX=([release]="v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(-alpha|-beta)*\.*(0|[1-9][0-9]*)?"
[dotzero]="v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.0$"
[build]="([0-9]{1,})\+([0-9a-f]{5,40})"
[cibuild]="([0-9]{1,})\+*([0-9a-f]{5,40})*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should probably be ?s instead of *s? probably not a huge deal though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I suspect this entire thing should be optional. right now git describe --tags prints out v1.3.11-beta.0 and v1.4.6-beta.0, where the 0 comes from the tag. git doesn't add a 0 if there are no additional commits past the tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need some separation here. VER_REGEX[build] is used in places throughout that do require a releasable version string. And [cibuild] is not a releasable version string.

Copy link
Member

@ixdy ixdy Nov 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying we should change VER_REGEX[build]. what I'm saying is that the bug is when the git branch was just tagged with a release string, it will only match VER_REGEX[release] with no additional suffix.

For example,

$ git checkout v1.4.5
HEAD is now at 5a0a696... Kubernetes version v1.4.5
$ ( source hack/lib/version.sh && KUBE_ROOT=$(pwd) kube::version::get_version_vars && kube::version::save_version_vars /dev/stdout )
KUBE_GIT_COMMIT='5a0a696437ad35c133c0c8493f7e9d22b0f9b81b'
KUBE_GIT_TREE_STATE='clean'
KUBE_GIT_VERSION='v1.4.5'
KUBE_GIT_MAJOR='1'
KUBE_GIT_MINOR='4'

I'm pretty sure v1.4.5 will not match ${VER_REGEX[release]}\.${VER_REGEX[cibuild]}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Understood. PTAL.

ixdy
ixdy reviewed Nov 3, 2016
View reviewed changes
push-build.sh
@@ -98,7 +98,7 @@ RELEASE_BUCKET_MIRROR=$FLAGS_bucket_mirror
[[ $KUBE_GCS_UPDATE_LATEST == "n" ]] && FLAGS_noupdatelatest=1

KUBECTL_OUTPUT=$(cluster/kubectl.sh version --client 2>&1 || true)
if [[ "$KUBECTL_OUTPUT" =~ GitVersion:\"(${VER_REGEX[release]}\.${VER_REGEX[build]})\", ]]; then
if [[ "$KUBECTL_OUTPUT" =~ GitVersion:\"(${VER_REGEX[release]}\.${VER_REGEX[cibuild]})?\", ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh, this now makes the entire thing optional. I don't think that's what we want?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a separate [cibuild] - just change \.${VER_REGEX[build]} to (\.${VER_REGEX[build]})?.

ixdy
ixdy reviewed Nov 3, 2016
View reviewed changes
push-build.sh
@@ -98,7 +98,7 @@ RELEASE_BUCKET_MIRROR=$FLAGS_bucket_mirror
[[ $KUBE_GCS_UPDATE_LATEST == "n" ]] && FLAGS_noupdatelatest=1

KUBECTL_OUTPUT=$(cluster/kubectl.sh version --client 2>&1 || true)
if [[ "$KUBECTL_OUTPUT" =~ GitVersion:\"(${VER_REGEX[release]}\.${VER_REGEX[build]})\", ]]; then
if [[ "$KUBECTL_OUTPUT" =~ GitVersion:\"((${VER_REGEX[release]}(\.${VER_REGEX[cibuild]})?)\", ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now this has 3 (s and 2 )s?

I still don't understand why we need a separate [cibuild] - this will now match something like v1.4.6-beta.0.1 which is not valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, WIP. Will PTAL when the time is right.

WRT to [cibuild], anago checks for an incoming version to build a release from. This release should be a full CI version string with a +sha.

@david-mcmahon david-mcmahon changed the title Make push-build.sh version regex'ing CI-friendly. WIP: Make push-build.sh version regex'ing CI-friendly. Nov 3, 2016
@david-mcmahon david-mcmahon changed the title WIP: Make push-build.sh version regex'ing CI-friendly. Make push-build.sh version regex'ing CI-friendly. Nov 3, 2016
@david-mcmahon
Copy link
Contributor Author

PTAL

@david-mcmahon
Copy link
Contributor Author

ping. Just want to make sure this isn't blocking.

ixdy
ixdy reviewed Nov 4, 2016
View reviewed changes
Copy link
Member

@ixdy ixdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay tests!

lib/gitlib_test.sh
DOTZERO="v1.4.0"
SHORT="v1.4.5"
LONG="v1.4.5-alpha.0"
BUILD="v1.4.5-alpha.0.435"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this actually a version we'll ever see? I think if you are getting the build number, git describe (which is what hack/lib/version.sh uses) will also always append the sha.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the move into CI, I'm not sure what versions I'll see, but will take your guidance here. If you don't encounter $BUILD in CI, we can remove.

ixdy
ixdy reviewed Nov 5, 2016
View reviewed changes
Copy link
Member

@ixdy ixdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG with one fix. (maybe verify that tests pass? :)

lib/gitlib_test.sh
fi

printf "%-40s" "$SHA : "
if [[ $SHA =~ ${VER_REGEX[release]}\.${VER_REGEX[cibuild]} ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/ci// :)

@david-mcmahon
Copy link
Contributor Author

PTAL

@david-mcmahon david-mcmahon merged commit 6def804 into kubernetes:master Nov 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ixdy ixdy ixdy approved these changes

Assignees

@ixdy ixdy

Labels
priority/P1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

handle release tags as versions correctly
3 participants
@david-mcmahon @ixdy @googlebot