Skip to content

fix(port): do not check port availability when machine start#92

Merged
ihexon merged 1 commit intomainfrom
fixport
Sep 12, 2025
Merged

fix(port): do not check port availability when machine start#92
ihexon merged 1 commit intomainfrom
fixport

Conversation

@ihexon
Copy link
Collaborator

@ihexon ihexon commented Sep 12, 2025

No description provided.

@ihexon ihexon requested a review from BlackHole1 as a code owner September 12, 2025 09:23
@coderabbitai
Copy link

coderabbitai bot commented Sep 12, 2025

Walkthrough

Introduces a pre-check to obtain the SSH port via mc.GetSSHPort() before constructing the gvproxy command. Adds error handling that aborts Start with a wrapped error if port retrieval fails. The rest of the gvproxy initialization sequence remains unchanged.

Changes

Cohort / File(s) Summary of changes
SSH port acquisition pre-check
pkg/machine/gvproxy/gvproxy.go
Replaced direct SSH.Port assignment with mc.GetSSHPort() call before gvproxy command construction; added error handling to return “unable to get available ssh port: %w” on failure; no other flow alterations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (2 warnings, 1 inconclusive)

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The current title "fix(port): do not check port availability when machine start" is misleading relative to the changeset: the diff and summary show the code now calls mc.GetSSHPort() and adds error handling before constructing the gvproxy command (i.e., it adds an explicit port retrieval/check), so the title does not accurately describe the primary change. Rename the PR to clearly reflect the actual change, for example "fix(port): retrieve available SSH port before starting gvproxy and return error if unavailable" or "fix(gvproxy): use mc.GetSSHPort() to obtain SSH port and handle errors"; keep it short and focused on the added port retrieval/error handling.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check ❓ Inconclusive I cannot evaluate the PR description because no current description text was provided in the inputs; without the PR description I cannot determine whether it is related to the changeset (the provided raw_summary does describe the ssh-port retrieval change, but that is not the PR description itself). Please provide the PR description text or update the description to state that pkg/machine/gvproxy now retrieves the SSH port via mc.GetSSHPort() before starting gvproxy and returns an error if retrieval fails so the description can be validated as related to the changeset.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fixport

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • MCP integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7341a35 and e06a0a7.

📒 Files selected for processing (1)
  • pkg/machine/gvproxy/gvproxy.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
pkg/machine/gvproxy/gvproxy.go

[failure] 38-38:
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)

🔇 Additional comments (1)
pkg/machine/gvproxy/gvproxy.go (1)

36-48: Keep GetSSHPort() — it populates mc.SSH.Port used elsewhere

GetSSHPort() can allocate and assign mc.SSH.Port if the configured port is in use; multiple subsystems expect mc.SSH.Port to be populated, so removing the call risks breaking SSH-related flows.

  • Definition: pkg/machine/vmconfig/vmconfig.go (GetSSHPort sets mc.SSH.Port).
  • Consumers: pkg/machine/ssh/service/runner.go (ssh.NewConfig uses mc.SSH.Port), pkg/api/backend/exec.go (ssh.Dial uses mc.SSH.Port), pkg/api/backend/info.go (returns SSHPort), pkg/machine/machine_common.go (passes mc.SSH.Port).

Keep the call, or refactor GetSSHPort to separate "probe/allocate" behavior from "populate" so allocation can be deferred if you truly must avoid probing at machine start.

Likely an incorrect or invalid review comment.

@ihexon ihexon merged commit 8aca952 into main Sep 12, 2025
2 of 3 checks passed
@ihexon ihexon deleted the fixport branch September 12, 2025 09:32
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