Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tweak docker entrypoint to support regtest, testnet #3813

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Tweak docker entrypoint to support regtest, testnet #3813

merged 1 commit into from
Jul 7, 2020

Conversation

joemphilips
Copy link
Contributor

@joemphilips joemphilips commented Jul 5, 2020 •
edited
Loading

Before this, docker image will never detects that
lightning-rpc was created if it is running in regtest
or testnet, because the file will be created under
subfolder for each network name.
By recursively checking the file creation, now docker
can be run in any network configuration.
I had to deal the path for argument to the socat , so I added a new ENV entry in Dockerfile to detect the correct network.

@joemphilips joemphilips force-pushed the enable_to_run_docker_in_testnet branch from 879830a to f591958 Compare July 5, 2020 15:13
@joemphilips joemphilips changed the title Add -r option to ionotifywait in docker entrypoint Tweak docker entrypoint to support regtest, testnet Jul 5, 2020
ZmnSCPxj
ZmnSCPxj requested changes Jul 6, 2020
View reviewed changes
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

shellcheck has a couple of complaints. Though one of them is not on code you modified, huh. https://travis-ci.org/github/ElementsProject/lightning/jobs/705137711#L967-L977

tools/docker-entrypoint.sh Outdated Show resolved Hide resolved
tools/docker-entrypoint.sh Outdated Show resolved Hide resolved
@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Jul 6, 2020

Also commit title should probably change as well.

@joemphilips joemphilips force-pushed the enable_to_run_docker_in_testnet branch 2 times, most recently from 3a039cf to 2cb94e2 Compare July 6, 2020 02:42
@joemphilips
Copy link
Contributor Author

Updated according to the review. I also dropped -r option indocker-entrypoint.sh since it is unnecessary after I introduced LIGHTNING_NETWORK env variable.

ZmnSCPxj
ZmnSCPxj reviewed Jul 6, 2020
View reviewed changes
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

Should not commit message say "take LIGHTNING_NETWORK env variable in Dockerfile"?

@joemphilips joemphilips force-pushed the enable_to_run_docker_in_testnet branch from 2cb94e2 to 37887d1 Compare July 6, 2020 02:54
@joemphilips
Copy link
Contributor Author

Should not commit message say "take LIGHTNING_NETWORK env variable in Dockerfile"?

Right, I was careless. updated.

ZmnSCPxj
ZmnSCPxj approved these changes Jul 6, 2020
View reviewed changes
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

ACK 37887d1

@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Jul 6, 2020

Also probably deserves a Changelog entry in the commit message:

Changelog-Added: Docker build now includes a `LIGHTNINGD_NETWORK` variable you can override to `regtest` or `testnet` or any valid argument to `--network`.

Though wording might be changed depending on how exactly Docker terminology is, I am not a Docker expert.

@joemphilips joemphilips force-pushed the enable_to_run_docker_in_testnet branch from 37887d1 to ad0ab73 Compare July 6, 2020 03:16
@joemphilips
Copy link
Contributor Author

Changelog added. I couldn't find any past changelog which uses newline. So I avoided using it.

ZmnSCPxj
ZmnSCPxj approved these changes Jul 6, 2020
View reviewed changes
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

ACK ad0ab73

@ZmnSCPxj ZmnSCPxj force-pushed the enable_to_run_docker_in_testnet branch from ad0ab73 to 5092f33 Compare July 6, 2020 06:10
@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Jul 6, 2020

Rebased on master. Reapplying ACK:

ACK 5092f33

@cdecker
Copy link
Member

cdecker commented Jul 6, 2020 •
edited
Loading

Will merge as soon as Travis is happy 👍

ACK 5092f33

See @ZmnSCPxj's feedback below.

ZmnSCPxj
ZmnSCPxj requested changes Jul 6, 2020
View reviewed changes
tools/docker-entrypoint.sh Outdated Show resolved Hide resolved
@joemphilips
Take LIGHTNINGD_NETWORK env variable in Dockerfile.
5debe5b 
Before this, docker image will never detects that
`lightning-rpc` was created if it is running in regtest
or testnet, because the file will be created under
subfolder for each network name, and entrypoint does not
check "lightning-rpc" file in those folders.
By specifying `LIGHTNINGD_NETWORK` environment var
in dockerfile, we can now check correct path.

Changelog-Added: Docker build now includes `LIGHTNINGD_NETWORK` ENV variable which defaults to "bitcoin". An user can override this (e.g. by `-e` option in `docker run`) to run docker container in regtest or testnet or any valid argument to `--network`.
@joemphilips joemphilips force-pushed the enable_to_run_docker_in_testnet branch from 5092f33 to 5debe5b Compare July 7, 2020 05:40
ZmnSCPxj
ZmnSCPxj approved these changes Jul 7, 2020
View reviewed changes
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

ACK 5debe5b

@cdecker cdecker merged commit 0b1f8fd into ElementsProject:master Jul 7, 2020
@joemphilips joemphilips mentioned this pull request Jul 10, 2020
Closed
@azuchi azuchi mentioned this pull request Oct 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZmnSCPxj ZmnSCPxj ZmnSCPxj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joemphilips @ZmnSCPxj @cdecker