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

Go 1.13 #2040

Closed
wants to merge 5 commits into from
Closed

Go 1.13 #2040

wants to merge 5 commits into from

Conversation

chenrui333
Copy link

Description of the change:

Upgrade the repo to use go v1.13.

Previous efforts in here, #1949

Motivation for the change:

Homebrew build is using go v1.13, Homebrew/homebrew-core#45185

Also personal interest to bump the repo to use the latest golang offering.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 10, 2019
@openshift-ci-robot
Copy link

Hi @chenrui333. Thanks for your PR.

I'm waiting for a operator-framework or openshift 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.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 10, 2019
@chenrui333
Copy link
Author

/ok-to-test

@openshift-ci-robot
Copy link

@chenrui333: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

I've actually been working on a branch for this myself, but this looks like a good start, so I'm happy to use this one.

There are a few more changes we need to make in this PR:

  1. Refactor code that references GO111MODULE and GOPROXY environment variables, since there are new defaults in go1.13. I think the list of changes is probably:
    • Remove the lines in Makefile that set GO111MODULE and GOPROXY
    • Update internal/util/projutil.GoModOn() code and comments to handle new GO111MODULE behavior
    • Update error text in places that GoModOn is called to reflect new GO111MODULE behavior
  2. Update comments and documentation to reflect the new requirement to use go1.13+ and the new Go tooling behavior (e.g. https://github.com/operator-framework/operator-sdk/blob/master/doc/user-guide.md#go-modules)
  3. Find any other references to go 1.12 and change them to go 1.13 (e.g. in hack/ci/setup-build-dependencies.sh)

Another thing we're planning to do when we switch to go 1.13 is drop support for generating projects to use dep, but I think we can probably do that as a separate PR since it isn't directly related.

go.mod Outdated Show resolved Hide resolved
@joelanford
Copy link
Member

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 11, 2019
@joelanford
Copy link
Member

@chenrui333 You can use #1949 to find the changes I made that I mentioned above. That PR also has the dep removal included as well, but for the sake of smaller PRs, I'd suggest ignoring those changes.

go.sum Outdated
github.com/ugorji/go v1.1.1 h1:gmervu+jDMvXTbcHQ0pd2wee85nEoE0BsVyEuzkfK8w=
github.com/ugorji/go v1.1.1/go.mod h1:hnLbHMwcvSihnDhEfx2/BzKp2xb0Y+ErdfYcrs9tkJQ=
github.com/ugorji/go v0.0.0-20181204163529-d75b2dcb6bc8 h1:Kcv6ck5PWsaDifMwDDgexKfbmP1k63r3tcVppPa+FyM=
github.com/ugorji/go v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:hnLbHMwcvSihnDhEfx2/BzKp2xb0Y+ErdfYcrs9tkJQ=
github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8 h1:3SVOIvH7Ae1KRYyQWRjXWJEA9sS/c/pjvH++55Gr648=
Copy link
Contributor

Choose a reason for hiding this comment

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

@joelanford.

I was using the 1.13 for a mistake and I checked the error; #2055

Which would be solved by replacing the version as the following example.
// Workaround to solve gin-gonic/gin#1673
github.com/ugorji/go v1.1.1 => github.com/ugorji/go/codec v0.0.0-20190204201341-e444a5086c43

Just to share in order to help with.

@chenrui333
Copy link
Author

circle back on this now. Thanks for the direction, @joelanford !

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 15, 2019
@chenrui333
Copy link
Author

Relates to openshift/release#5443

@openshift-ci-robot
Copy link

@chenrui333: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/unit da39cd5 link /test unit
ci/prow/sanity da39cd5 link /test sanity
ci/prow/e2e-aws-go da39cd5 link /test e2e-aws-go
ci/prow/e2e-aws-helm da39cd5 link /test e2e-aws-helm
ci/prow/images da39cd5 link /test images
ci/prow/e2e-aws-ansible da39cd5 link /test e2e-aws-ansible
ci/prow/e2e-aws-subcommand da39cd5 link /test e2e-aws-subcommand

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@joelanford
Copy link
Member

@chenrui333 It turns out we need the --dep-manager removal from #1949 a little sooner than I thought (for #2083). Since #1949 has those changes and is passing CI, I'm going to close this in favor of that.

Sorry about the mixup, and thanks for the contribution and assistance! More PRs are definitely welcome!

@joelanford joelanford closed this Oct 21, 2019
@chenrui333
Copy link
Author

no problem, glad it works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

4 participants