-
-
Notifications
You must be signed in to change notification settings - Fork 350
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
feat(portforwarding): Allow running script upon port forwarding success #2399
base: master
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.
Nice, thank you for this PR 👍
We need to notably:
- change this to a run command (instead of script file)
- make code ready for a down command (later 🙏, if anyone needs such command)
- make a PR to the github wiki to document this (probably in the setup/advance/vpn-port-forwarding document) and mention the command is a single command, which doesn't understand shell specific syntax such as
&&
, and one should use/bin/sh -c "my shell syntax"
to do so if they want (bash isn't installed by default, only sh)
Thanks!
Ok, I believe that I have resolved all the issues with the initial implementation, I am not a huge fan of comma separating the ports, but that is how the current code works to give maximum compatibility |
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.
Nice job! 👍
Just 2 renames to do 😉
And once done I'll handle the using-that-command-package-and-plug-lines-to-the-logger. I just worked on it yesterday so it's still fresh in my mind 😉 !
) | ||
|
||
func (s *Service) runUpCommand(ctx context.Context, ports []uint16) (err error) { | ||
// run command replacing {{PORTS}} with the ports (space separated) |
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.
any better suggestion than using CSV to seperate values? That could be added in #1113 !
"strings" | ||
) | ||
|
||
func (s *Service) runUpCommand(ctx context.Context, ports []uint16) (err error) { |
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.
I'll add a test to make sure for example this works: /bin/sh -c "echo {{PORTS}}"
- given it plugs arguments by splitting them by spaces, I'm not sure it would handle that case with double quotes correctly. Will check!
@@ -29,6 +29,11 @@ type PortForwarding struct { | |||
// to write to a file. It cannot be nil for the | |||
// internal state | |||
Filepath *string `json:"status_file_path"` | |||
// Command is the port forwarding status command |
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.
Self comment to self-resolve: don't forget to merge PR qdm12/gluetun-wiki#89 !
FYI I'm working on your branch right now, almost done rebasing+fixing the last feedback I left, so please hold 😉 Thanks!! |
Hi @lavalleeale could you give me (force) push access to your Gluetun branch |
Thanks for working on this, can't wait ! |
This PR allows an environment variable (VPN_PORT_FORWARDING_STATUS_SCRIPT) to be set that will run a script whenever there is an update to port forwarding status that could be used to sync with other programs that need to know what ports have been forwarded.
EDIT by qdm12: