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

Fix CI failures for Go 1.17 and verifying invalid examples #830

Merged
merged 5 commits into from
Aug 26, 2021

Conversation

youngnick
Copy link
Contributor

@youngnick youngnick commented Aug 26, 2021

Signed-off-by: Nick Young ynick@vmware.com

What type of PR is this?

/kind cleanup
/kind failing-test

What this PR does / why we need it:
Fixes up the generated docs with Go 1.17 build tags, should mean they pass CI now.
Updates #828

It also turned out that the verify-examples script was failing and not actually testing what we though, so I fixed that as well.

Signed-off-by: Nick Young <ynick@vmware.com>
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 26, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 26, 2021
@jpeach
Copy link
Contributor

jpeach commented Aug 26, 2021

/lgtm

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

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

Would make generate on a local machine without go1.17 revert this now? So developers need to use 1.17 now? That seems ok, just wanted to check if that is the case

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, youngnick

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

@youngnick
Copy link
Contributor Author

Would make generate on a local machine without go1.17 revert this now? So developers need to use 1.17 now? That seems ok, just wanted to check if that is the case

I'm not sure. It depends on what Go 1.16.x does with build tags that start with // +go:build instead of just // +build. But I think we should probably mandate 1.17 anyway, since all the tooling is now migrated.

@robscott
Copy link
Member

🤦 I forgot about this part, it looks like we need to copy kubernetes/kubernetes#103692.

Boilerplate header is wrong for: /home/prow/go/src/sigs.k8s.io/gateway-api/apis/v1alpha2/zz_generated.deepcopy.go
Boilerplate header is wrong for: /home/prow/go/src/sigs.k8s.io/gateway-api/apis/v1alpha1/zz_generated.deepcopy.go

@robscott
Copy link
Member

Better link: kubernetes/kubernetes@f11a3cd

@youngnick
Copy link
Contributor Author

Looks like golangci-lint also has a deprecation that's causing a failure, sigh. I'll keep ticking things off.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 26, 2021
@youngnick youngnick changed the title Update generated files for Go 1.17 Fix CI failures for Go 1.17 and verifying invalid examples Aug 26, 2021
Signed-off-by: Nick Young <ynick@vmware.com>
Nick Young added 2 commits August 26, 2021 06:41
Signed-off-by: Nick Young <ynick@vmware.com>
@youngnick
Copy link
Contributor Author

Okay, done. In total, this PR fixes the following:

  • go build tags weren't getting parsed out properly, even after the migration to go 1.17
  • turned out the invalid-examples thing wasn't actually checking anything. It should be slightly more reliable now. I've tried to comment this a bit.
  • Debugging which test out of verify-all.sh was failing was tricky, I missed the go build tag lines failing for hours, so I added in some extra log lines to make it clearer when running in verbose mode.

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @youngnick! A couple tiny nits but otherwise LGTM, this is a huge improvement.

hack/verify-examples-kind.sh Outdated Show resolved Hide resolved
hack/verify-examples-kind.sh Outdated Show resolved Hide resolved
@hbagdi
Copy link
Contributor

hbagdi commented Aug 26, 2021

/lgtm
Adding hold for Rob's comment.
/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 Aug 26, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2021
Signed-off-by: Nick Young <ynick@vmware.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2021
@robscott
Copy link
Member

Thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2021
@youngnick
Copy link
Contributor Author

/hold cancel

@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 Aug 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit 4ebc7e2 into kubernetes-sigs:master Aug 26, 2021
@robscott robscott mentioned this pull request Aug 30, 2021
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

6 participants