WIP: Check if project is missing from vendor during ensure #884
Conversation
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 it. This change checks if we're not already writing the vendor directory whether any of the projects are missing from the vendor directory and if they are, ensures the writer writes out the vendor directory. Fixes golang#883.
| projVendorPath := filepath.Join(p.AbsRoot, "vendor", string(proj.Ident().ProjectRoot)) | ||
| projInVendor, err := fs.IsNonEmptyDir(projVendorPath) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "ensure %v is a directory or remove it") |
There was a problem hiding this comment.
Needs an extra arg to Wrapf, the %v is not satisfied.
There was a problem hiding this comment.
Thanks @groob, very strange as I was switching between variables at the time, and seemed to have incorrectly settled on neither.
But I'm more concerned that this is even required, as the functionality seems to be missing (as per the issue), but even the README documents this should work. So I don't actually think this PR is even required, or at least the bug is likely elsewhere.
|
Note to self: try and remove the previous check if the vendor folder exists, no point checking parent and children, which just checking the children is probably enough. Also what's solution != nil in the previous if block? |
|
Closing in favour of #889. |
What does this do / why do we need it?
Currently it appears (see #883) that a project missing from the vendor directory isn't added during a
dep ensure.What should your reviewer look out for in this PR?
Everything, this is my first contribution, importantly, the original issue needs to be verified first. Then is this the best way and location to do this behaviour.
It's a WIP until the issue can be verified first.
Do you need help or clarification on anything?
The error handling from
fs.IsNonEmptyDir, it's not clear how we should handle these errors (log the error and continue, or try and delete the directory?).The verbose logging, I couldn't find nearby examples of best current practices, so I've just made it up. I think we should log this behaviour, but is this the correct logger, format, message and terminology (
vendor pathorvendor treeor something else)? Currently only one message will be written and then the loop is broken, so the message instead be%q (and potentially more) needs to be added to vendor path?Commit message needs to be verified.
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.