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

Update to go1.10.2 #63412

Merged
merged 1 commit into from
May 5, 2018
Merged

Update to go1.10.2 #63412

merged 1 commit into from
May 5, 2018

Conversation

praseodym
Copy link
Contributor

@praseodym praseodym commented May 3, 2018

What this PR does / why we need it:
Update to use go1.10.2 which includes some compiler and linker fixes.

Special notes for your reviewer:
Docker image golang:1.10.2 should be available soon.

Release note:

Update to use go1.10.2

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 3, 2018
Copy link
Member

@cblecker cblecker left a comment

Choose a reason for hiding this comment

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

/cc @ixdy
for image build/push

@@ -321,7 +321,7 @@ EOF
local go_version
IFS=" " read -ra go_version <<< "$(go version)"
local minimum_go_version
minimum_go_version=go1.10.1
minimum_go_version=go1.10.2
Copy link
Member

Choose a reason for hiding this comment

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

This will likely fail because all the test-infra hasn't been upgraded yet. I don't think we need to enforce this as a minimum quiet yet either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, removed.

Copy link
Member

Choose a reason for hiding this comment

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

@BenTheElder can we bump test-infra?

Copy link
Member

Choose a reason for hiding this comment

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

though in general I think the minimum go version here is more important for minor version bumps (e.g. 1.9 -> 1.10) than patch version bumps.

Copy link
Member

Choose a reason for hiding this comment

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

yes, though not much picks up the go versions there, since mostly we either pick up the (in CI doubly so) containerized or bazel builds.

Copy link
Member

Choose a reason for hiding this comment

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

discussed offline, we'll bump all the things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've re-added the bump here!

sha256 = "4b2c61795ac2eefcb28f3eb8e1cb2d8fb3c2eafa0f6712473bc5f93728f38758",
urls = mirror("https://github.com/bazelbuild/rules_go/releases/download/0.10.2/rules_go-0.10.2.tar.gz"),
sha256 = "1868ff68d6079e31b2f09b828b58d62e57ca8e9636edff699247c9108518570b",
urls = mirror("https://github.com/bazelbuild/rules_go/releases/download/0.11.1/rules_go-0.11.1.tar.gz"),
Copy link
Member

Choose a reason for hiding this comment

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

we should bump this instead to 0.10.4 (which still has the necessary go 1.10.2 support); 0.11.x is going to require BUILD updates, which will be more invasive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 0.10.4, let's do the 0.11.x update in another PR.

@BenTheElder
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 3, 2018
@@ -15,7 +15,7 @@
# This file creates a standard build environment for building cross
# platform go binary for the architecture kubernetes cares about.

FROM golang:1.10.1
FROM golang:1.10.2
Copy link
Member

Choose a reason for hiding this comment

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

this still doesn't appear to exist yet.

Copy link
Member

@BenTheElder BenTheElder May 3, 2018

Choose a reason for hiding this comment

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

ah, you have that in the PR body :-), hit this trying to prepare the related test-infra PR per @ixdy's request 🤷‍♂️

@praseodym
Copy link
Contributor Author

/retest

@cblecker
Copy link
Member

cblecker commented May 4, 2018

Tests aren’t going to pass until images are built and pushed.

@praseodym
Copy link
Contributor Author

praseodym commented May 4, 2018

The golang:1.10.2 Docker image was just pushed 🙂 edit: but test infra isn’t, so that part still fails

@ixdy
Copy link
Member

ixdy commented May 4, 2018

kube-cross image has been built; I'm working on getting this pushed to k8s.gcr.io now.

@ixdy
Copy link
Member

ixdy commented May 4, 2018

/test pull-kubernetes-cross

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.

/approve

I think the test-infra images still need to be bumped. As soon as they're up in this lgtm.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2018
@ixdy
Copy link
Member

ixdy commented May 4, 2018

/assign @BenTheElder

@BenTheElder
Copy link
Member

test-infra bump is ready in kubernetes/test-infra#7934

@BenTheElder
Copy link
Member

test-infra bump went in
/retest

@dims
Copy link
Member

dims commented May 5, 2018

/lgtm

(cross my fingers)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, ixdy, praseodym

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-github-robot
Copy link

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

@k8s-github-robot
Copy link

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

@k8s-github-robot k8s-github-robot merged commit 678152b into kubernetes:master May 5, 2018
@praseodym praseodym deleted the go1.10.2 branch May 5, 2018 02:00
@ingvagabund
Copy link
Contributor

@ixdy, @BenTheElder, @dims can you CC me the next the minimal required Go version is bumped? So I can get ready and update the CI job for the node e2e tests over containerized kubelet in time. :) Thanks

@dims
Copy link
Member

dims commented May 7, 2018

ack @ingvagabund will try to remember :)

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants