-
Notifications
You must be signed in to change notification settings - Fork 5k
Ensure specific golang version everywhere in CI based on Makefile #5095
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
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh 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 |
41ab354
to
f63208b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it updated my golang locally from 1.12.5 to 1.12.8 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version number seems duplicated ?
Probably should be 1.12.9 now, too
@@ -22,6 +22,9 @@ ISO_VERSION ?= v$(VERSION_MAJOR).$(VERSION_MINOR).0 | |||
VERSION ?= v$(VERSION_MAJOR).$(VERSION_MINOR).$(VERSION_BUILD) | |||
DEB_VERSION ?= $(VERSION_MAJOR).$(VERSION_MINOR).$(VERSION_BUILD) | |||
RPM_VERSION ?= $(VERSION_MAJOR).$(VERSION_MINOR).$(VERSION_BUILD) | |||
# used by hack/jenkins/release_build_and_upload.sh and KVM_BUILD_IMAGE, see also BUILD_IMAGE bellow | |||
GO_VERSION ?= 1.12.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this 1.12.9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to make it 1.12.9 but the kubec-cross image is not available in that version yet ! I rather we build everything with same version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes perfect sense, and it is version 1.12.8 that has the needed security fix anyway
I saw the commit updating the version, and naively thought the image was built too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively we can make everything build in host golang and not use any docker at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks wonderful. Thank you for doing this!
function install_golang { | ||
echo "Installing golang version: $1 on $2" | ||
pushd /tmp >/dev/null | ||
sudo curl -qL -O "https://storage.googleapis.com/golang/go${1}.linux-amd64.tar.gz" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be installed to the users home directory instead? It seems awkward to have an automated system upgrading system binaries as a non-system user, only to chown it back to themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, nothing in the home directory can be assumed to be in the PATH everywhere
Might as well use the standard location ? And have it owned by root
, as per documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm....
sudo curl -qL -O "https://storage.googleapis.com/golang/go${1}.linux-amd64.tar.gz" && | ||
sudo tar xfa go${1}.linux-amd64.tar.gz && | ||
sudo rm -rf "${2}/go" && | ||
sudo mv go "${2}/" && sudo chown -R $(whoami): ${2}/go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the chown
step here necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only because doing tar
as root
?
But it also seems wrong, files under /usr/local
should be owned by root (this is not brew)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason I did this is because, the previous version of golang was installed by a user other than "jenkins" . the /usr/local/go was owned by "tstromberg"
so when jenkins was trying to delete that folder and re-install it again, it had permission error.
so I deleted and downloded with sudo and then later changed back to jenkins !
this was the only automated way I could find!
let me know what you guys think ! I personally think this was best way given the previous manual installation situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have provided a comment on why I was doing sudo tar
# install_golang takes two parameters version and path to install. | ||
function install_golang { | ||
echo "Installing golang version: $1 on $2" | ||
pushd /tmp >/dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mktemp -d
is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminds me of this poem http://blog.medyagh.com/2017/07/thats-more-preferable.html
Before this PR run on the jenkins box:
on the first run of this PR if wrong verison of golang is installed, it will warn and then install the right version:
when the golang version is not same as Makefile jenkins will warn and then install the right version
here is the output:
after the first run I can see in the jenkins box ssh:
and also when the right version is already installed it won't re-install but lets you know it is good !