-
Notifications
You must be signed in to change notification settings - Fork 246
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
Fix #845 and dockerfile cleanup #848
base: master
Are you sure you want to change the base?
Conversation
There seem to be more bugs in the docker images. WIP |
There were more issues with the alpine nginx images than I first realised. This patch: - Moves nginx/conf.d to nginx/http.d because that is how the directory is now called within images - Fixes the nginx/start script to work with /etc/nginx/http.d instead of the deprecated and missing /etc/nginx/conf.d - Fixes alpine-based Dockerfiles to copy config files from/to the correct locations - Fixes amazonlinux-based Dockerfiles to also store config in /etc/nginx/http.d so nginx/start works as expeted - Harmonized files in dockerfiles/v1/. Lots of changes in the past were not ported to all of these files. They should only differ in the base image used (alpine/amazonlinux) and the bbb-playback target. Base image versions or other details should be the same.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There were a lot more issues with the docker images and the commit got kinda big. Again, please review and test these changes. Try to build the images. Try to start them. I tested the changes, but only very roughly. Some of the older images were so outdated that they did not build at all. If these are no longer maintained, the Dockerfiles should probably better be removed. I tried to fix the mess, but I may have very well overlooked some details. |
Thanks for the PR once again @defnull it is a good base for a deeper review later on. The issue is the start bash script in the custom nginx image. The undelyining issue though is the custom nginx image itself (which I never liked and I never will, but it is still there). Also updates to xenial images are not necessary, they are not officially supported anyways. But since you updated all the images (including amazonlinux), a massive test needs to be performed. I would say, for the moment, we should better stick with the monkey-patch and give a quick solution to those still stuck with the systemd deployment and/or using the outdated docker-compose deployment (based on the custom nginx image). The idea is (and has been all the way) to come up with one (and only one) deployment, and keep only two base docker images (one based on alpine and the other on amazonlinx). But no more nginx, poller, recording-importer. (So many potential points of failure). And I am serious when I say that we should talk. Cheers |
I understand that this PR is probably to involved to be merged. I got carried away ;) Maybe just remove the outdated Dockerfiles then and wait for 2.0 for a major cleanup. |
The last fix for #845 was incomplete, the startup script still references files from the deprecated /etc/nginx/conf.d/ directory. Fixing the start script however breaks alpine-linux images. It's a mess.
While looking into that, I noticed that there were a lot more more issues with the docker images than I first realized. This patch: