Dockerfile: use unprivileged nginx #1657
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cfelder
force-pushed
the
unpriviliged
branch
from
a02be1b
to
06567cf
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cfelder - I've reviewed your changes - here's some feedback:
Overall Comments:
- Could you explain how the default nginx configuration from nginx-unprivileged meets your application's needs? The removal of the custom nginx.conf might affect routing or other specific settings.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
bpedersen2
approved these changes
Nov 12, 2024
|
the e2e tests require the nginx.conf file, don't delete it |
bpedersen2
requested changes
Nov 12, 2024
bpedersen2
reviewed
Nov 13, 2024
| @@ -8,8 +8,6 @@ RUN npm ci | |||
| COPY . /frontend/ | |||
| RUN npx ng build | |||
|
|
|||
| FROM nginx:1.25-alpine | |||
| RUN rm -rf /usr/share/nginx/html/* | |||
There was a problem hiding this comment.
this cleanup line should probably also stay
cfelder
force-pushed
the
unpriviliged
branch
from
06567cf
to
8d158a7
Compare
|
The mapping change in docker-compose was correct. But preserve the nginx.conf file and the cleanups in the docker file. Note that this Dockerfile not meant for direct production use, as it contains lots of default passwords. |
cfelder
force-pushed
the
unpriviliged
branch
2 times, most recently
from
7d3902e
to
ef7cffa
Compare
bpedersen2
requested changes
Nov 18, 2024
This allows running this container w/ arbitrary uid support
Junjiequan
force-pushed
the
unpriviliged
branch
from
ef7cffa
to
9e137f0
Compare
Open
|
needs a rebase to get the restructred e2e tests ( the docker composefiel touched is then correct |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows running this container w/ arbitrary uid support
Description
Short description of the pull request
Motivation
running SciCat in an OpenShift environment w/ arbitrary uids
Fixes:
Changes:
Summary by Sourcery
Use the unprivileged nginx image to support running the container with arbitrary user IDs, and update the exposed port to 8080.
Build:
CI: