ensure: always write vendor even if not empty #889
Conversation
bradleyfalzon
force-pushed
the
ensure-always-vendor
branch
from
0679151 to
9bafc5f
Compare
|
I had been thinking about the slow downs, but dep ensure is a pretty deliberate action, I think users would be 100% OK with this. But my main issue was dep ensure wasn't adding dependencies when they didn't exist, due to my workflow documented in README - we could implement the previous PR which only rebuilt vendor when the dependency was missing? |
|
@bradleyfalzon oh no, no, i also think this is the right, safest solution for now, and that that's where need to start. i was just trying to draw @ibrasho's attention to some of the relevant considerations, here 😄 |
|
Can't think of anything wrong with this atm. Only nit is that the |
I agree @ibrasho, the docs suggests this too:
I think we should then remove the if block entirely. |
If the vendor directory already exists, and the lock file hasn't changed, even though a project may be missing from the vendor directory, dep ensure would not add the dependency to the vendor folder. If the project is in the vendor folder, but it has been modified (no longer in sync with lock file), ensure would not replace the dependency in the vendor folder. This change causes dep ensure to run anytime there's a solution, regardless of the state vendor folder is in, erring on the side of replacing vendor without checking existing state which is the most correct behaviour given the ensure intention. Future optimisations may want to check and verify the contents of the vendor folder before blinding replacing it. Fixes golang#883.
bradleyfalzon
force-pushed
the
ensure-always-vendor
branch
from
9bafc5f to
1aa2730
Compare
|
I've amended the commit, @ibrasho can you review again? |
|
I'm good with this atm. 😁 @sdboyer I'm not sure where vendor verification is going to be triggered and how it affects the |
once we have verification in place, we'll need to rewrite a few things, possibly including that flag. verification is likely to be at a different level, though, as verification will be on a per-project basis, not the whole vendor dir. |
|
yep, i think we're good. in we go - thanks, @bradleyfalzon! 🎉 |
What does this do / why do we need it?
If the vendor directory already exists, and the lock file hasn't
changed, even though a project may be missing from the vendor
directory, dep ensure would not add the dependency to the vendor
folder.
If the project is in the vendor folder, but it has been modified
(no longer in sync with lock file), ensure would not replace the
dependency in the vendor folder.
This change causes dep ensure to run anytime there's a solution,
regardless of the state vendor folder is in, erring on the side
of replacing vendor without checking existing state which is the
most correct behaviour given the ensure intention.
Future optimisations may want to check and verify the contents of
the vendor folder before blinding replacing it.
What should your reviewer look out for in this PR?
Check the original issue #883, and then my initial proposal PR in #884, sdboyer then gave this solution a tentative OK in #883 (comment)
Do you need help or clarification on anything?
Edge cases, or any thoughts where this might be a bad idea. Such as if we've ever recommended doing in place edits to vendor directory.
No tests are included, as this is pretty critical behaviour, I presume an integration test or similar would be in order?
Which issue(s) does this PR fix?
Fixes #883.