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: switch to nginx in docker image #2721

Merged
merged 3 commits into from
May 12, 2022

Conversation

dvikan
Copy link
Contributor

@dvikan dvikan commented May 11, 2022

TLDR: nginx is better.

Also I (we?) are more familiar with nginx so it becomes easier to introduce a new document root while not breaking BC.

Removed some TLS cruft. They are not used.

@yamanq
Copy link
Contributor

yamanq commented May 11, 2022

👍 I tried it and it works I think.
Can you combine the RUN statements?

@yamanq
Copy link
Contributor

yamanq commented May 12, 2022

Suggestion: && mv

@yamanq yamanq merged commit 829fc6c into RSS-Bridge:master May 12, 2022
Alkarex added a commit to Alkarex/rss-bridge that referenced this pull request May 30, 2022
RSS-Bridge#2721 introduced a regression by not properly declaring the exposed port in Dockerfile.
The Apache version did it correctly:
https://github.com/docker-library/php/blob/faf8864e3845ced80780c03eefc66c022e2f9ac1/8.1/buster/apache/Dockerfile#L289

The consequence is that other systems, e.g. Traefik, could not know what port to route to.
Kwbmm pushed a commit to Kwbmm/rss-bridge that referenced this pull request Jun 17, 2022
Co-authored-by: Yaman Qalieh <ybq987@gmail.com>
Comment on lines +4 to +5
access_log /var/log/nginx/rssbridge.access.log;
error_log /var/log/nginx/rssbridge.error.log;
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing logs inside the image is not common for production and is a regression compared to the previous image
#3333

@dvikan dvikan deleted the docker-nginx branch May 10, 2023 23:40
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.

None yet

3 participants