-
-
Notifications
You must be signed in to change notification settings - Fork 114
Docker: improve docker compose structure #722
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
base: main
Are you sure you want to change the base?
Conversation
Previously, each container would be handed all the env vars int the .env file. This is bad practice for a variety of reasons. This commit changes it so the docker compose file specified the env file for each container. the benefits: - less env vars to be set by the end user - containers are only given vars relevant to them - when running docker compose up, only containers affected by changed vars are restarted
files starting with a `.` chatacter are "hidden files" in UNIX systems. some less knowledgable users may get confused by not seeing a file they expect. changing the example env file to not be a hidden file, making it easier to discover for a user
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
note: unsure about the "required" field, as any required vars are now defined in the docker compose file
I would like to note that this comes with the benefit of reducing the amount of people asking for help, as potential mistakes are reduced. The only issue is what to do with the "required" field. Should it be left, changed, or removed completely? |
Note: this currently fails at compile time
Thoughts on #693 ? |
@UndyingSoul I am not 100% sure as I have never seen such a docker compose file.
I think that (In terms of docker) the commit introduces additional complexity, reducing only one issue, and probably introducing a bunch of confusion. It looks like it is deprecating the |
You bring up some excellent points. Thanks for the feedback. My PR does not address your first point in any way. My personal preference is to expose everything that can be configured to the user. While I agree that having the frontend/backend URLs exposed to the user can easily lead to problems, I do think there is value in having as much documentation in the docker-compose.yml, which includes everything that can be configured. The second point you raised is a great one, yml anchor and alias notation can be kind of difficult to understand if you're not used to seeing it. The reason I choose anchors and aliases over just putting the environment variables directly under each service is so I could reuse the common environment variables between the backend and DB. I also think it's great idea to have all the main configuration in one spot, similar to an .env file. Which brings me to your last point, I took a closer look at this PR and it looks like we're doing very similar things, but our implementation is different. I chose to implement the secret file handling in the entrypoint.sh, which, in my opinion, it was the minimally invasive option. Which is better? I'm not sure. Then, I wanted to address the depreciation of the .env file. The way docker handles .env files is very confusing, and it's tough to find the source of truth. With both of our implementations, any .env file in the same directory as the docker-compose.yml, will override any overlapping environment variables defined in the docker-compose.yml file. In a similar way, if you Thanks again for bringing up those important topics, if you think I should document this more, whether it in the AdventureLog docs or on either of our PRs, let me know. Edit: Fixed some grammatical/spelling errors. |
Note: as we are discussing only docker-compose and environment vars from here on out, reference changes up to and including 537790c only. I will do a revert of the last commit when I have a moment on my private system. Right, I see what you're saying, but I don't agree.
Let's take the I am a big fan of the GNOME HIG Design Principles. While this is mainly focused on application design, It can be applied in other places too. Here I would like to point out two points under the "Reduce User Effort" section:
I could similarly quote KDE's motto, encoded into the KDE HIG. "Simple by default, powerful when needed". With this commit, I am removing complexity by default, while still allowing more complex changes to be made. Again, I would like to point out that nothing is stopping a user to make a complete mess by changing the behavior in the docker compose.
If you look at my
I don't think I need to add anything to this, it speaks for itself.
All right, i have no issue with this. When I made my first comment, I had no time originally to look if you handled file secrets. If your implementation is working, that's enough. 👍🏻
What? I do not agree at all. Neither on confusing or tough to find the source of truth. My original issue with the project's docker-compose, is that it simply passed everything inside the I am no stranger to docker compose, and every single project uses either a
This remains true with the changes I proposed. An existing deployment would remain entirely unaffected. |
I realized I had some incorrect assumptions about how Docker Compose handles the Test Setup
services:
test:
image: docker.io/ubuntu:latest
environment:
SOME_USER: ${SOME_USER:-dockercomposeuser}
SOME_PASSWORD: dockercomposepassword
SOME_EMAIL: ${SOME_EMAIL}
SOME_USER_COMBO: ${SOME_USER}:${SOME_PASSWORD}
SOME_SOLUTION: ${SOME_SOLUTION:-dockercomposesolution}
entrypoint: ["bash", "-c", "printenv"]
SOME_USER=dockerenvuser
SOME_PASSWORD=dockerenvpassword
SOME_EMAIL=envemail Expected OutputOne might expect SOME_USER=dockerenvuser
SOME_PASSWORD=dockerenvpassword
SOME_EMAIL=envemail
SOME_USER_COMBO=dockerenvuser:dockerenvpassword
SOME_SOLUTION=dockercomposesolution Actual OutputUsing Podman Compose: $ podman compose up
SOME_USER=dockerenvuser
SOME_PASSWORD=dockercomposepassword
SOME_EMAIL=envemail
SOME_USER_COMBO=dockerenvuser:dockerenvpassword
SOME_SOLUTION=${SOME_SOLUTION:-dockercomposesolution} Using Docker Compose: $ docker compose up
SOME_USER=dockerenvuser
SOME_PASSWORD=dockercomposepassword
SOME_EMAIL=envemail
SOME_USER_COMBO=dockerenvuser:dockerenvpassword
SOME_SOLUTION=dockercomposesolution Discussion
Additional NotesAfter some digging, I found this issue is specific to older versions of Podman Compose. I was using v1.3.0, which has a known bug related to variable substitution containers/podman-compose#1105. After updating to v1.5.0, it now behaves as your PR intends. Overall, I support the approach you've taken in your PR—default values in Curious to hear what others think too—it’s just the two of us so far. |
Firstly, I want to apologise if I sounded defensive or otherwise unpleasant during this interaction, that is in no way my intention. I appreciate that you are working on this project, and I admire regular FOSS contributors. I find that when communicating over text, everything sounds more assertive or aggressive. However, I truly believe the particular issue of our discussion is an open and shut case. I want to go over one more example to hopefully clarify my stance.
I think that this perspective is fair, as there is an install script in case you can't run |
About the podman issue. These are two similar yet very distinct projects. I definetly support people making sure tools can be used by either docker or podman. |
update so that the docker compose file specifies the env vars for each container.
the benefits:
additionally, rename .env.example to example.env so that it is not a hidden file.