make: add docker-dev-* targets for building dev images#10912
make: add docker-dev-* targets for building dev images#10912ZZiigguurraatt wants to merge 1 commit into
Conversation
Summary of ChangesHello, 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 set of Makefile targets to simplify the process of building and managing development-specific docker images. By wrapping 'docker buildx build', these changes improve the developer experience in containerized environments while ensuring efficient use of BuildKit cache mounts. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 the 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 counterproductive. 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. Footnotes
|
13e4b8c to
e04ff7e
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new Makefile targets (docker-dev-build, docker-dev-lndinit-build, and docker-dev-lndinit-build-push) to streamline building and pushing development Docker images for lnd and lndinit using BuildKit, alongside updating the corresponding documentation and release notes. The review feedback highlights two important issues in the Makefile: first, the shell parameter expansion for parsing DOCKER_DEV_IMAGE fails when the image name contains a registry port or lacks a tag; second, running the build outside of a git repository causes git describe to fail, leading to an invalid empty tag. Both comments provide robust shell script suggestions to handle these edge cases.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # like `kvdb/v1.5.1` — `/` is not a valid character in a docker image tag. | ||
| # `--match='__no_such_tag__'` forces `git describe` to ignore all tags so | ||
| # `--always` falls back to the short hash. | ||
| DOCKER_DEV_COMMIT := $(shell git describe --always --dirty --match='__no_such_tag__') |
There was a problem hiding this comment.
If make is run outside of a git repository (e.g., from a source archive or in a bare container), git describe will fail and print an error to stderr, resulting in an empty DOCKER_DEV_COMMIT. This causes DOCKER_DEV_IMAGE to be tagged as lnd-dev:, which is an invalid Docker tag. Adding a fallback to latest and redirecting stderr to /dev/null makes the build more robust in non-git environments.
DOCKER_DEV_COMMIT := $(shell git describe --always --dirty --match='__no_such_tag__' 2>/dev/null || echo "latest")
There was a problem hiding this comment.
do we care about this since we already have git used by another of other things in the makefile without this safeguard?
github-actions
Bot
added
severity-low
PR Severity: LOW - Automated classification | 3 files | 110 lines changed. Files: Makefile (build tooling), docs/DOCKER.md (documentation), docs/release-notes/release-notes-0.22.0.md (release notes). All LOW severity. No bumps (3 files, 110 lines). <!-- pr-severity-bot --> |
Adds three make targets that wrap dev.Dockerfile: - docker-dev-build builds an lnd dev image, tagged $(DOCKER_DEV_IMAGE) (default lnd-dev:<short-hash>). - docker-dev-lndinit-build layers an lndinit image on top, tagged $(LNDINIT_REPO):lnd-dev-<short-hash> (default lndinit:...). - docker-dev-lndinit-build-push builds and pushes the lndinit image. All three targets go through `docker buildx build` so the BuildKit cache mounts added in 20e6518 are used.
e04ff7e to
50c5702
Compare
|
|
||
| # Build context (path or git URL with optional #ref) for lndinit's dev.Dockerfile. | ||
| # NOTE: `#` is escaped as `\#` so make doesn't treat the ref as a comment. | ||
| LNDINIT_CONTEXT ?= https://github.com/lightninglabs/lndinit.git\#main |
There was a problem hiding this comment.
do we want to set this to a release instead of tracking the default branch?
cons are we would have to remember to update here when we have new lndinit releases.
There are no changes to request re-review on. I'm waiting on an initial review. |
|
@calvinrzachman: review reminder |
|
Could you clarify why the lnd Makefile should own building and publishing an external lndinit image? The docker-dev-build target seems directly related to this repository, but the docker-dev-lndinit-* targets couple it to the lndinit dev.Dockerfile and its build arguments. Would these targets fit better in the lndinit repository or in the downstream containerized testing environment that consumes both images? |
| #? docker-dev-build: Build a development docker image from dev.Dockerfile (override DOCKER_DEV_IMAGE=<name:tag>) | ||
| docker-dev-build: | ||
| @$(call print, "Building dev docker image $(DOCKER_DEV_IMAGE).") | ||
| docker buildx build -t $(DOCKER_DEV_IMAGE) -f dev.Dockerfile . |
There was a problem hiding this comment.
These recipes run docker buildx build with no --platform, so they build for the host's arch. When building on one arch for a different target (e.g. an Apple Silicon Mac for an amd64 host), the image comes out the wrong arch and won't run there. What do you think about a small knob, defaulting to native so existing behavior is unchanged?
DOCKER_DEV_PLATFORM ?= linux/$(shell go env GOARCH)
# in each recipe:
docker buildx build --platform $(DOCKER_DEV_PLATFORM) ...A cross-build is then just DOCKER_DEV_PLATFORM=linux/amd64. Single-platform on purpose; multi-arch manifests seem reasonably out of scope for a dev image.
| docker buildx build -t $(DOCKER_DEV_IMAGE) -f dev.Dockerfile . | ||
|
|
||
| #? docker-dev-lndinit-build: Build an lndinit dev image layered on the docker-dev-build image (override LNDINIT_REPO=<name>, LNDINIT_CONTEXT=<git ref or path>) | ||
| docker-dev-lndinit-build: docker-dev-build |
There was a problem hiding this comment.
The lnd -> lndinit chain assumes the default docker driver. docker-dev-build builds the lnd image but doesn't --push or --load it, so this target's FROM ${BASE_IMAGE} only resolves on the default docker driver, where -t lands in the local image store and FROM reads it. On a docker-container driver the base isn't resolvable from the local store even with --load (I verified this: FROM falls back to a registry pull and fails), so the base has to be --pushed to a registry. Worth either defaulting docker-dev-build to --push, or documenting that the local chain requires the docker driver, so it doesn't silently fail on docker-container. Not about multi-arch, just the base being resolvable across setups.
There was a problem hiding this comment.
I have not used that workflow, so will look into this.
There was a problem hiding this comment.
To keep this simple: it's fine as-is on the default docker driver. For the docker-container case the lnd image just needs to be pushed to a registry first (--load into the local store isn't enough there — the FROM still resolves from a registry), so no need to support it in the target. Might be worth a one-line note in DOCKER.md that the chain assumes the default docker driver, but that's it.
The number of contributors to lndinit is quite low (https://github.com/lightninglabs/lndinit/graphs/contributors?all=1) and the activity and releases are less dramatic, yet anyone who wants to do dev work and deploy to a kubernetes pod will want to deploy along with lndinit. By having the lnd make file source and build a remote lndinit dockerfile does a few things
|
5 participants
Adds three make targets that wrap dev.Dockerfile:
All three targets go through
docker buildx buildso the BuildKit cache mounts added in 20e6518 are used.This change helps development testing in a containerized network environment.