[home] Reintroduce nginx.conf.template #628
Conversation
Allows env vars (PORT) in nginx conf
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @Seburan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request reintroduces the nginx.conf.template file for the Home service, enabling environment variable substitution within the Nginx configuration. The changes involve modifying the Dockerfile to copy the template file and setting an environment variable for the output directory. Additionally, the nginx.conf file is updated to use environment variables for the listening port.
Highlights
- Reintroduction of Nginx Template: The
nginx.conf.templateis reintroduced to allow environment variable substitution in the Nginx configuration. - Dockerfile Modification: The Dockerfile is updated to copy the
nginx.conf.templateand set theNGINX_ENVSUBST_OUTPUT_DIRenvironment variable. - Nginx Configuration Update: The
nginx.conffile is modified to use the${PORT}environment variable for the listening port.
Changelog
- services/home/Dockerfile
- Added
ENV NGINX_ENVSUBST_OUTPUT_DIR=/etc/nginxto set the default output directory for templating (line 18). - Modified the COPY command to copy
nginx.confto/etc/nginx/templates/nginx.conf.template(line 21). - Removed
EXPOSE 8080(line 24).
- Added
- services/home/nginx.conf
- Replaced hardcoded port
8080with${PORT}for the listen directive (line 75 and 76).
- Replaced hardcoded port
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A template's return,
Env vars now can burn,
Nginx's config,
No longer rigid,
Flexibility we earn.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a change to use environment variable substitution in the nginx.conf file. The changes involve moving the nginx.conf file to a templates directory and modifying it to use environment variables. However, there are some issues that need to be addressed before this pull request can be merged.
Summary of Findings
- Missing Nginx Configuration: The nginx.conf file, which contains the reverse proxy configurations, is now located in the /nginx directory, but it's not being copied into the container. This will cause the reverse proxy to fail.
- Environment Variable Substitution: The nginx.conf file is now a template, but there is no mechanism to perform the environment variable substitution. This means that the $PORT variable will not be replaced with the actual port number.
Merge Readiness
This pull request is not ready to be merged. The nginx configuration is not being properly loaded into the container, and the environment variable substitution is not being performed. These issues must be addressed before merging. I am unable to approve this pull request, and users should have others review and approve this code before merging.
* add uint array back * fix bug for contextual * Linter fixes for EJS templates (#618) * Feature/bidding and auction (#619) Documentation Updates: The bidding-and-auction.md file has been updated to clarify the demo's goals, assumptions, and key exclusions. The steps have been revised to provide more precise instructions for users. Console Logging: Console log messages in ssp-a, ssp-x, and ssp-y components have been modified to provide more context about the auction configurations. Code Cleanup: Unnecessary code related to encoding requests has been removed from ssp-y/construct-component-auction.ts. * Bugfix/bna demo for April 18th release (#617) * [docker-compose] ad network configuration Setting network configuration to allow integration with bidding and auction demo. Privacy Sandbox Demos will use 172.16.128.0/24 range to not collide with Bidding and Auction services with fixed ips on 172.16.0.0/24 * package.json scripts - update start command to be able to refresh a specific service. use `npm run start -- [service-name]` - update stop command to remove volumes - update build command to build without cache * docker-compose : fix race condition on ad-tech services ad-tech services are using the same codebase and were all mounting the local ./services/ad-tech folder causing race conditions. Often one of the ad-tech services would not start properly. as a temporary workaround we are removing the filesystem mounted volume. * Bugfix/mmt-multi-touch (#620) * add xxx-news services in firebase for Multi Touch Attribution Demo * multi-touch attribution demo doc : update link * fix firebase_deploy.sh * Bugfix/home docs (#626) * demo docs fixes : - fix issue #624 - fix issue #623 - fix issue #622 - fix issue #621 Documentation Fixes: The PR resolves multiple documentation issues by fixing relative file paths and URLs in the markdown files. It also updates image links to use relative paths instead of absolute URLs. Docusaurus Configuration: The Docusaurus configuration is updated to throw an exception when Markdown links are broken, ensuring link integrity. The trailingSlash option is enabled to add trailing slashes to URLs. Nginx Configuration: The Nginx configuration is strengthened by upgrading Nginx to version 1.27.5-alpine and adding directives for security and optimization. The configuration file is now directly copied to /etc/nginx/nginx.conf instead of using a template. Docker Compose Update: The docker-compose.yml file is updated to use the newer Nginx image version 1.27.5-alpine. Markdown file paths are preferred. see https://docusaurus.io/docs/next/markdown-features/links updated docusaurus config to throw an exception when Markdown links are broken * [home] strenghten nginx configuration upgrade nginx add nginx.conf directives for security and optimization * fix docs link to full sized image hosted on Github * [home] Reintroduce nginx.conf.template (#628) Allows env vars (PORT) in nginx conf --------- Co-authored-by: chasinandrew <[email protected]> Co-authored-by: Severin FERRAND <[email protected]> Co-authored-by: Sid Sahoo <[email protected]>
Description
Re-introducing the nginx template to allows environment variable substitution in nginx.conf
Affected services
Other: