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

fixes #1553 #1554

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fixes #1553 #1554

wants to merge 2 commits into from

Conversation

belfhi
Copy link

@belfhi belfhi commented Aug 14, 2024

as described in the issue, the user does not need to be root, even for nginx.
This PR sets the nginx.conf the way the official docs describe what is needed to run as a rootless user.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Contributor

@bpedersen2 bpedersen2 left a comment

Choose a reason for hiding this comment

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

Generally a good idea.

But as it breaks existing installations, it needs prominent mentions in changelog and readme, and probabaly an announcement in the scicat mailinglist at least.

access_log /var/log/nginx/access.log main;


client_body_temp_path /tmp/client_temp;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to keep the original locations ( var/cache/nginx/... ) and instead fix permissions in the image build.

Copy link
Author

Choose a reason for hiding this comment

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

That's also an option. It was more along the line of "just log to stdout / stderr if it's running in a container".

Copy link
Contributor

Choose a reason for hiding this comment

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

See also #1657

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