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

Enable dep's automatic vendor pruning #387

Closed
wants to merge 3 commits into from

Conversation

sdboyer-stripe
Copy link
Contributor

Summary

This turns on all of dep's pruning options. That may be too aggressive, but let's start from there and work backwards.

Motivation

Because...well, look at that line count :)

Test plan

Rollout/monitoring/revert plan

This turns on all the pruners, which may be too aggressive.
@cory-stripe
Copy link
Contributor

Reminder to add a changelog entry!

@sdboyer-stripe
Copy link
Contributor Author

sdboyer-stripe commented Feb 16, 2018

done :)

do we know if any of our dependencies need anything other than pure Go files to compile correctly? hopefully not - if they don't, then those pruning levels should be fine.

@cory-stripe
Copy link
Contributor

Looks like this needs a gofmt or something since it blew up in travis?

But overall, if this compiles them I'm great with it. :)

@sdboyer-stripe
Copy link
Contributor Author

Hmmm...modifying stuff in vendor, even for a gofmt, is gonna be a no-no pretty soon in dep. i guess i can do it for now (it seems like we already have fugly hacks for this in travis that make it tricky for me to change the gofmt invocation), but we'll need to stop fmting vendor soon.

This needs to not be a requirement anymore.
@ChimeraCoder
Copy link
Contributor

Hmmm...modifying stuff in vendor, even for a gofmt, is gonna be a no-no pretty soon in dep

Why is that? If we're vendoring an improperly-formatted repository, shouldn't we be able to fix it? (yes, I know the answer is "they should be gofmt-ing, but as you know, that doesn't always happen).

It's easy for us to disable the check on the vendor directory (just add a git checkout vendor) if we really need to.

@sdboyer-stripe
Copy link
Contributor Author

Why is that?

When we add verification in dep - golang/dep#121 - it will mean that dep keeps its own hashes of the contents of vendor, generated on the basis of exactly what the upstream produced. Those hashes will be used to (among other things) not have to fully repopulate vendor on every dep ensure run. Modifying the files in there will result in dep thinking that they're...well, modified, and thus fully regenerating that subtree.

@ChimeraCoder
Copy link
Contributor

I don't want to block this PR, but a couple of things that come to mind:

  1. The three repositories that are not properly gofmt-ed - is the problem simply that they're using a different version of Go? Would updating them to a newer version of the library fix any of them?

  2. Does vendor/ verification golang/dep#121 mean running gofmt -w . will no longer work as desired, because it won't ignore the vendor directory?

  3. If necessary to merge this PR, I can update the Travis CI check (in a separate PR) to ignore the vendor directory. It'll actually require two steps (not one), but that's fine. Let me know if that's a blocker and I'll do it.

@sdboyer-stripe
Copy link
Contributor Author

The three repositories that are not properly gofmt-ed - is the problem simply that they're using a different version of Go? Would updating them to a newer version of the library fix any of them?

From the diff, it looks like it is at least partially that the upstream authors didn't properly run gofmt themselves; e.g., i see some spaces instead of tabs.

Does golang/dep#121 mean running gofmt -w . will no longer work as desired, because it won't ignore the vendor directory?

yes, if the dependencies have problems like this, it will probably be an issue with dep, as dep will consider their un-fmt'ed form to be what's expected, and will complain about a mismatch.

If necessary to merge this PR, I can update the Travis CI check (in a separate PR) to ignore the vendor directory. It'll actually require two steps (not one), but that's fine. Let me know if that's a blocker and I'll do it.

it's not a blocker, but it would be nice to do. i think the key here is that we shouldn't need to care about whether or not our dependencies have run gofmt or not.

@ChimeraCoder
Copy link
Contributor

yes, if the dependencies have problems like this, it will probably be an issue with dep, as dep will consider their un-fmt'ed form to be what's expected, and will complain about a mismatch.

That definitely seems like an issue in dep, then - I know the gofmt -w . workflow is pretty common, especially since gofmt -w ./... doesn't work. The fact that we happen to have a build check for those changes notwithstanding, I don't think it'd be great for people to have to do a git checkout vendor/ every time they run gofmt across their project in order to prevent this.

Though, that's not related to this issue here. I'm willing to lgtm this, after rebasing onto master!

lgtm

@ChimeraCoder
Copy link
Contributor

Also, just note: this branch hasn't actually been run on Jenkins yet. You may want to resubmit this as a new PR to force it to register on Jenkins.

@sdboyer-stripe
Copy link
Contributor Author

(this is now done)

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

Successfully merging this pull request may close these issues.

3 participants