Wrap-up changes for v3 release #45
Conversation
Summary of ChangesHello @mesudip, 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! This pull request represents a significant wrap-up for the v3 release, primarily by modernizing and expanding the project's documentation to reflect new capabilities. It introduces robust support for Docker Swarm, advanced SSL certificate management via Cloudflare DNS challenges, and sticky sessions, all configurable through environment variables. The accompanying utility scripts for certificate handling have also been refactored to integrate seamlessly with these new configuration paradigms, ensuring a more streamlined and powerful user experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prepares for the v3 release by adding code coverage configuration, significantly overhauling the README.md for better documentation, and refactoring the getssl and verify helper scripts. The script refactoring is well-done, making them more robust and aligned with the application's configuration patterns. The new README is a substantial improvement, but I've found several typos and formatting issues that should be corrected to ensure clarity for users. My review focuses on these documentation fixes.
| docker run -d \ | ||
| --name registry \ | ||
| --network frontend \ | ||
| -e VIRTUAL_HOST="htttps://registry.example.com/v2" \ |
There was a problem hiding this comment.
There is a typo in the URL scheme. htttps should be https. This will cause the example to fail if copied by a user.
| -e VIRTUAL_HOST="htttps://registry.example.com/v2" \ | |
| -e VIRTUAL_HOST="https://registry.example.com/v2" \ |
README.md
Outdated
| - **Swarm Ready:** Compatible with Docker Swarm services. | ||
|
|
||
| ## Philosophy | ||
| `nginx-proxy` is built on the belief that infrastructure should be invisible. Each service keeps it's configuration. |
There was a problem hiding this comment.
There is a grammatical error here. it's is a contraction for 'it is' or 'it has'. The possessive form is its.
| `nginx-proxy` is built on the belief that infrastructure should be invisible. Each service keeps it's configuration. | |
| `nginx-proxy` is built on the belief that infrastructure should be invisible. Each service keeps its configuration. |
| | `NGINX_WORKER_CONNECTIONS` | `65535` | Max connections per worker. | | ||
| | `CERT_RENEW_THRESHOLD_DAYS` | `30` | By default certificates are renewed when they have <=30 days remaining. | | ||
| | `ENABLE_IPV6` | `false` | Enable IPv6 support on nginx. | | ||
| | `DOCKER_SWARM` | `ignore` | Treats every container like local by defeault. Set `enable` for Swarm support, `strict` for Swarm-only or`exclude` to not include swarm containers | |
There was a problem hiding this comment.
There are a couple of typos in the description for DOCKER_SWARM:
defeaultshould bedefault.- The backtick in
or\exclude`seems misplaced, which affects formatting. It should probably beor `exclude``.
| | `DOCKER_SWARM` | `ignore` | Treats every container like local by defeault. Set `enable` for Swarm support, `strict` for Swarm-only or`exclude` to not include swarm containers | | |
| | `DOCKER_SWARM` | `ignore` | Treats every container like local by default. Set `enable` for Swarm support, `strict` for Swarm-only or `exclude` to not include swarm containers | |
README.md
Outdated
| | `example.com -> :8080` | Proxies to port 8080. | | ||
| | `https://example.com` | HTTPS proxy to container exposed http port | | ||
| | `https://example.com/api` | Only /api path is passed on to container. /api prefix is not removed | | ||
| | `https://example.com/api -> /v1` | Re-maps path `/api` to `/v1` on contianer exposed port| |
There was a problem hiding this comment.
Typo: contianer should be container.
| | `https://example.com/api -> /v1` | Re-maps path `/api` to `/v1` on contianer exposed port| | |
| | `https://example.com/api -> /v1` | Re-maps path `/api` to `/v1` on container exposed port| |
README.md
Outdated
| | `https://example.com` | HTTPS proxy to container exposed http port | | ||
| | `https://example.com/api` | Only /api path is passed on to container. /api prefix is not removed | | ||
| | `https://example.com/api -> /v1` | Re-maps path `/api` to `/v1` on contianer exposed port| | ||
| | `https://example.com/api -> :8080/v1` | Re-maps path `/api` to `/v1` on contianer port 8080| |
There was a problem hiding this comment.
Typo: contianer should be container.
| | `https://example.com/api -> :8080/v1` | Re-maps path `/api` to `/v1` on contianer port 8080| | |
| | `https://example.com/api -> :8080/v1` | Re-maps path `/api` to `/v1` on container port 8080| |
README.md
Outdated
| - **Auto-Upgrade:** `VIRTUAL_HOST=https+wss://example.com` (Supports both HTTP and WSS on te host). | ||
|
|
||
| ### Multiple Hosts | ||
| A single container can serve multiple domains path mappings. All that matters is that ev variable starts with `VIRTUAL_HOST` e.g. `VIRTUAL_HOST1`, `VIRTUAL_HOST2`, etc. |
There was a problem hiding this comment.
Typo: ev variable should be env variable.
| A single container can serve multiple domains path mappings. All that matters is that ev variable starts with `VIRTUAL_HOST` e.g. `VIRTUAL_HOST1`, `VIRTUAL_HOST2`, etc. | |
| A single container can serve multiple domains path mappings. All that matters is that env variable starts with `VIRTUAL_HOST` e.g. `VIRTUAL_HOST1`, `VIRTUAL_HOST2`, etc. |
README.md
Outdated
| Format: `STATIC_VIRTUAL_HOST=domain.com->http://192.168.0.1:8080`. | ||
|
|
||
| `-e "VIRTUAL_HOST=https+wss://example.com"` | ||
| **Note** Beaware that if domain as target, nginx will crash if DNS resolution fails. |
There was a problem hiding this comment.
Typo: Beaware should be Be aware.
| **Note** Beaware that if domain as target, nginx will crash if DNS resolution fails. | |
| **Note** Be aware that if domain as target, nginx will crash if DNS resolution fails. |
README.md
Outdated
| ### Multiple Virtual Hosts on same container | ||
| To have multiple virtual hosts out of single container, you can use `VIRTUAL_HOST1`, `VIRTUAL_HOST2`, `VIRTUAL_HOST3` and so on. In fact the only thing it matters is that the environment variable starts with `VIRTUAL_HOST`. | ||
| ## Docker Swarm Support [Preview] | ||
| Enable warm mode by setting `DOCKER_SWARM` to `enable` (local & swarm) or `strict` (swarm only). |
There was a problem hiding this comment.
Typo: warm mode should be swarm mode.
| Enable warm mode by setting `DOCKER_SWARM` to `enable` (local & swarm) or `strict` (swarm only). | |
| Enable swarm mode by setting `DOCKER_SWARM` to `enable` (local & swarm) or `strict` (swarm only). |
README.md
Outdated
| If current node is not manager, set `SWARM_DOCKER_HOST=tcp://manager:2375`. | ||
|
|
||
| **Example:** setting up a go-ethereum node. | ||
| **Warning** : Automatic exposed port detection will not work when swrm support is enabled. You must explicitly set port on the `VIRTUAL_HOST` or set `VIRTUAL_PORT` on the container. |
There was a problem hiding this comment.
Typo: swrm support should be swarm support.
| **Warning** : Automatic exposed port detection will not work when swrm support is enabled. You must explicitly set port on the `VIRTUAL_HOST` or set `VIRTUAL_PORT` on the container. | |
| **Warning** : Automatic exposed port detection will not work when swarm support is enabled. You must explicitly set port on the `VIRTUAL_HOST` or set `VIRTUAL_PORT` on the container. |
|
|
||
| ## Manual Certificate Commands | ||
| ``` | ||
| docker exec nginx-proxy verify www.example.com ## check if request routes back to this server. |
There was a problem hiding this comment.
The double hash ## is unusual for a shell comment in an example. For clarity and consistency, a single hash # is preferred.
| docker exec nginx-proxy verify www.example.com ## check if request routes back to this server. | |
| docker exec nginx-proxy verify www.example.com # check if request routes back to this server. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45 +/- ##
===========================================
- Coverage 70.15% 58.43% -11.72%
===========================================
Files 45 31 -14
Lines 3980 2507 -1473
Branches 522 406 -116
===========================================
- Hits 2792 1465 -1327
+ Misses 1013 932 -81
+ Partials 175 110 -65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.