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.

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

ref(docker): create a standalone docker artifact to run lightwalletd #476

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gustavovalverde
Copy link

@gustavovalverde gustavovalverde commented Feb 26, 2024

Please ensure this checklist is followed for any pull requests for this repo. This checklist must be checked by both the PR creator and by anyone who reviews the PR.

  • Relevant documentation for this PR has to be completed before the PR can be merged
  • A test plan for the PR must be documented in the PR notes and included in the test plan for the next regular release

As a note, all CI tests need to be passing and all appropriate code reviews need to be done before this PR can be merged


Motivation

This PR is meant to align lightwalletd docker execution to what we previously had working in ZcashFoundation/zebra#8323 (but we no longer need in our repo). Allowing users to use an standalone lightwalletd docker image without further configurations, allowing users to just do:

docker run --detach -it -p 9067:9067 -p 9068:9068 electriccoinco/lightwalletd

And have a working lightwalletd server out-of-the-box (at least for testing/development purposes)

Changes

  • Upgrade to golang:1.19 as 1002f42 made this a requirement, considering some dependencies no longer work with golang:1.17
  • Use go instead of make to build inside the image (this is the expected approach with Docker)
  • Remove instructions with docker_img_run_zcashd as we don't have zcashd nor zcash-cli` inside the container
  • Add a .dockerignore file to just have the required file inside the container
  • Use a multistage Dockerfile to have a resulting image with the bare minimum: lightwalletd, and with a considerably reduced image size
  • Set some variables as ARGs, so people can change their values if they'd like to build the image locally.
  • Create a default zcash.conf to use this image as a standalone artifact without further configurations
  • Use the available cert.key and cert.pem from the repo to avoid running it with --no-tls-very-insecure by default

Testing

You can test this using:

docker build --target runtime --tag local/lightwalletd .
docker run -p 9067:9067 -p 9068:9068  local/lightwalletd

If you'd like to test this running alongside Zebra, you can test with:

docker-compose -f docker-compose.zebra.yml up --build -d

@gustavovalverde gustavovalverde changed the title ref(docker): create a standalone Dockerfile to run lightwalletd ref(docker): create a standalone docker artifact to run lightwalletd Feb 26, 2024
@gustavovalverde
Copy link
Author

Also, as this is now using a multistage Dockerfile, this can be easily extended to add a tests stage in it, to run with CI

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK with suggestion.

@gustavovalverde
Copy link
Author

Thank you @daira! I actually ended up removing the whole section with comments, as this was recently removed anyways 38b751e

@gustavovalverde
Copy link
Author

I'd like to follow-up with some extra changes on top of this, @daira @LarryRuane are you aware if there's something missing here to get it merged?

@LarryRuane
Copy link
Collaborator

I don't understand how to run this (to test it). I'm sure part of the problem is that I'm unfamiliar with docker. For example, am I expected to start a zcashd instance manually, or does this happen within docker? If I start it manually, should I give any non-default command line (or zcash.conf) options? Should I be using the make targets, and if so, which ones? Can you document the exact steps need to run this? Actually, I think updating docs/docker-run.md might be a good place to do this, so people don't have to refer to this PR.

Also I see that this PR needs to be rebased onto the latest master branch. (I did that here locally, it wasn't difficult, but if you'd like I can push my branch to GitHub.)

@mpguerra
Copy link

I see this has been approved now, thank you! Is there anything in particular that is preventing this PR from merging?

@gustavovalverde
Copy link
Author

@LarryRuane I'll be adding the process and documentation so it's clearer and easier to test this changes.

@LarryRuane
Copy link
Collaborator

@gustavovalverde

Is there anything in particular that is preventing this PR from merging?

No, we can merge it, but I just wanted to do some testing myself as an extra safety margin. But since this PR makes only docker-related changes, it's safe for anyone not using docker.

Now that @gustavovalverde will be adding some documentation, we could wait for this and have me test it. Or would you rather have me merge it right away? I'm okay with either plan, I don't want to hold anyone up.

@gustavovalverde
Copy link
Author

Let's wait for the extra documentation to be on the safe side.

This also includes documentation on how to run it
@gustavovalverde
Copy link
Author

Hi @LarryRuane!

I've added extra testing steps into the PR body, and also in the documentation.

I added an extra docker-compose file to keep the changes as minimum as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants