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

Docker from Debian base image #3500

Merged
merged 3 commits into from
Sep 13, 2023
Merged

Conversation

Alkarex
Copy link
Contributor

@Alkarex Alkarex commented Jul 6, 2023

  • Fix expose Docker port 9000? #3234
  • Re-fix better logs Use standard Docker logs #3333
  • Update to Debian 12 Bookworm instead of Debian 10 Buster
  • Use Debian packaging instead of having to keep track of and manually install -dev libraries, and with LTS support
  • Update to PHP 8.2 instead of PHP 8.0
  • Divided by two the size of the Docker image (from 485MB down to 202MB)

* Fix expose RSS-Bridge#3234
* Re-fix better logs RSS-Bridge#3333
* Update to Debian 12 Bookworm instead of Debian 10 Buster
* Use Debian packaging instead of having to keep track of and manually install -dev libraries, and with LTS support
* Update to PHP 8.2 instead of PHP 8.0
@Alkarex Alkarex mentioned this pull request Jul 6, 2023
Comment on lines +5 to +6
access_log /var/log/nginx/access.log;
error_log /var/log/nginx/error.log;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using symlink to stdout / stderr instead, cf. Dockerfile

COPY ./config/php-fpm.conf /etc/php/8.2/fpm/pool.d/rss-bridge.conf
COPY ./config/php.ini /etc/php/8.2/cli/conf.d/90-rss-bridge.conf

# logs should go to stdout / stderr
Copy link
Contributor Author

@Alkarex Alkarex Jul 6, 2023

Choose a reason for hiding this comment

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

@@ -13,6 +13,6 @@ server {

location ~ \.php$ {
include snippets/fastcgi-php.conf;
fastcgi_pass 127.0.0.1:9000;
fastcgi_pass unix:/var/run/php/php8.2-fpm.sock;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using socket instead of TCP (should have less overhead)

Dockerfile Outdated Show resolved Hide resolved
# logs should go to stdout / stderr
RUN ln -sfT /dev/stderr /var/log/nginx/error.log; \
ln -sfT /dev/stdout /var/log/nginx/access.log; \
chown -R --no-dereference www-data:adm /var/log/nginx/
Copy link
Contributor

Choose a reason for hiding this comment

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

does nginx run as www-data user? i guess so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are the same rights than what nginx sets by default on the logs

@dvikan
Copy link
Contributor

dvikan commented Jul 6, 2023

  1. i like this pr

  2. this is a bump from php 8.0 to php 8.2. might break some older code

  3. does this mesh well with curl-impersonator? @wrobelda

@Alkarex
Copy link
Contributor Author

Alkarex commented Jul 6, 2023

This PR could be broken down if any part is not appropriate for now

@dvikan
Copy link
Contributor

dvikan commented Jul 6, 2023

i tested the pr and all is good. there were some additional deprecation notices etc. but unable to test all bridges so cannt tell if any of them errors.

can we preserve the usage of php 8.0.x ? dont want accidental breakage when users upgrade their docker containers. otherwise all is good

@Alkarex
Copy link
Contributor Author

Alkarex commented Jul 6, 2023

Using the suggested Debian base image, supporting PHP 8.0 would not be convenient: Debian has long term support for PHP 7.4 and PHP 8.2, nothing in between:
https://packages.debian.org/bookworm/php
Moving to the Debian base image indeed offers much longer LTS options, but with fewer version choices.

With the base image used before this PR, it would be possible to support PHP 8.0 for 4 more months (until November 2023):
https://www.php.net/supported-versions.php

Note: the base image used before this PR was with a specific version 8.0.27, so it was not automatically updated to currently supported releases (4 months since last update), but it would be possible to update to 8.0-fpm-bullseye

php                 8.0.27-fpm-buster  18d81894df3e   4 months ago 390MB

An Ubuntu base image could also be an option, as it has more frequent releases than Debian (though more "corporate" than Debian), but only down to PHP 8.1, not PHP 8.0, using Ubuntu 22.04 LTS
https://packages.ubuntu.com/jammy/php

Otherwise, Alpine Linux could also be an option (but less known than Debian for developers, and with a relatively different configuration) with support for PHP 8.0 until May 2024
https://pkgs.alpinelinux.org/package/v3.16/community/x86_64/php8
https://alpinelinux.org/releases/

For info, the new approach also decreases the size of the Docker image by over a factor two:

rssbridge/rss-bridge:debian12  latest  e9b7a56c1d63   3 hours ago  202MB
rssbridge/rss-bridge           latest  7c58e48a220d  16 hours ago  485MB

@dvikan
Copy link
Contributor

dvikan commented Jul 6, 2023

i have done research.

there is not much BC break from 8.0 to 8.2.

i think it's okay.

lets wait for input from others

To optimise caching
@Alkarex
Copy link
Contributor Author

Alkarex commented Jul 7, 2023

FYI, I have also made this related PR to also update to Debian 12 Bookworm: lwthiker/curl-impersonate#168

@wrobelda
Copy link
Contributor

  • does this mesh well with curl-impersonator? @wrobelda

Looks good to me, but indeed needs curl-impersonate to also bump their base image, which @Alkarex already handled with his (pending) PR.

@dvikan
Copy link
Contributor

dvikan commented Aug 22, 2023

@Alkarex @wrobelda anything new here

@dvikan
Copy link
Contributor

dvikan commented Sep 12, 2023

@Alkarex @wrobelda anything new here

@Alkarex
Copy link
Contributor Author

Alkarex commented Sep 13, 2023

@dvikan As far as I can see, everything is working here, including the current curl-impersonate (maybe testing a few bridges depending on the curl-impersonate features would be good, though).
So although it would be nice to see lwthiker/curl-impersonate#168 merged, I do not believe there is a need to wait for it.

@dvikan dvikan merged commit 0175e13 into RSS-Bridge:master Sep 13, 2023
@Alkarex Alkarex deleted the docker-from-debian branch September 13, 2023 19:31
@Alkarex
Copy link
Contributor Author

Alkarex commented Sep 22, 2023

Just for information, lwthiker/curl-impersonate#168 has been merged, though no new release yet.
I have tried a manual build of curl-impersonate (Firefox version based on Debian 12 Bookworm) with the latest Dockerfile of RSS-Bridge, and that seems to work fine.

@dvikan
Copy link
Contributor

dvikan commented Sep 22, 2023

thanks for keeping an eye on this

Alkarex added a commit to Alkarex/rss-bridge that referenced this pull request Jan 4, 2024
Mistake from RSS-Bridge#3500
Wrong file extension: should have been `.ini` and not `.conf` otherwise it has no effect.
See docker-library/php#1360 and docker-library/php#878 (comment)
ENV CURL_IMPERSONATE ff91esr

COPY ./config/nginx.conf /etc/nginx/sites-available/default
COPY ./config/php-fpm.conf /etc/php/8.2/fpm/pool.d/rss-bridge.conf
COPY ./config/php.ini /etc/php/8.2/fpm/conf.d/90-rss-bridge.conf
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in #3875

dvikan pushed a commit that referenced this pull request Jan 5, 2024
Mistake from #3500
Wrong file extension: should have been `.ini` and not `.conf` otherwise it has no effect.
See docker-library/php#1360 and docker-library/php#878 (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.

3 participants