Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Generate vendor directories #28

Merged
merged 3 commits into from
May 9, 2019
Merged

Generate vendor directories #28

merged 3 commits into from
May 9, 2019

Conversation

terrytangyuan
Copy link
Member

@terrytangyuan terrytangyuan commented Apr 24, 2019

This is basically the output of go mod vendor. Also updated .gitignore to stop ignoring updates in vendor directories.

We may want to consider keeping vendor directories even though go command completely ignores them when using modules. This allows interoperation with older versions of Go, or to ensure that all files used for a build are stored together in a single file tree as mentioned here. Some IDEs might need a vendor directory in order to correctly find the source files as well. It doesn't seem like vendoring is going away as illustrated here based on community feedback. @merlintang also raised a question related to this in #23 where vendor directories might help with compatibility.

Let's discuss this in the PR.


This change is Reviewable

@richardsliu
Copy link
Contributor

@terrytangyuan A couple of questions:

  • What happens if we change the dependencies in go.mod? Wouldn't the vendor files be out of sync? Do we need to automatically run this whenever go.mod changes?
  • How expensive is it to upgrade to new versions of Go? Using go modules seem to be the recommended way going forward.
  • If we really have to support interop with older Go versions, should we add Travis tests for older Go versions?

@johnugeorge What do you think?

@terrytangyuan
Copy link
Member Author

@terrytangyuan A couple of questions:

  • What happens if we change the dependencies in go.mod? Wouldn't the vendor files be out of sync? Do we need to automatically run this whenever go.mod changes?

Yes we would need to run go mod vendor every time when there are dependency changes. I don't think this happens very often though. We could add some pre-commit git hooks if needed to make sure it's always up-to-date or have CI to do this for us.

  • How expensive is it to upgrade to new versions of Go? Using go modules seem to be the recommended way going forward.

If it's an existing large codebase, it would take quite some time, especially if developers need to stop developing feature branches and let upgrade go in first (to avoid conflicts during upgrade especially for long-running branches). Even though go modules seem to be recommended, there are quite few use cases and discussions from the community indicating that vendoring isn still needed in the future for quite some time.

  • If we really have to support interop with older Go versions, should we add Travis tests for older Go versions?

Definitely, we could do that if we agree to support them. It's fairly easy to test multiple versions of Go on Travis.

@johnugeorge What do you think?

@johnugeorge
Copy link
Member

Just to add. There are incompatible style changes that have happened in Go 1.11. Because of this, it is difficult to support Go linter in 1.11 and earlier versions together.

@terrytangyuan
Copy link
Member Author

I think it's fine since they are only style changes. I don't see a strong reason to support multiple versions of Go linter though (I believe there are workaround to support them too if we really want to do so, e.g. an example). We can just use a specific version of gofmt as also mentioned here:

Note that these kinds of minor updates to gofmt are expected from time to time. In general, systems that need consistent formatting of Go source code should use a specific version of the gofmt binary.

@jian-he
Copy link
Contributor

jian-he commented Apr 30, 2019

I'm always running issues with go build like below.
After I remove go.sum and rebuild, it works fine.. any idea what's going on ?

jianhs-MacBook-Pro:common jian.h$ go build ./...
go: verifying k8s.io/kubernetes@v1.11.2: checksum mismatch
	downloaded: h1:AAwZncqPvpeV6DPF8/z2g05/79cumA6IunGDByI1bOs=
	go.sum:     h1:0XeIy/mHhEFiXkcCNn25muHIMJ/aYT2E9Uea40aD9Ck=

@terrytangyuan
Copy link
Member Author

terrytangyuan commented Apr 30, 2019

I had similar issue earlier and thought that was fixed by the updates on go.sum. It looks the fix doesn't work for everyone then. I am not sure what is causing that but it brings up one benefit of keeping vendor directories - users can build the project via go build -mod=vendor ./... using a specific path to vendor directories if they want to without going through potential problems with go modules.

@gaocegege
Copy link
Member

@jian-he I think the problem may be caused by the update of the upstream. k8s is updated but the version in go.sum is older. go module will check the version every time we run go build or other commands.

@terrytangyuan
Copy link
Member Author

Thanks everyone. If there’s any objection to this PR, please comment here.

@richardsliu
Copy link
Contributor

/lgtm

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.

/lgtm
/approve

@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

@k8s-ci-robot k8s-ci-robot merged commit 3ddaa9a into kubeflow:master May 9, 2019
@terrytangyuan terrytangyuan deleted the vendor-folder branch May 9, 2019 01:53
georgkaleido pushed a commit to georgkaleido/common that referenced this pull request Jun 9, 2022
* disable nudging for old ps plugin
* Update dangerfile.ts

Co-authored-by: Alexander Graf <alex@basecamp.tirol>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants