-
Notifications
You must be signed in to change notification settings - Fork 2.2k
make: use Docker named volumes for ~21x faster local linting (Mac OS) #10412
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
Conversation
Summary of ChangesHello @Roasbeef, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a strategic optimization to the Docker-based linting process, significantly improving the local development experience on macOS and Windows by leveraging Docker named volumes for caching. This change drastically reduces linting times for warm caches without impacting the efficiency of CI pipelines, which continue to utilize existing GitHub Actions caching mechanisms. The Makefile has been updated to dynamically apply the correct caching strategy based on the execution environment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a smart caching strategy for the Docker-based linter, using named volumes for faster local builds and bind mounts for CI environments to leverage existing caching mechanisms. The implementation is clean and well-commented. My review includes one suggestion to add a make target for cleaning up the newly introduced Docker named volumes, which would help developers manage disk space and ensure a clean build environment when needed.
| -v lnd-go-build-cache:/tmp/build/.cache \ | ||
| -v lnd-go-mod-cache:/tmp/build/.modcache \ | ||
| -v lnd-go-lint-cache:/root/.cache/golangci-lint \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the introduction of named Docker volumes for local development, it would be beneficial to also add a make target to clean them up. These volumes will persist on the user's machine and can grow in size over time. A developer might want to clear them to start from a clean slate or to free up disk space.
You could add a new target like this:
.PHONY: clean-docker-volumes
clean-docker-volumes:
@$(call print, "Removing Docker cache volumes.")
docker volume rm lnd-go-build-cache lnd-go-mod-cache lnd-go-lint-cache || trueThis would improve the developer experience by providing a managed way to handle these new resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding this cleanup also makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will addd
ziggie1984
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, my mac is generally super fast so I did not immediately saw the improvement but also not deterioration therefore if it works for other people let's use named volumes.
This commit optimizes Docker cache mounting for the linter with a CI-aware strategy: **Local development (macOS/Windows)**: Uses Docker named volumes which keep data inside Docker's native Linux filesystem, avoiding the slow host-syncing overhead of bind mounts. This yields ~21x faster linting on warm cache. **CI (GitHub Actions)**: Uses bind mounts to host paths (`~/.cache/go-build`, `~/go/pkg/mod`) that GitHub Actions already caches via the setup-go action. This ensures CI benefits from cached dependencies across runs. The Makefile detects CI mode via the `CI` environment variable that GitHub Actions sets automatically. Local benchmark results: - Cold run (empty cache): ~2m 28s - Warm run (cached): ~11s (~21x faster) Key improvements in warm runs: - Go packages loading: 1m 58s → 5.6s - Linters execution: 20.5s → 2.7s - Total execution: 2m 20s → 8.6s
dac428c to
a21d436
Compare
This commit optimizes Docker cache mounting for the linter with a CI-aware strategy:
Local development (macOS/Windows): Uses Docker named volumes which keep data inside Docker's native Linux filesystem, avoiding the slow host-syncing overhead of bind mounts. This yields ~21x faster linting on warm cache.
CI (GitHub Actions): Uses bind mounts to host paths (
~/.cache/go-build,~/go/pkg/mod) that GitHub Actions already caches via the setup-go action. This ensures CI benefits from cached dependencies across runs.The Makefile detects CI mode via the
CIenvironment variable that GitHub Actions sets automatically.Local benchmark results:
Key improvements in warm runs:
This should keep CI the same, while dramatically speeding up lint time locally (at least on mac).