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

Bump Go to 1.18 #5152

Merged
merged 1 commit into from Jun 6, 2022
Merged

Bump Go to 1.18 #5152

merged 1 commit into from Jun 6, 2022

Conversation

lucacome
Copy link
Contributor

@lucacome lucacome commented May 25, 2022

Signed-off-by: Luca Comellini luca.com@gmail.com

Pull Request Motivation

Bumps Go from 1.17 to 1.18.
Also updates rules_go and bazel_gazelle for Bazel.

Kind

/kind cleanup

Release Note

Bump cert-manager's version of Go to 1.18

@jetstack-bot jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels May 25, 2022
@jetstack-bot
Copy link
Collaborator

Hi @lucacome. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@jetstack-bot jetstack-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 25, 2022
@lucacome
Copy link
Contributor Author

/assign @irbekrm

@irbekrm
Copy link
Collaborator

irbekrm commented May 25, 2022

Thank you for the PR!

/ok-to-test

@jetstack-bot jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 25, 2022
@irbekrm
Copy link
Collaborator

irbekrm commented May 25, 2022

/test pull-cert-manager-e2e-v1-23

@irbekrm
Copy link
Collaborator

irbekrm commented May 26, 2022

Thanks for the PR! Before we merge this, somebody should go through the release notes and check for any changes potentially related to our codebase https://go.dev/doc/go1.18
If you'd like to have a go feel free, but appreciate that it might be proportionally a lot of work with a less familiar codebase

@irbekrm
Copy link
Collaborator

irbekrm commented May 31, 2022

I've read through Go 1.18 release notes, does not look like there is anything that would affect our codebase.

Some thoughts (not related to this PR's contents):

The only small nit is that we should remove the // +build lines here and here as those are now obsolete- @lucacome I'm adding a 'hold' in case you like to do that in this PR else we can merge as is

/lgtm
/approve
/hold

@jetstack-bot jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 31, 2022
@irbekrm
Copy link
Collaborator

irbekrm commented May 31, 2022

/test pull-cert-manager-make-e2e-v1-24
/test pull-cert-manager-e2e-feature-gates-disabled
/test pull-cert-manager-e2e-v1-20
/test pull-cert-manager-e2e-v1-21
/test pull-cert-manager-e2e-v1-22
/test pull-cert-manager-e2e-issuers-venafi-cloud
/test pull-cert-manager-issuers-venafi-tpp

@cert-manager cert-manager deleted a comment from jetstack-bot May 31, 2022
@irbekrm
Copy link
Collaborator

irbekrm commented May 31, 2022

Actually this will not pass now as we have some infra issues. The Venafi Cloud one passes though, so that should be ok

/override pull-cert-manager-issuers-venafi-tpp

@jetstack-bot
Copy link
Collaborator

@irbekrm: Overrode contexts on behalf of irbekrm: pull-cert-manager-issuers-venafi-tpp

In response to this:

Actually this will not pass now as we have some infra issues. The Venafi Cloud one passes though, so that should be ok

/override pull-cert-manager-issuers-venafi-tpp

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.

@irbekrm
Copy link
Collaborator

irbekrm commented May 31, 2022

known flake

/test pull-cert-manager-e2e-feature-gates-disabled

@jetstack-bot jetstack-bot added area/acme Indicates a PR directly modifies the ACME Issuer code area/acme/dns01 Indicates a PR modifies ACME DNS01 provider code and removed lgtm Indicates that a PR is ready to be merged. labels May 31, 2022
@lucacome
Copy link
Contributor Author

lucacome commented May 31, 2022

@irbekrm I've removed // +build and ran go fix. I've left it in a different commit to make it easier to see the changes, because I think something's off with bazel but I'm not 100% sure.
Also go fix fixed unrelated stuff that I don't know if we want to include in this PR.

EDIT: I see that the build is failing because of the changes in bazel

@lucacome
Copy link
Contributor Author

/retest

1 similar comment
@lucacome
Copy link
Contributor Author

lucacome commented Jun 1, 2022

/retest

@irbekrm
Copy link
Collaborator

irbekrm commented Jun 1, 2022

Thanks for making the changes! I think it's just the goimports that is failing - left a comment for that.

@lucacome lucacome force-pushed the bump-go-1.18 branch 2 times, most recently from cc94f5e to a5d09f2 Compare June 2, 2022 20:44
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jun 2, 2022
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jun 2, 2022
Signed-off-by: Luca Comellini <luca.com@gmail.com>
@lucacome
Copy link
Contributor Author

lucacome commented Jun 2, 2022

@irbekrm I've reverted the change and added back // +build tools in tools.go since it was making Bazel fail.

@lucacome
Copy link
Contributor Author

lucacome commented Jun 2, 2022

/retest

@irbekrm
Copy link
Collaborator

irbekrm commented Jun 6, 2022

Thanks @lucacome !

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2022
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, lucacome

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

@irbekrm
Copy link
Collaborator

irbekrm commented Jun 6, 2022

/hold cancel

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2022
@jetstack-bot jetstack-bot merged commit b72869c into cert-manager:master Jun 6, 2022
@jetstack-bot jetstack-bot added this to the v1.9 milestone Jun 6, 2022
@lucacome lucacome deleted the bump-go-1.18 branch June 6, 2022 07:33
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. area/acme/dns01 Indicates a PR modifies ACME DNS01 provider code area/acme Indicates a PR directly modifies the ACME Issuer code dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. ok-to-test release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

None yet

3 participants