-
Notifications
You must be signed in to change notification settings - Fork 29
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
Dockerfile: use unprivileged nginx #1657
base: master
Are you sure you want to change the base?
Conversation
a02be1b
to
06567cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
the e2e tests require the nginx.conf file, don't delete it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the e2e tests require the nginx.conf file, don't delete it
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this cleanup line should probably also stay
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. |
7d3902e
to
ef7cffa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed the correct e2e docker compose file:
github uses CI/ESS/e2e/docker-compose.e2e.yaml
note the ESS in path.
Corresponding changes for the backend tests are probably needed.
ef7cffa
to
9e137f0
Compare
needs a rebase to get the restructred e2e tests ( the docker composefiel touched is then correct |
This allows running this container w/ arbitrary uid support
9e137f0
to
2fd9103
Compare
Change-Id: Ia245afd6a832889bd057ae3e6755f30910f96edf
Change-Id: I5f4e45ab694e7aa8fdefaf66911b49e74deb1403
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be finalized in of sciat online meeting .
( i added this review to block merging until then, otherwise the code changes are OK for me.)
I added a few more reviewers, as this also requires all downstream consumers of the image to adjust the port mapping. Probably needs updates in the docs and in scicatlive. BE e2e tests should work automatically, as the frontend container only exposes one port, so that traefik will pick it up correctly (https://doc.traefik.io/traefik/providers/docker/#port-detection). |
Scicatlive is also fine as it also uses traefik |
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: