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

Upgrade to go 1.14.4 #9499

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Upgrade to go 1.14.4 #9499

merged 1 commit into from
Jul 14, 2020

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented Jul 5, 2020

k/k 1.19 is using go 1.14.4 so this updates kops to match. kubernetes/kubernetes#88638

Using these PRs as a reference: #8882 #8886

Depends on #9428

also reverts #9396 since it is no longer needed (go 1.14's default behavior changed)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 5, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2020
@rifelpet
Copy link
Member Author

rifelpet commented Jul 5, 2020

We may have to update the required statuses in test-infra which means any open PRs would need to rebase :/ unless we decide to drop the jobs being added/removed from being required for some period of time.

https://github.com/kubernetes/test-infra/blob/1771aee48e8d2e88811e71d50b18994d79e01f53/config/prow/config.yaml#L195-L202

what if, for a long term solution, we used a different term instead of the go version in the GHA job name? like:

  • build (ubuntu-18.04, 1.13) -> build (linux, primary)
  • build (ubuntu-18.04, 1.14) -> build (linux, secondary)
  • build (macos-10.15, 1.13) -> build (darwin, primary)
  • verify (ubuntu-18.04, 1.13) -> verify (linux, primary)

suggesting that we have a "primary" and "secondary" go versions. That way we can bump Go or OS versions without adjusting the required status contexts.

thoughts?

@rifelpet rifelpet force-pushed the go-1-14 branch 3 times, most recently from f635c64 to 78b57bd Compare July 5, 2020 22:17
@johngmyers
Copy link
Member

Using "primary"/"secondary" would have the consequence of allowing a merge based on a GHA job that ran with an older go version. That might be okay, since it's only macos where we don't have Prow coverage.

@rifelpet
Copy link
Member Author

rifelpet commented Jul 5, 2020

Yea that's true. I suppose we had the same limitation with Travis before GHA. Regardless I think we'll need to remove at least some of the GHA jobs from the required statuses and we'll need to wait some time before readding the new jobs' names in order to minimize disruption of other open PRs.

@johngmyers
Copy link
Member

Certainly allowing merges based on runs with older Go versions is better than allowing merges with failed runs.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020
@rifelpet
Copy link
Member Author

With #9552 merged this has been simplified a bit and is ready for review


env GOBIN="${WORK_DIR}/go/bin" GOPATH="${WORK_DIR}/go/" go install -v k8s.io/code-generator/cmd/conversion-gen/
env GOBIN="${WORK_DIR}/go/bin" GOPATH="${WORK_DIR}/go/" go install -mod=mod -v k8s.io/code-generator/cmd/conversion-gen/
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether this should be mod or vendor? But I don't think it's a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not vendoring k8s.io/code-generator so I believe this was always installing from upstream or the module cache. It might be a good idea to vendor it by adding it to tools.go but that can happen in a followup PR.

@justinsb
Copy link
Member

This LGTM, but I'm afraid there's a conflict on the makefile

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, rifelpet

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 removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 14, 2020
@hakman
Copy link
Member

hakman commented Jul 14, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2020
@hakman
Copy link
Member

hakman commented Jul 14, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 7540a3d into kubernetes:master Jul 14, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jul 14, 2020
@rifelpet rifelpet mentioned this pull request Jul 14, 2020
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants