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

DNM: Debug pr 1657 #1675

Closed
wants to merge 4 commits into from
Closed

Conversation

bpedersen2
Copy link
Contributor

@bpedersen2 bpedersen2 commented Nov 29, 2024

Just trying to debug #1657 failures

Summary by Sourcery

Update Docker configuration to use nginxinc/nginx-unprivileged and change the server port to 8080. Modify CI workflow to include frontend service logs on failure.

Build:

  • Switch base image in Dockerfile to nginxinc/nginx-unprivileged and change exposed port to 8080.

CI:

  • Add logging for the frontend service in GitHub Actions workflow on failure.

cfelder and others added 2 commits November 29, 2024 08:55
This allows running this container w/ arbitrary uid support
Change-Id: Ieb6167f31ef2d34defbf4f40cde42e07776e5019
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @bpedersen2 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please provide a proper description explaining the purpose of switching to nginx-unprivileged and the port changes. If this security improvement is not directly related to issue Dockerfile: use unprivileged nginx #1657, consider making it a separate PR.
  • Once debugging is complete, update the PR title to remove 'DNM' and include a clear description of what was found and fixed.
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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Change-Id: I484d57aa2ca58d6ba2647a15ecd6dde1a6c9be00
Change-Id: I98f4a468a499f30644a9d09cff61b98bc79d7549
@bpedersen2
Copy link
Contributor Author

Closing as PR against #1657 with the necessary fixes has been created

@bpedersen2 bpedersen2 closed this Nov 29, 2024
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