Skip to content

[WIP] 🌱 Regenerate with Go 1.17rc1 #4953

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

Closed

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Jul 16, 2021

Signed-off-by: Stefan Büringer [email protected]

Please ignore this PR for now, let's discuss it via #4954 first

What this PR does / why we need it:

Our CI Image has been bumped yesterday: test-infra commit. The new image now uses Go 1.17 rc1.

In case we stay on Go 1.17 rc1 this PR would fix our CI by:

  • regenerating with Go 1.17 rc1
  • adjusting boilerplate.py

But I'm not sure what our policy regarding Go upgrades is.

(Note: the Go upgrade only affects the targets we run directly on the host which should be mostly things like generate or lint, our release artifacts are not affected)

P.S. I regenerated via:

# required on Mac to delete the Mac binaries, because they won't work inside the container
make clean-bin

docker run -it -v $(PWD):/go/src/sigs.k8s.io/cluster-api -w /go/src/sigs.k8s.io/cluster-api --entrypoint /bin/bash --privileged gcr.io/k8s-testimages/kubekins-e2e:v20210715-521d667-go-canary runner.sh ./scripts/ci-verify.sh

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 16, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vincepri after the PR has been reviewed.
You can assign the PR to them by writing /assign @vincepri in a comment when ready.

The full list of commands accepted by this bot can be found 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 16, 2021
@sbueringer sbueringer force-pushed the pr-regen-with-go117rc1 branch from ab6f2c6 to f4cec23 Compare July 16, 2021 07:51
Signed-off-by: Stefan Büringer [email protected]
@sbueringer sbueringer force-pushed the pr-regen-with-go117rc1 branch from f4cec23 to 6d17ee2 Compare July 16, 2021 07:52
@fabriziopandini
Copy link
Member

Q: Does this impact developers (What if they run make generate locally with an older version of go)?

@randomvariable
Copy link
Member

Yeah, it would seem a bit weird to impose this on devs as a requirement.

Kubernetes acts a a canary for Go releases, which is why we got bumped to a release candidate version in CI to ensure Go 1.17 is good to go, but it's not our policy to build against RC versions.

Can we disable the go 1.17 vet warning for mismatched headers?

@sbueringer
Copy link
Member Author

sbueringer commented Jul 16, 2021

@randomvariable @fabriziopandini
The issue we run into is that the generate target produces a different output. With our current implementation of generate we force that we use the same version of Go locally and in CI (which didn't matter for the most time, but now we get differences with Go 1.17 rc1). So yes when we keep the current implementation of the generate target we have to use Go 1.17 rc1 locally to get the same results in generate (which is definitely not great).

Note: The difference is that in every generated file we now have the following header:

//go:build !ignore_autogenerated_core_v1alpha3
// +build !ignore_autogenerated_core_v1alpha3

Instead of only:

// +build !ignore_autogenerated_core_v1alpha3

Alternative solutions:

  • downgrade our CI image again. Not sure if that's possible, because the upgrade is automated, but maybe we can opt out of the canary images?
    • A lot of other jobs are using: gcr.io/k8s-testimages/kubekins-e2e:v20210715-521d667-master So I assume we can try to open a PR to change to that image and then the image bumper will keep us on that "version" of the image.
  • run our generate targets with a specified go version and not the local one. Probably the easiest way to do this is to run it in a container.
  • not sure why it generates another header now, maybe we can do something about it?

@randomvariable
Copy link
Member

randomvariable commented Jul 16, 2021

not sure why it generates another header now, maybe we can do something about it?

It's in the release notes for Go 1.17: https://tip.golang.org/doc/go1.17

//go:build lines
The go command now understands //go:build lines and prefers them over // +build lines. The new syntax uses boolean expressions, just like Go, and should be less error-prone. As of this release, the new syntax is fully supported, and all Go files should be updated to have both forms with the same meaning. To aid in migration, gofmt now automatically synchronizes the two forms. For more details on the syntax and migration plan, see https://golang.org/design/draft-gobuild.

New warning for mismatched //go:build and // +build lines
The vet tool now verifies that //go:build and // +build lines are in the correct part of the file and synchronized with each other. If they aren't, gofmt can be used to fix them. For more information, see https://golang.org/design/draft-gobuild.

We could do a hack here with sed and ensure the //go:build lines are added for Go 1.16 as well.

@sbueringer
Copy link
Member Author

gofmt (1.17) also works

@sbueringer sbueringer changed the title 🌱 Regenerate with Go 1.17rc1 [WIP] 🌱 Regenerate with Go 1.17rc1 Jul 16, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2021
@sbueringer
Copy link
Member Author

As written above let's continue in: #4954

@vincepri
Copy link
Member

/hold

@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 Jul 16, 2021
@sbueringer
Copy link
Member Author

/close
will be fixed via: kubernetes/test-infra#22916

@k8s-ci-robot
Copy link
Contributor

@sbueringer: Closed this PR.

In response to this:

/close
will be fixed via: kubernetes/test-infra#22916

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants