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

Refactor Docker setup #50

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

Conversation

rvanasperen
Copy link

@rvanasperen rvanasperen commented Nov 5, 2024

This PR refactors the (in my respectful opinion) shoddy Docker setup to something more practical. There's an easy Dockerless setup for people to use already. If people do want to use Docker, one can assume those people have some technical know-how already.

The old setup creates a huge Docker image by copying everything over, convoluted by adding a redundant dependency on make into the mix.

The new setup uses Docker Compose (provided by Docker out-of-the-box nowadays) using a lightweight Nginx container instead, using volume mounting, and not reliant on make.

@alex-taxiera
Copy link

I like this a lot, but a few things:

  1. Remove the "fixes" for local art, it was not the correct way to fix the issue.
  2. I think you should undo the formatting changes to the README.
  3. I would add docker-compose.override.yml to the .gitignore, I would use this personally.

This compose file is working great though, no more wasted time copying files :)

@rvanasperen rvanasperen changed the title Refactor Docker, fix Docker local_art, update README Refactor Docker setup Dec 21, 2024
@rvanasperen
Copy link
Author

rvanasperen commented Dec 21, 2024

@alex-taxiera Good point. I reverted the changes to the readme and the local art fix I made.

Do you know what the correct fix is for the local art? Is that already present in the main branch? I see some PRs open for it, but I'm unaware of what the correct fix would be.

@alex-taxiera
Copy link

Do you know what the correct fix is for the local art? Is that already present in the main branch? I see some PRs open for it, but I'm unaware of what the correct fix would be.

I opened #52 which I believe is the "best" fix based on this comment

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