Skip to content

Add support for STATUS_LISTEN_PORT env var #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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rohvani
Copy link
Member

@rohvani rohvani commented Jul 10, 2025

This PR adds support to nginx-proxy so that you can specify which port that it'll listen on for the status/metrics page.

Depended on by https://github.com/secondlife/conductor-poc so that we can have reverse-proxies to backbonecommon and counter without relying on redirects through the ELB.

@rohvani rohvani requested review from bennettgoble and Copilot July 10, 2025 18:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds configurability for the NGINX status page port via a new STATUS_LISTEN_PORT environment variable.

  • Reads STATUS_LISTEN_PORT in the NGINX status config template instead of hard-coding 8091.
  • Updates documentation (README) to describe the new variable and its default.
  • Sets a default in the Dockerfile and adjusts the Datadog label to reference the new variable.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/etc/nginx/conf.d/status.conf.template Changed listen 8091; to use {{ .Env.STATUS_LISTEN_PORT }}
README.md Added STATUS_LISTEN_PORT to the env var table and usage section
Dockerfile Defined ENV STATUS_LISTEN_PORT=8091 and updated Datadog label
Comments suppressed due to low confidence (3)

README.md:20

  • [nitpick] The table entry for STATUS_LISTEN_PORT is missing an example value—consider adding one (e.g., 9090) to illustrate its usage.
| `STATUS_LISTEN_PORT` | nginx status port | No | 8091 | | 

README.md:70

  • [nitpick] For consistency and clarity, wrap the variable reference in code ticks, e.g., http://localhost:`${STATUS_LISTEN_PORT}`/nginx_status.
http://localhost:${STATUS_LISTEN_PORT}/nginx_status.

Dockerfile:14

  • There are no tests verifying that STATUS_LISTEN_PORT is correctly applied to the NGINX configuration; consider adding an integration or template‐rendering test.
ENV STATUS_LISTEN_PORT=8091

@@ -1,6 +1,6 @@
# NGINX status/stats used by datadog, et al.
server {
listen 8091;
listen {{ .Env.STATUS_LISTEN_PORT }};
Copy link
Preview

Copilot AI Jul 10, 2025

Choose a reason for hiding this comment

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

If STATUS_LISTEN_PORT is unset, this will emit an invalid listen directive. Consider using a default/fallback (e.g., via Sprig’s default function) or validating the variable before template rendering.

Copilot uses AI. Check for mistakes.

@@ -11,6 +11,7 @@ COPY src /
ENV KEEPALIVE_TIMEOUT=65
ENV PROXY_UWSGI=0
ENV LISTEN_PORT=80
ENV STATUS_LISTEN_PORT=8091
Copy link
Preview

Copilot AI Jul 10, 2025

Choose a reason for hiding this comment

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

The default port (8091) is defined in multiple places (Dockerfile, template, docs); consider centralizing this default or referencing a single source to avoid drift.

Suggested change
ENV STATUS_LISTEN_PORT=8091
ARG DEFAULT_STATUS_LISTEN_PORT=8091
ENV STATUS_LISTEN_PORT=$DEFAULT_STATUS_LISTEN_PORT

Copilot uses AI. Check for mistakes.

…status page port used by datadog/metric collectors
@rohvani rohvani force-pushed the pepper/status-page-port-env branch from c3e3c60 to e9e59e6 Compare July 10, 2025 19:05
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