Skip to content

Modify healthcheck to get port from config.json instead of hard-coding to 5150 - #114

Open
shahmirn wants to merge 2 commits into
rstrouse:masterfrom
shahmirn:master
Open

shahmirn wants to merge 2 commits into
rstrouse:masterfrom
shahmirn:master

Conversation

@shahmirn

@shahmirn shahmirn commented Aug 24, 2026

Copy link
Copy Markdown

Hello!

My setup is a bit different where I'm not using a bridge network but a macvlan network, so I can't just expose 5150 to a different port. Instead, I've modified my config.json to have:

      "http": {
...
        "port": 80,
...
      },

This PR modifies the Dockerfile and the compose file so that it calls docker/healthcheck.js instead, where it gets the port from /app/config.json, and falls back to 5150 as the default


Before and after:

Screenshot 2026-08-24 at 12 51 02 PM

The top container is using macvlan and the bottom one is using bridge; both are using the same config

@shahmirn shahmirn changed the title Modify healthcheck to get port dynamically instead of hard-coding to 5150 Modify healthcheck to get port from config.json instead of hard-coding to 5150 Aug 24, 2026
@shahmirn

Copy link
Copy Markdown
Author

@tagyoureit Could you take a look when you get a chance? I appreciate it. Thank you!

@tagyoureit tagyoureit left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review: Healthcheck Dynamic Port

Nice improvement — the script is clean and well-structured. One functional concern and a couple of minor suggestions:

🟡 Medium: Environment variable override not detected

The config loader in server/config/Config.ts applies POOL_WEB_SERVERS_HTTP_PORT (and similar env vars) at runtime in-memory — they're never written back to config.json on disk. If a user configures their port solely via environment variable (as documented in docker-compose.yml comments), the healthcheck will still read 5150 from the static file and fail.

Suggested fix in docker/healthcheck.js — after reading the config file, add an env var fallback:

// Match the config loader's env var behavior
const envPort = process.env.POOL_WEB_SERVERS_HTTP_PORT;
if (envPort) {
    const parsed = parseInt(envPort, 10);
    if (!isNaN(parsed)) port = parsed;
}

💡 Minor: curl is now unused

The runtime stage still installs curl (apk add --no-cache curl) which was only needed for the old healthcheck. Removing it saves ~2MB from the image.

💡 Minor: COPY placement

The COPY docker/healthcheck.js /usr/local/bin/healthcheck.js line is placed after USER node. While this works (COPY runs as root at build time regardless), it's more conventional to group it with the other COPY statements above the USER directive.


Otherwise the implementation looks solid — proper timeout handling, clean status code check, graceful config fallback with optional chaining.

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