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 provided #9391

Open
wants to merge 2 commits 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.

- --zmq-pub=tcp://0.0.0.0:18084
- --zmq-rpc-bind-port=1882
- --zmq-rpc-bind-ip=0.0.0.0
- --log-level=4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why --rpc-ssl=disabled? Also --log-level=4 is too high, it will cause performance issues due to massive amount of logs written.

Copy link
Author

Choose a reason for hiding this comment

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

It's just a template for development and everyone could modify them by their needs. I think Server-side services communicate under the Docker network by service name in most cases and SSL isn't required between them.
However, it seems it's better to change them let me remove these and add some comments from "monerod --help" to it.

…d --rpc-sssl=disable to default for security (based on PR comments)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants