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

Dockerfile: remove CMD line and docker-compose file is added #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mjhatami
Copy link

@mjhatami mjhatami commented Jul 3, 2024

Improved Containerization with Docker Compose

Summary

This pull request updates the containerization approach for the repository by changing the Dockerfile and adding a docker-compose.yml file. This change aims to improve build times and provide greater flexibility when adjusting command-line arguments, enhancing the development workflow.

Changes Made

1. Dockerfile Update:

  • Removed the CMD instruction from the Dockerfile.

2. Introduction of Docker Compose:

  • Added a docker-compose.yml file to facilitate easier management and configuration of the container.
  • The command previously specified in the Dockerfile's CMD is now included in the command section of the Docker Compose file.
  • This allows for quicker adjustments to command-line arguments without needing to rebuild the Docker image, streamlining the development process.

Benefits

  • Faster Development: By removing the CMD line from the Dockerfile and leveraging Docker Compose, developers can adjust the command-line arguments without rebuilding the entire image, saving significant time during the development cycle.
  • Enhanced Flexibility: The new setup allows for easy addition or removal of command-line arguments through the docker-compose.yml file, providing a more flexible and convenient approach to configuration changes.

Please review the changes and let me know if any further adjustments are needed. Thank you for considering this improvement to the project's containerization process.

Copy link
Owner

@vtnerd vtnerd left a comment

Choose a reason for hiding this comment

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

This really should be PR'ed against the develop branch. I can port back to develop branch if you don't want to open a new PR.

command:
- --db-path=/home/monero-lws/.bitmonero/light_wallet_server
- --daemon=tcp://monero:1882
- --sub=tcp://monero:18084
Copy link
Owner

Choose a reason for hiding this comment

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

--sub and --zmq-pub need to be on different ports. --zmq-pub needs to match the zmq-pub port of monerod and --sub can be any available port.

- --daemon=tcp://monero:1882
- --sub=tcp://monero:18084
- --zmq-pub=tcp://monero:18084
- --log-level=4
Copy link
Owner

Choose a reason for hiding this comment

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

I think the default log-level should be 0, as 4 spams lots of data to disk.

- --log-level=4
- --webhook-ssl-verification=none
- --disable-admin-auth
- --admin-rest-server=http://0.0.0.0:8443/admin
Copy link
Owner

Choose a reason for hiding this comment

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

The default should be https or port 8080. I would advocate for https, but transient certificates are a kludge.

- --webhook-ssl-verification=none
- --disable-admin-auth
- --admin-rest-server=http://0.0.0.0:8443/admin
- --rest-server=http://0.0.0.0:8443/basic
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

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.

2 participants