Content-Length: 358144 | pFad | http://github.com/coder/coder/pull/19092

D2 chore: move port forwarding out of cli package by ibetitsmike · Pull Request #19092 · coder/coder · GitHub
Skip to content

chore: move port forwarding out of cli package #19092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ibetitsmike
Copy link
Contributor

This PR is the initial work needed for hosting the port forwarding capabilities both from the vpn tunnel package and cli instead of having the logic duplicated.

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

We are adding new fields to a protobuf so we need to bump the corresponding api minor version in vpn/version.go. Convention here is also to add a comment detailing the version history.

@ibetitsmike ibetitsmike changed the title chore: port forwarding moved out of cli package chore: move port forwarding out of cli package Jul 30, 2025
@ibetitsmike ibetitsmike force-pushed the mike/port-forwarding branch from fb745d4 to 7512819 Compare July 30, 2025 07:59
Copy link
Contributor Author

Definitely, I've removed the protobuf changes from this PR.

@ibetitsmike ibetitsmike requested a review from deansheather July 30, 2025 09:39
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Changes look OK to me, some non-blocking comments below.

In the case of returning an interface, the caller still has the option to use either the interface or the struct, or define their own interface.

Comment on lines +41 to +45
AddForward(spec Spec) (Forwarder, error)
// RemoveForward removes an existing port forward.
RemoveForward(spec Spec) error
// ListForwards returns all active port forwards.
ListForwards() []Forwarder
Copy link
Member

Choose a reason for hiding this comment

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

nit: you could remove the Forward suffix from these method names for brevity

Comment on lines +80 to +85
func NewForwarder(spec Spec, opts Options) Forwarder {
return &localForwarder{
spec: spec,
opts: opts,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

suggest NewLocal() instead and returning a *LocalForwarder. Proverb: "accept interfaces, return structs"

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








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/coder/coder/pull/19092

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy