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.

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

Fix Version Output For Automated Container Builds #427

Merged

Conversation

seanmalloy
Copy link
Member

@seanmalloy seanmalloy commented Oct 13, 2020

Prior to this change the output from the command "descheduler version" when run using the official container images from k8s.gcr.io would always output an empty string. See below for an example.

docker run k8s.gcr.io/descheduler/descheduler:v0.19.0 /bin/descheduler version
Descheduler version {Major: Minor: GitCommit: GitVersion: BuildDate:2020-09-01T16:43:23+0000 GoVersion:go1.15 Compiler:gc Platform:linux/amd64}

This change makes it possible to pass the descheduler version information to the automated container image build process and also makes it work for local builds too.

Fixes #425

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 13, 2020
@seanmalloy
Copy link
Member Author

Not quite done testing yet.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 14, 2020
@seanmalloy seanmalloy changed the title WIP: fix version output Fix Version Output For Automated Container Builds Oct 14, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2020
@seanmalloy
Copy link
Member Author

Here is what the new output for descheduler version looks like after this change.

Example for local builds and automated builds using a commit(not a tag):

$ make build
CGO_ENABLED=0 go build -ldflags "-X sigs.k8s.io/descheduler/cmd/descheduler/app.version=v20201013-v0.18.0-181-g7ae3f9ea2 -X sigs.k8s.io/descheduler/cmd/descheduler/app.buildDate=2020-10-13T22:28:24-0500" -o _output/bin/descheduler sigs.k8s.io/descheduler/cmd/descheduler
$ ./_output/bin/descheduler version
Descheduler version {Major:0 Minor:18+ GitVersion:v20201013-v0.18.0-181-g7ae3f9ea2 BuildDate:2020-10-13T22:28:24-0500 GoVersion:go1.15.2 Compiler:gc Platform:darwin/amd64}

Example for automated builds using a tag:

$ VERSION=v20200521-v0.18.0 make build
CGO_ENABLED=0 go build -ldflags "-X sigs.k8s.io/descheduler/cmd/descheduler/app.version=v20200521-v0.18.0 -X sigs.k8s.io/descheduler/cmd/descheduler/app.buildDate=2020-10-13T22:35:46-0500" -o _output/bin/descheduler sigs.k8s.io/descheduler/cmd/descheduler
$ ./_output/bin/descheduler version
Descheduler version {Major:0 Minor:18+ GitVersion:v20200521-v0.18.0 BuildDate:2020-10-13T22:35:46-0500 GoVersion:go1.15.2 Compiler:gc Platform:darwin/amd64}

Example for automated builds using the helm chart tag. Yes, this is still broken:

$ VERSION=v20201012-descheduler-helm-chart-0.19.0-39-gb33928ac9 make build
CGO_ENABLED=0 go build -ldflags "-X sigs.k8s.io/descheduler/cmd/descheduler/app.version=v20201012-descheduler-helm-chart-0.19.0-39-gb33928ac9 -X sigs.k8s.io/descheduler/cmd/descheduler/app.buildDate=2020-10-13T22:37:09-0500" -o _output/bin/descheduler sigs.k8s.io/descheduler/cmd/descheduler
$ ./_output/bin/descheduler version
Descheduler version {Major: Minor: GitVersion:v20201012-descheduler-helm-chart-0.19.0-39-gb33928ac9 BuildDate:2020-10-13T22:37:09-0500 GoVersion:go1.15.2 Compiler:gc Platform:darwin/amd64}

@seanmalloy
Copy link
Member Author

/test pull-descheduler-verify-master

@seanmalloy
Copy link
Member Author

/assign @damemi
/cc @ingvagabund @lixiang233

@seanmalloy
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2020
@seanmalloy
Copy link
Member Author

Also, tested the make image target.

$ make image
docker run descheduler:v20201014-v0.18.0-181-gcf020224a /bin/descheduler version
Descheduler version {Major:0 Minor:18+ GitVersion:v20201014-v0.18.0-181-gcf020224a BuildDate:2020-10-14T06:06:27+0000 GoVersion:go1.15.2 Compiler:gc Platform:linux/amd64}

and ...

$ VERSION=v20200521-v0.18.0 make image
$ docker run descheduler:v20200521-v0.18.0 /bin/descheduler version
Descheduler version {Major:0 Minor:18+ GitVersion:v20200521-v0.18.0 BuildDate:2020-10-14T06:11:35+0000 GoVersion:go1.15.2 Compiler:gc Platform:linux/amd64}

Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

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

@seanmalloy looks like you've got it working, but as you mentioned in #427 (comment) this is obviously still going to print the wrong tag for automatic builds. So 2 questions: that shouldn't be a problem with images pulled from the published v0.19.0 tags right? And also, would there be a way to parse the semver out of the helm-chart tag and use that?

@seanmalloy
Copy link
Member Author

@damemi see responses to your questions below.

that shouldn't be a problem with images pulled from the published v0.19.0 tags right?

Short answer, correct.

I believe once this PR is merged the descheduler version command will print the correct version information for the container images that are built for releases. I think the only gotcha when creating a release is that you must create and push the git tag prior to creating the release in the GitHub UI. Maybe someday there will be a shell script to automate the few manual release steps.

And also, would there be a way to parse the semver out of the helm-chart tag and use that?

My concern would be that we would be putting in helm chart version into the descheduler binary instead of the descheduler version. Let me know if you disagree.

I don't have a great solution for this right now.

@damemi
Copy link
Contributor

damemi commented Nov 20, 2020

@seanmalloy cool, I actually think the recent updates we made to the chart releaser action might fix our automatic releases (the new version of the action tags from the branch it's created from, which will keep it separate from master see #436 (review)).

I think the only gotcha when creating a release is that you must create and push the git tag prior to creating the release in the GitHub UI.

Also btw, the tags can be created from the github UI when you create the release (this is how I've done the past couple releases 😉)

Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

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

@seanmalloy this looks good to me if you just squash the commits
/approve
/hold
@ingvagabund can lgtm (and remove the hold)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, seanmalloy

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2020
@seanmalloy
Copy link
Member Author

I'll squash commits after @ingvagabund reviews assuming there are not additional changes requested.

@ingvagabund
Copy link
Contributor

ingvagabund commented Nov 20, 2020

Looks good, you can squash. I will lgtm right after. Thanks!!!

Prior to this change the output from the command "descheduler version"
when run using the official container images from k8s.gcr.io would
always output an empty string. See below for an example.

```
docker run k8s.gcr.io/descheduler/descheduler:v0.19.0 /bin/descheduler version
Descheduler version {Major: Minor: GitCommit: GitVersion: BuildDate:2020-09-01T16:43:23+0000 GoVersion:go1.15 Compiler:gc Platform:linux/amd64}
```

This change makes it possible to pass the descheduler version
information to the automated container image build process and
also makes it work for local builds too.
@seanmalloy
Copy link
Member Author

@ingvagabund commits have been squashed.

@ingvagabund
Copy link
Contributor

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit c132371 into kubernetes-sigs:master Nov 21, 2020
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
…on-output

Fix Version Output For Automated Container Builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Descheduler Version Not Working in Official Container Images
4 participants