Skip to content

docker: Add a usable dockerfile - #1616

Merged
rustyrussell merged 1 commit into
ElementsProject:masterfrom
cdecker:dockerfile
Jun 21, 2018
Merged

docker: Add a usable dockerfile#1616
rustyrussell merged 1 commit into
ElementsProject:masterfrom
cdecker:dockerfile

Conversation

@cdecker

@cdecker cdecker commented Jun 21, 2018

Copy link
Copy Markdown
Member

This is based on @NicolasDorier's excellent proposal for a Dockerfile in PR #1318, sans the
writing of a config file.

@cdecker cdecker added this to the v0.6 milestone Jun 21, 2018
@cdecker cdecker self-assigned this Jun 21, 2018
@cdecker
cdecker requested a review from rustyrussell June 21, 2018 16:20
@cdecker cdecker added the docker label Jun 21, 2018
Comment thread README.md Outdated
cdecker/lightningd:latest
It has the following environment variable:

* `EXPOSE_TCP` default to false, if true, use expose c-lightning on port 9835. (Use this only for testing)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean 9735?

This is based on @NicolasDorier's excellent proposal for a Dockerfile, sans the
writing of a config file.

Co-authored-by: Nicolas Dorier <nicolas.dorier@gmail.com>
Co-authored-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>

@rustyrussell rustyrussell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack 1442d03

@rustyrussell
rustyrussell merged commit 0d3c3a2 into ElementsProject:master Jun 21, 2018
Comment thread Dockerfile
COPY . .

ARG DEVELOPER=0
RUN ./configure && make -j3 DEVELOPER=${DEVELOPER} && cp lightningd/lightning* cli/lightning-cli /usr/bin/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEVELOPER=${DEVELOPER} is needed ? I thought with ./configure it should be ok.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's backward compatibility in case someone would set environment variable and then expect things to work.

Comment thread Dockerfile
rm -rf glibc.apk glibc-bin.apk /var/cache/apk/*

ENV LIGHTNINGD_DATA=/root/.lightning
ENV LIGHTNINGD_PORT=9735

@NicolasDorier NicolasDorier Jun 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually uneeded and can lead to confusion because actual port depends on the parameters passed to docker run.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: It is actually used for socat.

@shesek shesek Jun 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still somewhat confusing, maybe rename it to something like RPC_TCP_PORT to make it distinct from the P2P network port?

edit: especially confusing with the default value being the p2p port...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry, my bad for mixing those two up :-(

Comment thread Dockerfile
&& tar -xzvf litecoin.tar.gz $BD/litecoin-cli --strip-components=1 --exclude=*-qt \
&& rm litecoin.tar.gz

ENV LIGHTNINGD_VERSION=master

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LIGHTNINGD_VERSION is not used for anything

@shesek

shesek commented Jun 22, 2018

Copy link
Copy Markdown
Contributor

Should the entry point also start bitcoind if an external node isn't configured? Why are the bitcoin binaries included if not?

@NicolasDorier

Copy link
Copy Markdown
Contributor

@shesek because clightning depends on bitcoin-cli.

@shesek

shesek commented Jun 23, 2018

Copy link
Copy Markdown
Contributor

Ah, I see, it's just the -cli binaries that are there, not that daemon itself. Would it make sense to bundle the daemon also?

@NicolasDorier

Copy link
Copy Markdown
Contributor

@shesek I don't think so. It makes the image two time bigger, also running two processes in one image is not recommended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants