Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile OverviewGreptile SummaryThis PR enables running multiple Fides development servers simultaneously by implementing dynamic port allocation and unique container naming based on git branch names. When port 8080 is already in use by a Fides container, the system automatically generates alternate ports using a hash of the branch name and appends branch-based suffixes to container and image names. Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
|
| return DEFAULT_PORTS.copy(), "", "" | ||
|
|
||
| # Port is in use - check if it's a fides or fidesplus container | ||
| if is_container_running("fides") or is_container_running("fidesplus-slim"): |
There was a problem hiding this comment.
Check if container name should be fides${CONTAINER_SUFFIX:-} to match the dynamic naming in docker-compose.yml. Currently checking for static name "fides" which won't detect secondary instances running with suffixes.
There was a problem hiding this comment.
This check is intentionally looking for the primary container name. The logic flow is:
- If port 8080 is NOT in use → use defaults
- If port 8080 IS in use AND it's a fides/fidesplus container → use alternate ports
- If port 8080 IS in use by something else → error
Secondary instances use alternate ports (not 8080), so they wouldn't trigger this code path in the first place. The purpose of this check is to verify whether the port 8080 conflict is caused by a Fides instance (so we can safely use alternate ports) vs some other application (which should error). Checking for the static "fides" name (the primary container) is correct here.
noxfiles/dev_nox.py
Outdated
| import os | ||
|
|
||
| if image_suffix: | ||
| os.environ["IMAGE_SUFFIX"] = image_suffix |
There was a problem hiding this comment.
The os import is placed in the middle of a function rather than at the top of the module. Move this import to the top of the file with the other imports.
| import os | |
| if image_suffix: | |
| os.environ["IMAGE_SUFFIX"] = image_suffix | |
| # Check for port conflicts with existing fides instances BEFORE building | |
| ports, container_suffix, image_suffix = check_for_conflicts(session) | |
| # Set IMAGE_SUFFIX env var for the build process | |
| if image_suffix: |
Then add import os at the top of the file (around line 6).
Context Used: Rule from dashboard - Python imports should always be placed at the top of the file, not near the code that uses them. (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def get_alternate_ports(branch: str) -> dict: | ||
| """Calculate alternate ports based on branch name.""" | ||
| hash_val = int(hashlib.md5(branch.encode()).hexdigest()[:8], 16) | ||
| base = 9000 + (hash_val % 50) * 100 | ||
| return { | ||
| "api": base + 80, | ||
| "db": base + 32, | ||
| "redis": base + 79, | ||
| "admin_ui": base, | ||
| "privacy_center": base + 1, | ||
| } |
There was a problem hiding this comment.
Port collision risk: if two different branches hash to the same base port range (hash_val % 50), they'll use identical ports and conflict. The modulo operation limits to 50 possible base values (9000-13900), which means collisions are likely with many branches.
There was a problem hiding this comment.
This is technically correct, but not a practical concern for this use case. This is a local development tool where developers typically run 2-3 instances at most. With 50 possible port ranges, the probability of collision for a small number of branches is very low.
That said, a potential improvement would be to verify the calculated alternate ports are actually available before using them, rather than assuming they will be free.
| # Check if the default API port is in use | ||
| if not is_port_in_use(DEFAULT_PORTS["api"]): | ||
| # No conflict, use defaults | ||
| return DEFAULT_PORTS.copy(), "", "" |
There was a problem hiding this comment.
Race condition: between checking is_port_in_use() and actually starting containers, another process could bind to the port. Consider checking all required ports (api, db, redis, ui, pc) are available, not just the API port.
There was a problem hiding this comment.
The race condition concern is theoretical - the window is milliseconds, and if Docker can't bind the port, it will error with a clear message. This is acceptable for a dev tool.
However, the suggestion to check all ports (not just API) has merit. If DB (5432) or Redis (6379) are in use by something else, those conflicts wouldn't be detected currently. This could be a future improvement, but isn't blocking for this PR.
Co-authored-by: Cursor <cursoragent@cursor.com>
thabofletcher
left a comment
There was a problem hiding this comment.
It works for me, but I did run into the race condition where they still both tried the same port from starting at the same time. I dont think its a typical use case for this though. Greptile needs to know an improvement when it sees one, considering running nox -s dev twice before wasn't even possible! Perfect is the enemy of the good @greptileai 😂
Description Of Changes
Allows multiple Fides servers to start in Docker. Generates unique image/container names and uses unallocated ports.
Steps to Confirm
nox -s devin one repo, verify the server startsnox -s devin another Fides repo, verify the second server starts as wellPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works