Skip to content
This repository has been archived by the owner on Dec 9, 2022. It is now read-only.

switch from go dep tool to rancher/trash #68

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

ilackarms
Copy link

@ilackarms ilackarms commented Aug 2, 2017

This PR moves us away from using dep and Gopkg.toml for dependency management in favor of rancher/trash. Changes

  • Remove Gopkg.{toml,lock}
  • Create vendor.conf needed by rancher
  • Re-generate vendor folder using trash; ~400 vendored files removed

@ilackarms
Copy link
Author

@enoodle @pweil- please review

@simon3z
Copy link

simon3z commented Aug 2, 2017

Nice! 🎉

@simon3z
Copy link

simon3z commented Aug 3, 2017

@enoodle can you review/approve?

@simon3z simon3z requested review from mfojtik and pweil- August 3, 2017 10:48
Dockerfile Outdated

RUN GOBIN=/usr/bin \
GOPATH=/go \
trash
Copy link

Choose a reason for hiding this comment

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

@ilackarms what is this needed for? is it downloading the missing dependencies? Looking at skopeo I couldn't find anything similar. When are they running it?

Copy link
Author

Choose a reason for hiding this comment

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

@simon3z come to think of it, we don't actually need this here since we are committing the vendor folder. As long as we make sure to keep vendor and vendor.conf updated in each commit, this line is not needed.

Copy link

Choose a reason for hiding this comment

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

@ilackarms great, then I think we should remove this.

Copy link
Author

Choose a reason for hiding this comment

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

It's removed

@sdboyer
Copy link

sdboyer commented Aug 4, 2017

hi! dep maintainer here. i'm not trying to throw a wrench in you process, but i was hoping you could tell me what problem you've encountered with dep that's motivating you to move to trash? i scanned through the other issues in the repo, but nothing jumped out at me.

again, i'm really not trying to argue about whether this is the right choice. i'm just trying to understand if there's something crucial dep is missing. learning about such things is how we improve!

@ilackarms
Copy link
Author

ilackarms commented Aug 4, 2017

@sdboyer we were experiencing a much larger vendor directory using dep; trash removed a lot more unnecessary files from vendor/

@sdboyer
Copy link

sdboyer commented Aug 4, 2017

@ilackarms ah yeah, gotcha 🖖 . is that including after running dep prune? (re: that, note also golang/dep#944)

@simon3z
Copy link

simon3z commented Aug 4, 2017

Hi @sdboyer we'd like to keep using dep if possible but even after pruning the amount of vendored files is much higher compared to trash. I'd have to say that I haven't investigated why is that (maybe you have some details on this). Any suggestion on how to achieve the same result of this PR with dep? (IIUC ~500 files removed from vendor)

@sdboyer
Copy link

sdboyer commented Aug 4, 2017

@simon3z i don't know offhand what trash's algorithm removes from vendor, unfortunately, so i can't say for sure. the obvious candidates would be *_test.go files and non-go files, as those are two things our current pruning logic doesn't do.

this is the kind of thing you could probably write a reasonably simple script to do. alternatively, one of our contributors has also set up their gitignore to do the work for them: golang/dep#120 (comment)

@ilackarms
Copy link
Author

@sdboyer i can run a quick comparison of dep + gitignore and trash, i'll post the results here

@ilackarms
Copy link
Author

against current version of master:

committed files in vendor
just dep 1033
dep + .gitignore 674
trash 367

using git diff I found a list of files that dep adds to vendor, which trash does not:

git diff trash-deps trashcomparison  --name-only --diff-filter=A | wc -l 
384

list of the files here

@sdboyer
Copy link

sdboyer commented Aug 4, 2017 via email

@sdboyer
Copy link

sdboyer commented Aug 4, 2017 via email

@ilackarms
Copy link
Author

yes, ran prune after ensure

@sdboyer
Copy link

sdboyer commented Aug 8, 2017

ahh, so, i think i see where the big gap is coming from: trash appears to be ignoring your test files with the // +build integrationtest tag. that renders those tests uncompilable. and, given that this is your depgraph:

dep

yaml and x/sys are likely excluded in their entirety by trash. when i remove that build tag so that trash sees ginkgo & gomega, the gap narrows considerably.

Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

Unless we can get the same level of pruning from dep I think we should switch. We can always change back when dep can improve our dependency management.

@simon3z
Copy link

simon3z commented Aug 23, 2017

yaml and x/sys are likely excluded in their entirety by trash. when i remove that build tag so that trash sees ginkgo & gomega, the gap narrows considerably.

@sdboyer is that something we can do ourselves / configure, or does it require code-changes in dep?

@sdboyer
Copy link

sdboyer commented Aug 24, 2017

@simon3z there are two ways i can think of to cause ginkgo and gomega to be ignored:

  1. add github.com/onsi/gomega and github.com/onsi/ginkgo to the ignored list in Gopkg.toml
  2. moving pkg/inspector/{image-inspector_ginkgo_test.go,inspector_test_suite.go} into a subdirectory that Go normally treats as hidden, e.g. pkg/inspector/_inspector_tests/

unfortunately, these are both a bit crude. but we don't yet support any kind of first-class operations relating to build tags - golang/dep#291.

@simon3z simon3z merged commit 2411a70 into openshift:master Aug 25, 2017
@ilackarms ilackarms deleted the trash-deps branch August 27, 2017 10:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants