fix(deployments): move port range to DockerExecutorConfig (AIRCORE-858) - #522
Merged
Conversation
Host port allocation now reads port_range_start/end from executor YAML instead of entity backend_config.docker, which was silently ignored. Removes port range fields from the entity schema and OpenAPI, adds range validation on DockerExecutorConfig, and updates tests and README docs. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughDocker host port range configuration moves from ChangesPort range migration to executor config
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-deployments/README.md`:
- Around line 29-31: Clarify the docker executor port allocation docs by stating
that port_range_end is inclusive in the named executor config description and
the example text. Update the README wording around port_range_start and
port_range_end so it explicitly says the allocator scans the full range from
start through end, using the existing executor/config terminology to avoid
misreading the valid port count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 55eac895-9c58-42c0-9ec6-58e368a2d0cb
📒 Files selected for processing (12)
plugins/nemo-deployments/README.mdplugins/nemo-deployments/openapi/openapi.yamlplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/ports.pyplugins/nemo-deployments/src/nemo_deployments_plugin/entities.pyplugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.pyplugins/nemo-deployments/tests/integration/test_reconcile_docker.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_executor_config.pyplugins/nemo-deployments/tests/unit/backends/docker/test_ports.pyplugins/nemo-deployments/tests/unit/test_registry.py
💤 Files with no reviewable changes (4)
- plugins/nemo-deployments/src/nemo_deployments_plugin/entities.py
- plugins/nemo-deployments/openapi/openapi.yaml
- plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py
- plugins/nemo-deployments/tests/integration/test_reconcile_docker.py
Contributor
|
Address CodeRabbit review on PR #522: document that the allocator scans port_range_start through port_range_end inclusively. Signed-off-by: Tyler Bray <tbray@nvidia.com>
The README follow-up commit cancelled the prior CI run via concurrency; push an empty commit to produce a single clean check rollup on PR #522. Signed-off-by: Tyler Bray <tbray@nvidia.com>
mckornfield
approved these changes
Jun 30, 2026
mckornfield
left a comment
mckornfield
left a comment
Contributor
There was a problem hiding this comment.
ugh this container port thing
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
2 participants
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.
Summary
port_range_start/port_range_endfrom entityDockerDeploymentConfigto executor-levelDockerExecutorConfigso platform YAML controls host port allocationcreate_deploymentandfind_available_portto read the executor port range; entitybackend_config.dockerretains only deployment-specific overrides likenetworkDockerExecutorConfig, and document executor YAML in the plugin READMETest plan
uv run pytest plugins/nemo-deployments/tests/unit/ -q(153 passed)make refresh-openapi(deployments spec updated;port_range_*removed from entity schema)uv run ruff checkon changed filesuv run pytest plugins/nemo-deployments/tests/integration/ -vSummary by CodeRabbit