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

Use go mod #1144

Merged
merged 12 commits into from
May 22, 2020
Merged

Use go mod #1144

merged 12 commits into from
May 22, 2020

Conversation

xychu
Copy link
Contributor

@xychu xychu commented Mar 17, 2020

Try migrating to use go mod.

Including:

  • go mod init
  • go mod tidy
  • go mod vendor

Note: all changes are made by "go mod" tools, have not checked the versions of all the dependencies one by one.


This change is Reviewable

@TravisBuddy
Copy link

Hey @xychu,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 6f9f7120-681a-11ea-bf55-7111d60dbb66

@TravisBuddy
Copy link

Hey @xychu,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: f43565c0-681a-11ea-bf55-7111d60dbb66

@TravisBuddy
Copy link

Travis tests have failed

Hey @xychu,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

go build -o tf-operator.v1 github.com/kubeflow/tf-operator/cmd/tf-operator.v1
go: inconsistent vendoring in /home/travis/gopath/src/github.com/kubeflow/tf-operator:
	github.com/mattn/goveralls@v0.0.5: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

run 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory
golangci-lint run ./...
ERRO Running error: context loading failed: failed to load program with go/packages: go [list -e -json -compiled=true -test=true -export=false -deps=true -find=false -- ./...]: exit status 1: go: inconsistent vendoring in /home/travis/gopath/src/github.com/kubeflow/tf-operator:
	github.com/mattn/goveralls@v0.0.5: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

run 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory
goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/client/*/*.go,pkg/client/*/*/*.go,pkg/client/*/*/*/*.go,pkg/client/*/*/*/*/*.go,pkg/client/*/*/*/*/*/*.go,pkg/client/*/*/*/*/*/*/*.go,pkg/util/*.go,pkg/util/*/*.go,pkg/apis/tensorflow/*/zz_generated.*.go,pkg/apis/tensorflow/*/*_generated.go,pkg/apis/common/*/zz_generated.*.go,pkg/apis/common/*/*_generated.go"
exit status 1
TravisBuddy Request Identifier: b6593da0-6822-11ea-bf55-7111d60dbb66

@coveralls
Copy link

coveralls commented Mar 17, 2020

Coverage Status

Coverage remained the same at 96.512% when pulling 72eca17 on xychu:use-go-mod into 7e06f5c on kubeflow:master.

@TravisBuddy
Copy link

Travis tests have failed

Hey @xychu,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

go build -o tf-operator.v1 github.com/kubeflow/tf-operator/cmd/tf-operator.v1
go: inconsistent vendoring in /home/travis/gopath/src/github.com/kubeflow/tf-operator:
	github.com/mattn/goveralls@v0.0.5: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

run 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory
golangci-lint run ./...
ERRO Running error: context loading failed: failed to load program with go/packages: go [list -e -json -compiled=true -test=true -export=false -deps=true -find=false -- ./...]: exit status 1: go: inconsistent vendoring in /home/travis/gopath/src/github.com/kubeflow/tf-operator:
	github.com/mattn/goveralls@v0.0.5: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

run 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory
goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/client/*/*.go,pkg/client/*/*/*.go,pkg/client/*/*/*/*.go,pkg/client/*/*/*/*/*.go,pkg/client/*/*/*/*/*/*.go,pkg/client/*/*/*/*/*/*/*.go,pkg/util/*.go,pkg/util/*/*.go,pkg/apis/tensorflow/*/zz_generated.*.go,pkg/apis/tensorflow/*/*_generated.go,pkg/apis/common/*/zz_generated.*.go,pkg/apis/common/*/*_generated.go"
exit status 1
TravisBuddy Request Identifier: 5f0182a0-6823-11ea-bf55-7111d60dbb66

.travis.yml Outdated

script:
- hack/verify-codegen.sh
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the verify script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will add it back later after the code-gen related dependencies get fixed.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks for the PR

@xychu
Copy link
Contributor Author

xychu commented Mar 18, 2020

/retest

@stpabhi
Copy link
Member

stpabhi commented Mar 26, 2020

@xychu is this PR ready to merge? If so, please assign an approver

@xychu
Copy link
Contributor Author

xychu commented Mar 30, 2020

@stpabhi Not yet.
While trying to fix the codegen relevant scripts, I found code-generator(https://github.com/kubernetes/code-generator/tree/v0.15.7) starts to support go mod from v0.15.7, so I've not gotten a nice way to get code-generator#kubernetes-1.12.3 into the vendor dir.
So is it okay we do some upgrade first to support kubernetes 1.15+ or add extra hack scripts to copy/move needed dependencies into vendor to make hack/verify-codegen.sh works for now?

@stpabhi
Copy link
Member

stpabhi commented Apr 1, 2020

@xychu I don’t see any reason not upgrading to 1.15+ kubeflow/common and kubeflow/mpi-operator use 1.15.10

@Jeffwan
Copy link
Member

Jeffwan commented May 16, 2020

@xychu any progress on 1.15? We are moving to kubeflow/common in next few weeks and go module support for tf-operator is a prerequisite step. If you still work on this PR, please help upgrade to 1.15.x or 1.16.x (latest kubeflow/common uses 1.16.x).

@xychu
Copy link
Contributor Author

xychu commented May 18, 2020

@Jeffwan great to know kubeflow/common v0.3.1 out, will start to upgrade to 1.16.x

@TravisBuddy
Copy link

Hey @xychu,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 04555100-98ee-11ea-860e-87b20eb63714

@TravisBuddy
Copy link

Hey @xychu,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 9e378e10-98f2-11ea-860e-87b20eb63714

@TravisBuddy
Copy link

Hey @xychu,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 04fca5d0-98f4-11ea-860e-87b20eb63714

@TravisBuddy
Copy link

Hey @xychu,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 1d845510-98f6-11ea-860e-87b20eb63714

// file from being included in builds.

import (
_ "k8s.io/code-generator"
Copy link
Member

Choose a reason for hiding this comment

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

minor: I would suggest to put into some pkg build util.go etc rather than a separate vendor file outside.


go_import_path: github.com/kubeflow/tf-operator

install:
# get coveralls.io support
- go get github.com/mattn/goveralls
- curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin v1.18.0
- go mod vendor
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to vendor dependencies since this PR moves to go modules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just thought maybe easier to go build with vendor dir, if this is not needed, I'll delete it.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine to keep it now. Not a concern at this moment.

@Jeffwan
Copy link
Member

Jeffwan commented May 18, 2020

/lgtm

@xychu
Copy link
Contributor Author

xychu commented May 19, 2020

@gaocegege @ChanYiLin plz have a look when you got time. I've updated tf-operator to use k8s 1.16.x and go mod since kubeflow/common v0.3.1 had been released out.

@Jeffwan
Copy link
Member

Jeffwan commented May 19, 2020

Thanks for the PR. Once this merged, I will plan to do some more common migration, more on the controller implementation.

@Jeffwan
Copy link
Member

Jeffwan commented May 21, 2020

@gaocegege @ChanYiLin Any comments on the PR? I think PR mainly upgrade kubernetes version, also upgrade kubeflow/common version to latest. (still using API but not implementation) Overall it looks good to me.

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/approve

Thanks for your contribution! 🎉 👍

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants