add option to disable ssh port forwarding#29
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds reusable port-forwarding submodules for storage and compute, extends the controller submodule with an ChangesSSH Port-Forwarding Module Refactor
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@modules/testing/default.nix`:
- Line 175: The patch hard-sets openstack-testing.enable = true in the
consumer(s) (seen in modules/testing/default.nix) which is redundant because
portForwarding and portForwardingStorage already declare
openstack-testing.enable with default = true and gating uses mkIf cfg.enable;
remove the explicit assignment from the consumer(s) (testController/testStorage)
so they no longer override the option, rely on the existing default, and ensure
any logic that uses cfg.enable continues to use that option rather than a
hard-coded true.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47f7469f-d1dd-4a36-9ff5-6d5bfc067caa
📒 Files selected for processing (1)
modules/testing/default.nix
pkr4711
force-pushed
the
fix-ssh-port-forwarding
branch
2 times, most recently
from
5086f85 to
dc0350d
Compare
pkr4711
force-pushed
the
fix-ssh-port-forwarding
branch
from
dc0350d to
3fb4770
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
modules/testing/default.nix (1)
204-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove hard-coded
openstack-testing.enable = truefrom test profiles.These assignments are redundant (option already defaults to
true) and make CI-level disabling harder, which conflicts with this PR’s stated use case.💡 Proposed minimal change
@@ - openstack-testing.enable = true; # enable / disable all port forwardings openstack-testing.sshHostPort = 1122; @@ - openstack-testing.enable = true; # enable / disable all port forwardings openstack-testing.sshHostPort = 3022; @@ - openstack-testing.enable = true; # enable / disable all port forwardings openstack-testing.sshHostPort = 2022;Also applies to: 304-305, 355-356
🤖 Prompt for 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. In `@modules/testing/default.nix` around lines 204 - 205, Remove the redundant hard-coded setting openstack-testing.enable = true from the test profiles (and related duplicate occurrences), leaving the option to inherit its default so CI can disable it centrally; keep any non-default assignments like openstack-testing.sshHostPort = 1122 if intended, but delete the explicit enable lines found alongside sshHostPort (the occurrences around the blocks that set openstack-testing.sshHostPort and similar entries at the other noted locations).
🤖 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.
Duplicate comments:
In `@modules/testing/default.nix`:
- Around line 204-205: Remove the redundant hard-coded setting
openstack-testing.enable = true from the test profiles (and related duplicate
occurrences), leaving the option to inherit its default so CI can disable it
centrally; keep any non-default assignments like openstack-testing.sshHostPort =
1122 if intended, but delete the explicit enable lines found alongside
sshHostPort (the occurrences around the blocks that set
openstack-testing.sshHostPort and similar entries at the other noted locations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4df5332e-3e87-4a3f-babd-b54c7d337f57
📒 Files selected for processing (1)
modules/testing/default.nix
pkr4711
force-pushed
the
fix-ssh-port-forwarding
branch
from
3fb4770 to
dc02876
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/testing/README.md (1)
3-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefault SSH ports conflict with the new “Default ssh port forwards” section.
This section documents different defaults than Line 114–Line 116 (
controllerVM: 2022,storageVM: 2122), so one of the two is stale and will mislead users.🤖 Prompt for 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. In `@modules/testing/README.md` around lines 3 - 13, The README's "Storage VM" and "Controller VM" SSH examples conflict with the "Default ssh port forwards" section which uses controllerVM: 2022 and storageVM: 2122; update the SSH command examples under the "Storage VM" and "Controller VM" headings to use the same port numbers as the defaults (or alternatively update the "Default ssh port forwards" values to match the examples), ensuring you reference the same identifiers (controllerVM and storageVM) so both sections are consistent.
🧹 Nitpick comments (1)
modules/testing/default.nix (1)
100-105: ⚡ Quick winClarify that compute
sshHostPortmust be explicitly set when enabling forwarding.
sshHostPorthas no default here (which is reasonable), but the option text does not clearly state it is required whenopenstack-testing.enable = true. Adding that note will prevent confusing eval failures.Suggested edit
sshHostPort = mkOption { type = types.port; description = '' - Host port to make the ssh server available. + Host port to make the ssh server available. + Must be set explicitly when openstack-testing.enable is true. ''; };🤖 Prompt for 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. In `@modules/testing/default.nix` around lines 100 - 105, Update the sshHostPort option description so it clearly states that this port must be explicitly set when openstack-testing.enable = true; locate the sshHostPort mkOption (in modules/testing/default.nix) and modify its description string to add a short note like "Must be explicitly set when openstack-testing.enable = true" so users know it's required in that mode.
🤖 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 `@modules/testing/default.nix`:
- Around line 361-364: Update the example SSH port to avoid collision: change
the example value for openstack-testing.sshHostPort from 2022 to 2122 (matching
the storage default) in the modules/testing/default.nix example block so the
example no longer conflicts with the controller default.
In `@modules/testing/README.md`:
- Around line 137-143: The Nix example ends with "nodes.computeVM2 = { ... }"
missing the terminating semicolon which breaks copy/paste; update the example so
the block for nodes.computeVM2 ends with a semicolon (i.e., change the closing
of the assignment for nodes.computeVM2 to include a trailing ';') to make the
snippet valid Nix.
---
Outside diff comments:
In `@modules/testing/README.md`:
- Around line 3-13: The README's "Storage VM" and "Controller VM" SSH examples
conflict with the "Default ssh port forwards" section which uses controllerVM:
2022 and storageVM: 2122; update the SSH command examples under the "Storage VM"
and "Controller VM" headings to use the same port numbers as the defaults (or
alternatively update the "Default ssh port forwards" values to match the
examples), ensuring you reference the same identifiers (controllerVM and
storageVM) so both sections are consistent.
---
Nitpick comments:
In `@modules/testing/default.nix`:
- Around line 100-105: Update the sshHostPort option description so it clearly
states that this port must be explicitly set when openstack-testing.enable =
true; locate the sshHostPort mkOption (in modules/testing/default.nix) and
modify its description string to add a short note like "Must be explicitly set
when openstack-testing.enable = true" so users know it's required in that mode.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad61b1cc-3b13-4e50-8571-25232d810baf
📒 Files selected for processing (3)
modules/testing/README.mdmodules/testing/default.nixtests/openstack-live-migration.nix
| # this is an example how to enable / disable all port forwardings or change port numbers | ||
| # openstack-testing.enable = true; # enable / disable all port forwardings | ||
| # openstack-testing.sshHostPort = 2022; | ||
|
|
There was a problem hiding this comment.
Use a non-conflicting SSH port in the storage example.
Line 363 uses 2022, which is also the controller default. Given this PR’s collision-avoidance goal, the example should use a distinct value (for example 2122, matching storage default).
Suggested edit
- # openstack-testing.sshHostPort = 2022;
+ # openstack-testing.sshHostPort = 2122;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # this is an example how to enable / disable all port forwardings or change port numbers | |
| # openstack-testing.enable = true; # enable / disable all port forwardings | |
| # openstack-testing.sshHostPort = 2022; | |
| # this is an example how to enable / disable all port forwardings or change port numbers | |
| # openstack-testing.enable = true; # enable / disable all port forwardings | |
| # openstack-testing.sshHostPort = 2122; |
🤖 Prompt for 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.
In `@modules/testing/default.nix` around lines 361 - 364, Update the example SSH
port to avoid collision: change the example value for
openstack-testing.sshHostPort from 2022 to 2122 (matching the storage default)
in the modules/testing/default.nix example block so the example no longer
conflicts with the controller default.
In out CI we need an option to disable all port forwardings to avoid port collisions in parallel pipeline runs. Signed-off-by: Paul Kroeher <paul.kroeher@cyberus-technology.de> On-behalf-of: SAP paul.kroeher@sap.com
pkr4711
force-pushed
the
fix-ssh-port-forwarding
branch
from
dc02876 to
7a516b9
Compare
2 participants
In out CI we need an option to disable all
port forwardings to avoid port collisions
in parallel pipeline runs.
Summary by CodeRabbit