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

Reintroduce vendoring #384

Closed
jayunit100 opened this issue Feb 15, 2019 · 11 comments
Closed

Reintroduce vendoring #384

jayunit100 opened this issue Feb 15, 2019 · 11 comments

Comments

@jayunit100
Copy link

jayunit100 commented Feb 15, 2019

Why vendoring is critical

  • Airgapped places and people building release candidates need all the dependencies, and cant rely on the internet (even if its available - see the next bullet
  • In upstream kubernetes, we were burnt pretty bad with Figure out what to do about go-bindata . kubernetes/kubernetes#65871 , so, as we move to more stable , independently runnable katib releases, we'll definetly want a hermetic build for the overall project.
  • Convention: other CNCF projects vendor all dependencies

Especially in the case of many AI companies ~ its pretty critical to be able to build a project without any external dependencies, and makes CI easier.

cc @pdmack @jlewi

Can we do it while still making PRs easy to review ?

Yes:

  • Lets make a policy that any vendor changes will happen in a separate commit, that is quite easy to enforce, and can even be automatically enforced
  • i volunteer to add a check for this that confirms orthogonal commits if thats a gating factor to reintroduce vendoring.
@jayunit100 jayunit100 mentioned this issue Feb 15, 2019
@jlewi
Copy link
Contributor

jlewi commented Feb 19, 2019

Are you proposing this just for Katib or more generally?

/cc @richardsliu @johnugeorge

@johnugeorge
Copy link
Member

Yeah. we haven't followed consistent style across packages. eg: TF/Pytorch operators are vendored. But, MPI-operator, Katib are not vendored currently.

@richardsliu
Copy link
Contributor

I agree, we should follow a consistent standard across our operators and introduce proper vendoring.

@gaocegege
Copy link
Member

I think we had a discussion #151

We keep the gopkg.lock in the version control system now. Users can restore the vendor using dep ensure.

AFAIK, CNCF project https://github.com/vitessio/vitess does not maintain vendor in its vendor dir: https://github.com/vitessio/vitess/tree/master/vendor

But, I also think we should follow a consistent standard across all projects.

@andreyvelich
Copy link
Member

What do you think about using Go module, instead of go dep ?
Pipelines have already migrated on it:
kubeflow/pipelines#581

@richardsliu
Copy link
Contributor

We are using go module in the common repo: https://github.com/kubeflow/common

The only requirement is moving to Golang 1.11 or above. I find it easier than vendoring.

@andreyvelich
Copy link
Member

I think, we can switch to go module when v1alpha2 components will be merge.
What do you think ?

@gaocegege
Copy link
Member

SGTM, but I think we should come to an agreement at the community level.

@johnugeorge
Copy link
Member

we can do it at a later point when all components are merged and before the current release in July.

@gaocegege
Copy link
Member

/close

We have vendor now.

@k8s-ci-robot
Copy link

@gaocegege: Closing this issue.

In response to this:

/close

We have vendor now.

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.

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

No branches or pull requests

7 participants