-
Notifications
You must be signed in to change notification settings - Fork 20
Add docker #71
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
base: main
Are you sure you want to change the base?
Add docker #71
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
5d4ec81 to
61851cb
Compare
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
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.
Thank you! Pretty much looks good to me!
One minor nit: please rewrite your commit messages to wrap at 72 characters. In general, feel free to consult https://cbea.ms/git-commit/ for guidance on how to write good commit messages.
docker-compose.yml
Outdated
|
|
||
| bitcoin: | ||
| container_name: ldk-server-bitcoin | ||
| image: blockstream/bitcoind:29.1@sha256:9fcffa83feed0fc382dfb2f26990ca07656d150ba49434096a5aeedac6695a01 |
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.
Would be cool to go for v30 directly here, just pinged somebody from Blockstream to check if they intend to upgrade the Docker image soon.
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.
Sounds good, let’s wait for their feedback then.
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.
And there it is: https://hub.docker.com/r/blockstream/bitcoind
Please update!
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.
Done!
Implements multi-stage build to produce a small final image by discarding build dependencies. Supports optional BUILD_FEATURES for additional features, with cached compilation in builder stage.
61851cb to
ccdefcd
Compare
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, I think.
Happy to land this as soon as #69 lands.
introduces docker-compose.yml with Bitcoin Core, CLN, and LND for Lightning testing. The default profile runs these services; the 'ldk-server' profile adds the LDK server; the 'rabbitmq' profile includes RabbitMQ for events.
ccdefcd to
d573de7
Compare
| args: | ||
| BUILD_FEATURES: "" | ||
| command: [ | ||
| "--node-listening-address=0.0.0.0:3001", |
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.
seems weird for ldk-server we use the non-default 3001 but give the default 9735 to lnd
| COPY --from=builder /opt/app/ldk-server/ldk-server-config.toml /usr/local/bin/ldk-server-config.toml | ||
| RUN chmod +x /usr/local/bin/ldk-server | ||
|
|
||
| EXPOSE 3000 3001 |
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.
should expose 9735
|
|
||
| RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y | ||
| ENV PATH="/root/.cargo/bin:${PATH}" | ||
| RUN rustup default 1.90.0 |
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.
why use a debian container if you're just going to install rustup into it? why not just use the rust container
| COPY --from=builder /opt/app/target/release/ldk-server /usr/local/bin/ldk-server | ||
| COPY --from=builder /opt/app/target/release/ldk-server-cli /usr/local/bin/ldk-server-cli | ||
| COPY --from=builder /opt/app/ldk-server/ldk-server-config.toml /usr/local/bin/ldk-server-config.toml | ||
| RUN chmod +x /usr/local/bin/ldk-server |
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.
is this needed? if it is should chmod the cli too
Adds Docker support for building and testing LDK Server. Includes a multi-stage Dockerfile for optimized builds and a docker-compose.yml for a full testing environment with Bitcoin Core, CLN, and LND.
BUILD_FEATURES) and final stage for runtime. Exposes ports 3000 and 3001 for REST and Lightning connectivity.docker compose up).docker compose --profile ldk-server up).docker compose --profile rabbitmq up).this pr depends on #69