fix: add user defined volumes to make targets#15347
Conversation
There was a problem hiding this comment.
Hey @BradLugo - I've reviewed your changes - here's some feedback:
- Add a brief comment in the Makefile explaining the purpose and usage of the
ADDITIONAL_LOCAL_VOLUME_ARGSvariable so it's easy for future maintainers to discover. - Consider using
LOCAL_VOLUME_ARGS += $(ADDITIONAL_LOCAL_VOLUME_ARGS)(with a default?=for the new var) instead of embedding the conditional in the:=assignment to improve readability.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at df3cfb7. To use with deploy scripts, first |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15347 +/- ##
==========================================
+ Coverage 49.13% 49.23% +0.10%
==========================================
Files 2569 2573 +4
Lines 188638 188845 +207
==========================================
+ Hits 92680 92978 +298
+ Misses 88646 88549 -97
- Partials 7312 7318 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Got me curious because I know very little about this. What was a scenario when you ran in this error? I see what you did in testing but I cannot figure what the real usage might have been. Also, what Sourcery suggests seems like an improvement to me.
Also, I'd call these |
I've been testing out a new workflow for larger projects I contribute to on a regular basis. The structure looks like this: To achieve this structure: This will create a bare git directory in the top-level The idea behind each of these directories:
Having these separate worktrees helps avoid needing to keep track of stashes, temporary branches/commits, and otherwise clutter versions of the project I want to keep clean (I often have a lot of junk lying around my |
Description
The
*-dockerizedmake targets will map, among other volumes, the current working directory to/src. Some scripts used by these make targets will run git commands against/src. However, a developer will be met with an error if the current working directory is a git worktree that references a git directory outside of the current directory.These changes allow developers to set a new variable,
ADDITIONAL_LOCAL_VOLUME_ARGS, that will be appended toLOCAL_VOLUME_ARGS.User-facing documentation
Testing and quality
[ ] the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flagAutomated testing
Current automated testing should be sufficient.
How I validated my change
Before:
After (note the variable export at the start):