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.

Sign up for GitHub

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

Jump to bottom

Go Modules Migration #2745

Merged
merged 12 commits into from Feb 19, 2020
Merged

Go Modules Migration #2745

merged 12 commits into from Feb 19, 2020

Conversation

LorbusChris
Copy link
Member

@LorbusChris LorbusChris commented Dec 3, 2019 •
edited

Migrate Installer to Go 1.13, Modules and flatten vendor dir.

Pending PRs:

Follow Ups:

Fixes: #1914
Fixes: #2384
Closes: #2676

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 3, 2019
@LorbusChris LorbusChris mentioned this pull request Dec 4, 2019
Closed
@gyliu513
Copy link
Contributor

gyliu513 commented Dec 4, 2019

Fixed #2384

@vrutkovs
Copy link
Member

vrutkovs commented Dec 4, 2019 

 go: modernc.org/golex@v1.0.0: git fetch -f origin refs/heads/*:refs/heads/* refs/tags/*:refs/tags/* in /go/pkg/mod/cache/vcs/9aae2d4c6ee72eb1c6b65f7a51a0482327c927783dea53d4058803094c9d8039: exit status 128:
	error: RPC failed; result=22, HTTP code = 404
	fatal: The remote end hung up unexpectedly

I had to run builds with GOPROXY when vendoring on this

@abhinavdahiya
Copy link
Contributor

if it makes it easier, i think we can drop the tests/bdd-smoke

abhinavdahiya
abhinavdahiya reviewed Dec 4, 2019
View reviewed changes
go.mod Show resolved Hide resolved
abhinavdahiya
abhinavdahiya reviewed Dec 4, 2019
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@LorbusChris
Copy link
Member Author

@abhinavdahiya what about the other one, tests/smoke? Deps there also seem out of date. Can we drop that, too?

abhinavdahiya
abhinavdahiya reviewed Dec 4, 2019
View reviewed changes
go.mod Outdated Show resolved Hide resolved
abhinavdahiya
abhinavdahiya reviewed Dec 4, 2019
View reviewed changes
go.mod Outdated Show resolved Hide resolved
abhinavdahiya
abhinavdahiya reviewed Dec 4, 2019
View reviewed changes
go.mod Outdated Show resolved Hide resolved
abhinavdahiya
abhinavdahiya reviewed Dec 4, 2019
View reviewed changes
go.mod Outdated Show resolved Hide resolved
abhinavdahiya
abhinavdahiya reviewed Dec 4, 2019
View reviewed changes
go.mod Outdated Show resolved Hide resolved
abhinavdahiya
abhinavdahiya reviewed Dec 4, 2019
View reviewed changes
tools.go Outdated Show resolved Hide resolved
@abhinavdahiya
Copy link
Contributor

@abhinavdahiya what about the other one, tests/smoke? Deps there also seem out of date. Can we drop that, too?

yes, can be dropped.

@abhinavdahiya
Copy link
Contributor

would also love to see updates on https://github.com/openshift/installer/blob/master/docs/dev/dependencies.md#go

to cover how to update dependencies? most common ones in-line here would be very useful and pointing to upstream docs for rest.

@LorbusChris
Copy link
Member Author

The govet test is OOMing

@LorbusChris
Copy link
Member Author

I believe the unit test fails because terraform and terraform-plugin-sdk ship some of the same types. I don't know how to fix that. Maybe we can omit just those types from the test somehow?

@LorbusChris
Copy link
Member Author

LorbusChris commented Dec 4, 2019 •
edited

The duplicate type also makes the e2e tests fail

panic: gob: registering duplicate types for "*tfdiags.rpcFriendlyDiag": *tfdiags.rpcFriendlyDiag != *tfdiags.rpcFriendlyDiag

Oh this actually happens inside the vendored files:
https://github.com/hashicorp/terraform/blob/master/tfdiags/rpc_friendly.go#L58
and
https://github.com/hashicorp/terraform-plugin-sdk/blob/master/internal/tfdiags/rpc_friendly.go#L58

Looks like they can't both be initialized at the same time...

This was referenced Dec 4, 2019
Closed
Closed
sgreene570
sgreene570 reviewed Dec 5, 2019
View reviewed changes
.yamllint Outdated Show resolved Hide resolved
hack/verify-vendor.sh Outdated Show resolved Hide resolved
hack/verify-vendor.sh Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2019
@LorbusChris
Copy link
Member Author

/retest

6 similar comments
@LorbusChris
Copy link
Member Author

/retest

@LorbusChris
Copy link
Member Author

/retest

@LorbusChris
Copy link
Member Author

/retest

@LorbusChris
Copy link
Member Author

/retest

@LorbusChris
Copy link
Member Author

/retest

@LorbusChris
Copy link
Member Author

/retest

@abhinavdahiya
Copy link
Contributor

/lgtm

the branching should happen today.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2020
@abhinavdahiya
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2020
@abhinavdahiya
Copy link
Contributor

/refresh

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@LorbusChris
Copy link
Member Author

seeing clone errs which certainly stem from some hickup due to the branching
/retest

@LorbusChris
Copy link
Member Author

/retest

1 similar comment
@abhinavdahiya
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

wanghaoran1988 referenced this pull request in wanghaoran1988/installer Feb 19, 2020
@wanghaoran1988
aws china
b851e23
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@rgolangh rgolangh mentioned this pull request Feb 19, 2020
Closed
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit a479310 into openshift:master Feb 19, 2020
@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-vsphere 112c64a link /test e2e-vsphere
ci/prow/e2e-libvirt d02d3f0 link /test e2e-libvirt
ci/prow/e2e-aws-scaleup-rhel7 d02d3f0 link /test e2e-aws-scaleup-rhel7

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.

@danwinship danwinship mentioned this pull request Feb 20, 2020
Merged
@sgreene570 sgreene570 moved this from In Review to Done in OKD4 Feb 21, 2020
This was referenced May 27, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@rgolangh rgolangh rgolangh left review comments

@sgreene570 sgreene570 sgreene570 left review comments

@jstuever jstuever Awaiting requested review from jstuever

@patrickdillon patrickdillon Awaiting requested review from patrickdillon

Assignees

@abhinavdahiya abhinavdahiya

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. platform/baremetal IPI bare metal hosts platform size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. version/4.5
Projects
OKD4
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Propose migrate from dep to go modules [RFE] Flat vendor directory