Repository navigation
Make every published host port overridable - #2
Merged
Merged
Conversation
The quickstart lists four URLs and then says to override the port if one is
already taken -- but only the frontend's was actually a variable. Postgres had
POSTGRES_HOST_PORT and nothing else did, so the example in that note
(FRONTEND_HOST_PORT=5175) worked while the API on 8000, the ingestion service
on 8080 and Redis on 6379 were pinned, and 6379 in particular is usually
already in use on a machine that runs Redis for anything else.
Add CORE_API_HOST_PORT, INGESTION_HOST_PORT and REDIS_HOST_PORT, and build the
frontend against ${CORE_API_HOST_PORT} so the browser follows the API when it
moves. Only the host side of each mapping changes; inside the compose network
everything still talks on the standard ports. The URL table now names the
variable for each row, and .env.example lists all five.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the same audit as sahilkalgutkar/modelforge#18 — checking the things CI never runs, the quickstart chief among them.
The quickstart's port table is followed by this note:
That advice only worked for one of the four ports in the table.
docker-compose.ymlhadPOSTGRES_HOST_PORTandFRONTEND_HOST_PORT, but the core API (8000), the ingestion service (8080) and Redis (6379) were hard-coded — and Postgres, the one that does have a variable, was not even in the table. On a machine already running Redis,docker compose upfails on a port bind and the README's own remedy does not help.This adds
CORE_API_HOST_PORT,INGESTION_HOST_PORTandREDIS_HOST_PORT, and threadsCORE_API_HOST_PORTthrough the frontend'sVITE_API_BASE_URLbuild arg — otherwise moving the API's host port would leave the browser calling a port nothing is listening on. Only the host side of each mapping moves; service-to-service traffic inside the compose network is untouched. The table now names the variable per row and includes Postgres and Redis, and.env.examplelists all five.Verified with
docker compose config: with all five set, the published ports resolve to the overrides andVITE_API_BASE_URLfollows tohttp://localhost:8800. I could not bring the full stack up to confirm at runtime — the Docker daemon on this machine is out of disk and its storage has gone read-only — but the change is confined to variable substitution in the port mappings and one build arg, anddocker compose configvalidates both the defaults and the overrides.