Skip to content
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

Add script option #153

Closed
wants to merge 9 commits into from
Closed

Add script option #153

wants to merge 9 commits into from

Conversation

terdei
Copy link
Contributor

@terdei terdei commented Jul 21, 2017

The script specified will be run after tunnel buildup and before teardown. Very similar to OpenVPN's up/down options.
The script will receive info about the tunnel as VPN_* environment variables.
Use cases are implementing split DNS, and/or overriding gateway specified routes.

After implementing this feature for myself, I noticed #101, and the option to use pppd ip-up/ip-down scripts for this task. So this is an alternative to that.

Tamas Erdei and others added 7 commits July 21, 2017 18:56
The script specified will be run after tunnel buildup and before
teardown. Very similar to OpenVPN's up/down options.
The script will receive info about the tunnel as VPN_* environment
variables.
This option can be used as an alternative to pppd ip-up/ip-down
scripts.
Use cases are implementing split DNS, and/or overriding gateway
specified routes.
@DimitriPapadopoulos
Copy link
Collaborator

I have modified your branch so that it passes the lint test.

@terdei
Copy link
Contributor Author

terdei commented Jul 24, 2017

Thanks for fixing that. I have seen the lint errors in travis, but have not had the time so far to fix them.

@lkundrak
Copy link
Collaborator

The commits should be logical units with the commit message describing the nature of the change and the reasoning behind it.

You seem to have split a single change into multiple commits and didn't provide explanations, which make it sort of difficult to review the changes. Perhaps they could just be squashed together?

@terdei
Copy link
Contributor Author

terdei commented Feb 21, 2018

@lkundrak : I agree with you, my merge request contained only a sinlge commit, but turned out that it had lint errors. Dimitri fixed this, so the other commits came from this.
And then, later the master was updated, so I merged the change to the new master.
Question is: how to fix this. Shall I close this mr, and create a new one on top of the latest master, also taking care of lint checks?
I am happy to do this, if there is a chance that my change will be merged in. But I would not waste my time, if the change gets rejected for other reasons.
Pls. let me know how to move on.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Apr 4, 2018

@terdei While a --script option is probably easier for end-users, using pppd ip-up/ip-down scripts is probably better for Linux packagers. See also new option --pppd-call introduced by #270 which paves the way for running openfortivpn as a regular user but requires a pppd configuration file in /etc/ppp anyway.
I'd rather avoid multiple methods to run scripts. How do you feel about this? Am I missing a use case where --script makes a difference?

@mrbaseman mrbaseman mentioned this pull request Nov 13, 2018
@mrbaseman
Copy link
Collaborator

can we close this one? We have added some information on our wiki how to use the already existing pppd mechanism

@mrbaseman
Copy link
Collaborator

what shall we do with this one? One would probably have to squash and rebase the changes manually, but as stated already before, similar functionality is provided by pppd ip-up/down scripts.
Therefore, can we close this one?

@mrbaseman
Copy link
Collaborator

Oh, I have scrolled over my earlier message - so sorry for asking the same question again. Given the fact that nobody has replied in a month I assume we can close it.

@mrbaseman mrbaseman closed this Dec 19, 2018
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.

4 participants