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

Use GO_VERSION as single source for the used Go version #10163

Merged
merged 3 commits into from
Feb 13, 2020
Merged

Use GO_VERSION as single source for the used Go version #10163

merged 3 commits into from
Feb 13, 2020

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Feb 12, 2020 •
edited by tgraf

Read the Go version from the GO_VERSION file and use it to replace the hard-coded versions across the Cilium repo, i.e. in the test scripts, Dockerfiles, packaging and documentation.

This allows to bump the Go version in a single place: the GO_VERSION file.

With this PR, updating to a new Go version (1.13.8 in this example) is now as easy as:

echo 1.13.8 > GO_VERSION && make update-golang

Verified by running the command above and checking that git grep 1\.13\.7 doesn't return any matches.

Follow up for #10139, as suggested by @florianl

This change is Reviewable

@tklauser tklauser requested review from a team as code owners February 12, 2020 10:40
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 12, 2020
@tklauser tklauser mentioned this pull request Feb 12, 2020
Closed
@tklauser
Copy link
Member Author

test-docs-please

aanm
aanm reviewed Feb 12, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

awesome improvement, unfortunately we will still miss these:

.travis.yml
cilium-docker-plugin.Dockerfile
cilium-operator.Dockerfile
Dockerfile.builder
contrib/packaging/builder/Dockerfile.runtime

@tklauser
Copy link
Member Author

awesome improvement, unfortunately we will still miss these:

.travis.yml
cilium-docker-plugin.Dockerfile
cilium-operator.Dockerfile
Dockerfile.builder
contrib/packaging/builder/Dockerfile.runtime

As far as I understand the Makefile these should already be covered by the update-golang-dockerfiles and update-travis-go-version targets (which get called by update-golang):

cilium/Makefile

Lines 454 to 461 in b4cd2bc

update-golang-dockerfiles:
$(QUIET) sed -i 's/GO_VERSION .*/GO_VERSION $(GO_VERSION)/g' Dockerfile.builder
$(QUIET) for fl in $(shell find . -name "*Dockerfile*") ; do sed -i 's/golang:.* /golang:$(GO_VERSION) as /g' $$fl ; done
@echo "Updated go version in Dockerfiles to $(GO_VERSION)"
update-travis-go-version:
$(QUIET) sed -e 's/TRAVIS_GO_VERSION/$(GO_VERSION)/g' .travis.yml.tmpl > .travis.yml
@echo "Updated go version in .travis.yml to $(GO_VERSION)"

@coveralls
Copy link

coveralls commented Feb 12, 2020 •
edited

Coverage Status

Coverage increased (+0.008%) to 44.54% when pulling 91dc217 on pr/tklauser/go-version-one-definition into c4b6095 on master.

@tklauser tklauser added dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs release-note/misc This PR makes changes that have no direct user impact. labels Feb 12, 2020
aanm
aanm approved these changes Feb 12, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

wow, nice @tklauser ! thanks

@aanm
Copy link
Member

aanm commented Feb 12, 2020

test-upstream-k8s

@tklauser
Copy link
Member Author

test-me-please

@joestringer joestringer added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 12, 2020
@tgraf tgraf removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 12, 2020
@maintainer-s-little-helper
Copy link

Commit f28ebad8234c0b01d93c878d5948c4bb93ed5b05 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/contributing/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 13, 2020
@tklauser
Documentation: read Go version from GO_VERSION file for development s…
82b809b 
…etup

This avoids having to bump the Go version in the docs manually and also
allows to get rid of the Go version check in Documentation/Makefile.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser
test: define variable for Go version
771aa8a 
This will allow to easily bump the version via a Makefile target
introduced in a successive commit.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser
Makefile: add target to update Go version in test scripts
91dc217 
Read the Go version from the GO_VERSION file and use it to replace the
hard-coded versions in the test scripts.

Together with the preceding commits, this allows to bump the Go version
in a single place: the GO_VERSION file.

Updating to a new Go version (1.13.8 in this example) is now as easy as:

  echo 1.13.8 > GO_VERSION && make update-golang

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser force-pushed the pr/tklauser/go-version-one-definition branch from f28ebad to 91dc217 Compare February 13, 2020 08:09
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 13, 2020
@tklauser
Copy link
Member Author

test-me-please

1 similar comment
@tklauser
Copy link
Member Author

test-me-please

@aanm aanm merged commit c95f93b into master Feb 13, 2020
1.8.0 automation moved this from In progress to Merged Feb 13, 2020
@aanm aanm deleted the pr/tklauser/go-version-one-definition branch February 13, 2020 12:28
@christarazi christarazi mentioned this pull request May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tklauser @coveralls @aanm @joestringer @tgraf @christarazi