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 Go 1.14.4 #328

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Conversation

seanmalloy
Copy link
Member

The kubernetes project has been updated to use Go 1.14.4. See below pull
request.

kubernetes/kubernetes#88638

After making the updates to Go 1.14 "make gen" no longer worked. The
file hack/tools.go had to be created to get "make gen" working with Go
1.14.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 23, 2020
// This package imports things required by build scripts, to force `go mod` to see them as dependencies
package tools

import _ "k8s.io/code-generator"
Copy link
Member Author

Choose a reason for hiding this comment

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

Without adding this file make gen fails with the below error ...

make gen
ls: cannot access '_output/bin/golangci-lint': No such file or directory
./hack/update-generated-conversions.sh
+ go build -o /Users/myuser/projects/go/src/sigs.k8s.io/descheduler/_output/bin/conversion-gen k8s.io/code-generator/cmd/conversion-gen
can't load package: package k8s.io/code-generator/cmd/conversion-gen: cannot find package "." in:
	/Users/myuser/projects/go/src/sigs.k8s.io/descheduler/vendor/k8s.io/code-generator/cmd/conversion-gen
make: *** [gen] Error 1

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this, like we talked about before this also lets us pin a code generator version so we don't get the random go.mod updates

@seanmalloy
Copy link
Member Author

/hold

Two things ...

The Prow jobs need to be updated to use Go 1.14, so that make verify-vendor will pass. Also, let's hold on this until #327 is merged and I will rebase this PR.

@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 Jun 23, 2020
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.

opened a PR to test-infra trying to update to 1.14 here: kubernetes/test-infra#18054

The kubernetes project has been updated to use Go 1.14.4. See below pull
request.

kubernetes/kubernetes#88638

After making the updates to Go 1.14 "make gen" no longer worked. The
file hack/tools.go had to be created to get "make gen" working with Go
1.14.
@seanmalloy
Copy link
Member Author

seanmalloy commented Jun 24, 2020

/hold cancel
/assign @ingvagabund @damemi

I'll back port the Go 1.14 update to the release-1.17 and release-1.18 branches after this PR is merged other wise the Prow CI tests will fail for those branches. Also, Go 1.13 will be unsupported once Go 1.15 is released.

@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 Jun 24, 2020
@ingvagabund
Copy link
Contributor

Preferable I'd rather wait until k8s v1.19 is cut and vendored. Though, given Prow jobs have been bumped to 1.14 and I think go1.14 is backward compatible with go1.13, I don't see a big deal of postponing the PR to get merged.

I'll back port the Go 1.14 update to the release-1.17 and release-1.18

We might update the Prow jobs to still spin go1.13 for the branches.

Also, Go 1.13 will be unsupported once Go 1.15 is released.

By k8s or Go upstream?

@seanmalloy
Copy link
Member Author

Preferable I'd rather wait until k8s v1.19 is cut and vendored. Though, given Prow jobs have been bumped to 1.14 and I think go1.14 is backward compatible with go1.13, I don't see a big deal of postponing the PR to get merged.

I'll back port the Go 1.14 update to the release-1.17 and release-1.18

We might update the Prow jobs to still spin go1.13 for the branches.

Also, Go 1.13 will be unsupported once Go 1.15 is released.

By k8s or Go upstream?

By Go upstream.

@seanmalloy
Copy link
Member Author

We might update the Prow jobs to still spin go1.13 for the branches.

Yeah ... we should probably do this. I guess my assumption was that the k/k repo would also be updating the Go version for 1.18 and 1.17 release banches, but maybe my assumption is wrong.

@seanmalloy
Copy link
Member Author

opened a PR to test-infra trying to update to 1.14 here: kubernetes/test-infra#18054

@damemi and @ingvagabund If we are going to hold on merging this PR to upgrade to Go 1.14 until k8s 1.19 then we should probably revert the test-infra PR.

@damemi
Copy link
Contributor

damemi commented Jun 24, 2020

I think we could upgrade to go 1.14 on our master branch, since this is technically the 1.19 branch until we cut that release. Wdyt?

@seanmalloy
Copy link
Member Author

I think we could upgrade to go 1.14 on our master branch, since this is technically the 1.19 branch until we cut that release. Wdyt?

@damemi I'm good with having it merged.

@ingvagabund
Copy link
Contributor

ingvagabund commented Jun 24, 2020

I think we could upgrade to go 1.14 on our master branch, since this is technically the 1.19 branch until we cut that release. Wdyt?

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2020
@damemi
Copy link
Contributor

damemi commented Jun 24, 2020

We might update the Prow jobs to still spin go1.13 for the branches.

Yeah ... we should probably do this. I guess my assumption was that the k/k repo would also be updating the Go version for 1.18 and 1.17 release banches, but maybe my assumption is wrong.

I think spinning 1.13 for those branches is probably the best approach. By the time go 1.15 is released, we will probably not need to support tests for 1.18 and 1.17 (and thus go 1.13) anymore anyway, right? (I'm completely guessing, because the go release cycle seems longer than k8s but I don't know exactly what it is)

@seanmalloy
Copy link
Member Author

We might update the Prow jobs to still spin go1.13 for the branches.

Yeah ... we should probably do this. I guess my assumption was that the k/k repo would also be updating the Go version for 1.18 and 1.17 release banches, but maybe my assumption is wrong.

I think spinning 1.13 for those branches is probably the best approach. By the time go 1.15 is released, we will probably not need to support tests for 1.18 and 1.17 (and thus go 1.13) anymore anyway, right? (I'm completely guessing, because the go release cycle seems longer than k8s but I don't know exactly what it is)

@damemi we can keep the release-1.17 and release-1.18 branches for descheduler using Go 1.13. I think the Go 1.15 release will be released in July or August. I think this PR can be merged. Someone will need to figure how to at least get the decheduler Prow jobs for the release-1.18 branch using Go 1.13.

I'll keep track of the Go version used in the k/k repo, and if k8s 1.18 gets updated to use Go 1.14 then I'll get the descheduler release-1.18 branch updated too.

@seanmalloy
Copy link
Member Author

I'm completely guessing, because the go release cycle seems longer than k8s but I don't know exactly what it is)

Go releases twice a year roughly in February and August, https://golang.org/doc/devel/release.html.

@ingvagabund
Copy link
Contributor

Someone will need to figure how to at least get the decheduler Prow jobs for the release-1.18 branch using Go 1.13.

kubernetes/test-infra#18076 could be it

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.

Approved @ingvagabund's changes to add the release-specific 1.13 tests in test-infra. That should do it

/approve

@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 Jun 25, 2020
@k8s-ci-robot k8s-ci-robot merged commit 7ece10a into kubernetes-sigs:master Jun 25, 2020
@seanmalloy seanmalloy deleted the go114 branch June 25, 2020 15:32
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants