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

Add Dockerfile #461

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

Add Dockerfile #461

wants to merge 4 commits into from

Conversation

timqi
Copy link

@timqi timqi commented Apr 24, 2024

No description provided.

.dockerignore Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Copy link
Collaborator

@biryukovmaxim biryukovmaxim left a comment

Choose a reason for hiding this comment

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

LGTM!

Dockerfile Outdated Show resolved Hide resolved
Copy link
Collaborator

@coderofstuff coderofstuff left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.

Content looks alright to me. I have some minor comments:

  • Please add a section in the README.md, maybe under the Installation section as Building on Docker.
  • The Dockerfile doesn't have an ENTRYPOINT or CMD (which is fine), so the documentation should also note examples on how to use the two binaries being built inside this Dockerfile

@supertypo
Copy link
Contributor

supertypo commented May 1, 2024

README.md: I think rusty-kaspa is a better tag

A few suggestions on changes to the Dockerfile based on Docker best practices:

  • A VOLUME should be declared for the data folder (~/.rust-kaspa)
  • EXPOSE could be used to declare the exposed ports (e.g. 16111, 16110, 17110, 18110 for mainnet)
  • ENTRYPOINT could be kaspad. Since you bundle two executables, you might opt to leave ENTRYPOINT empty and specify the executable in CMD instead.
  • CMD should be common sense default arguments to kaspad in array format (e.g. ["--yes", "--nologfiles", "--disable-upnp", "--utxoindex", "--rpclisten=0.0.0.0:16110", "--rpclisten-borsh=0.0.0.0:17110", "--rpclisten-json=0.0.0.0:18110"])
    Docker containers runs on an internal network by default, upnp is not relevant and rpc ports must be exposed to all interfaces for it to be reachable using Docker port forwarding. The listen port (16111) is already public by default.
  • A USER could ideally be declared to run with non-root permissions. It will complicate things a bit though as the user must be created beforehand. If a custom host folder is to be mounted in as the datafolder the folder must also be chowned to 16111 on the host. In case this is added, info on how to handle host mounts must be added to the README.md

EDIT: Revised the comment about ENTRYPOINT

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.

None yet

5 participants