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

Fix #845 and dockerfile cleanup #848

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

defnull
Copy link
Contributor

@defnull defnull commented Nov 28, 2022

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:

  • 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.

@defnull defnull changed the title Fix #845 Nginx - Connection refused (again) Fix #845 Nginx - Connection refused (WIP) Nov 28, 2022
@defnull
Copy link
Contributor Author

defnull commented Nov 28, 2022

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.
@sonarcloud
Copy link

sonarcloud bot commented Nov 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@defnull
Copy link
Contributor Author

defnull commented Nov 28, 2022

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.

@defnull defnull changed the title Fix #845 Nginx - Connection refused (WIP) Fix #845 and dockerfile cleanup Nov 28, 2022
@jfederico
Copy link
Member

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

@defnull
Copy link
Contributor Author

defnull commented Nov 29, 2022

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.

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