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

Improve Docker setup #655

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

Conversation

n-thumann
Copy link

This PR introduces some improvements regarding the Docker setup.

I tried to run reNgine in a newly created VM and noticed a few things that would allow a user to get reNgine up and running much quicker, which are:

  • Drop docker-compose (v1): Since Docker Engine 20.10.13 the new Docker Compose v2 CLI is available by default (see https://docs.docker.com/engine/release-notes/#201013). It's was available in Docker Desktop much earlier and is default since 4.8.0. Hence there's no need for manually installing docker-compose (v1). docker-compose-plugin is also installed automatically by get.docker.com.
  • Use Docker images from Docker Hub: Since the Docker images on GitHub (see https://github.com/yogeshojha/rengine/pkgs/container/rengine%2Frengine) hasn't been updated, but the ones on Docker Hub have (see https://hub.docker.com/r/yogeshojha/rengine/tags), we should use them
  • Don't enforce building images: Because we're can use the pre-built images from Docker Hub, we shouldn't force a user to build them locally. Explicitly building with make build is still possible of course.
  • General cleanups of Dockerfiles:
    • Remove unnecessary explicit context in build configuration
    • Remove duplicate networks specification
    • Remove publicly exposed ports of internal services in production environment
    • Always restart services

I am running these changes on my local machine without any issues :)

@AnonymousWP
Copy link
Contributor

Opinions? Not sure if it's still relevant. Some parts are, some aren't.

@n-thumann
Copy link
Author

Feel free to take this PR over :)

@psyray
Copy link
Contributor

psyray commented Nov 22, 2023

Need testing

@psyray psyray added the testing label Nov 22, 2023
@AnonymousWP
Copy link
Contributor

@psyray I'll test this after I heard your opinion about it. In my opinion, it's a good change, because it modernises the Docker process/workflow.

@n-thumann Could you please solve the conflicts and modernise it to today's date? Since your PR is from 2022. After that, I'll be happy to test this, and merge this into our project if the other maintainers agree as well. Thanks!

@psyray
Copy link
Contributor

psyray commented Nov 23, 2023

@psyray I'll test this after I heard your opinion about it. In my opinion, it's a good change, because it modernises the Docker process/workflow.

I f you think it's good, it's good for me

@n-thumann n-thumann force-pushed the improve_docker_setup branch from c85e704 to 018323c Compare November 23, 2023 22:35
@n-thumann
Copy link
Author

@n-thumann Could you please solve the conflicts and modernise it to today's date?

Please note that I haven't used reNgine for a long time and won't be able to test the changes now or implement any additional changes. I did a quick rebase and resolved the conflicts though.

For any additional change requests, don't hesitate to take over this PR and implement them yourself :)

Copy link
Contributor

@AnonymousWP AnonymousWP left a comment

Choose a reason for hiding this comment

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

Just a small nitpicks and a comment. I'll test this later, thanks!

install.sh Show resolved Hide resolved
:: Show this help.
if "%1" == "help" @echo Make application docker images and manage containers using docker-compose files only for windows.
if "%1" == "help" @echo Make application docker images and manage containers using docker compose files only for windows.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if "%1" == "help" @echo Make application docker images and manage containers using docker compose files only for windows.
if "%1" == "help" @echo Make application Docker images and manage containers using Docker Compose files only for Windows.

Small nit.


prune: ## Remove containers and delete volume data.
@make stop && make rm && docker volume prune -f

help: ## Show this help.
@echo "Make application docker images and manage containers using docker-compose files."
@echo "Make application docker images and manage containers using docker compose files."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@echo "Make application docker images and manage containers using docker compose files."
@echo "Make application Docker images and manage containers using Docker Compose files."

@AkechiShiro
Copy link

@AnonymousWP is there any tests that was done since ? Maybe a next step would be to use docker compose v3 or draft a PR for it, apparently it's not too hard to port v2 to v3 from what I read from here : https://vsupalov.com/upgrade-docker-compose-file-version/

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

Successfully merging this pull request may close these issues.

5 participants