-
Notifications
You must be signed in to change notification settings - Fork 954
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
fb745d4
to
7512819
Compare
Definitely, I've removed the protobuf changes from this PR. |
There was a problem hiding this 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.
AddForward(spec Spec) (Forwarder, error) | ||
// RemoveForward removes an existing port forward. | ||
RemoveForward(spec Spec) error | ||
// ListForwards returns all active port forwards. | ||
ListForwards() []Forwarder |
There was a problem hiding this comment.
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
func NewForwarder(spec Spec, opts Options) Forwarder { | ||
return &localForwarder{ | ||
spec: spec, | ||
opts: opts, | ||
} | ||
} |
There was a problem hiding this comment.
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"
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.