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 more advanced VPN status/connection script #388

Merged
merged 1 commit into from Apr 28, 2016
Merged

Conversation

@jessejoe
Copy link

@jessejoe jessejoe commented Apr 12, 2016

The existing vpn-check.3s.sh script simply checks VPN status. I wanted to actually connect/disconnect the VPN as well.

This script has the following features:

  • Will actually launch a VPN connection and wait for it to complete
  • Configurable and could work with any CLI VPN client
  • Will update the menu item instantly after connecting/disconnecting

Example:

@keithamus
Copy link
Collaborator

@keithamus keithamus commented Apr 12, 2016

Thanks for the PR @jessejoe. The policy for this repository is that we'd rather have one plugin to handle one thing (were thing is "vpn" in this case). Could you please, instead of writing a new vpn script, update the existing one? Thanks 😄

@jessejoe
Copy link
Author

@jessejoe jessejoe commented Apr 12, 2016

@keithamus no problem! I didn't want to overwrite the simple one if people found it useful. Will modify the PR in a bit.

@jessejoe
Copy link
Author

@jessejoe jessejoe commented Apr 12, 2016

@keithamus Changes have been submitted. I renamed the file simply to vpn.sh since the script doesn't need to be refreshing every 3 seconds in this implementation. Note that this pretty much changes the use of the script from being a "VPN status check" to a "VPN connect/disconnect and status check" which requires more configuration that the one it's replacing.

Also, looks like the build failed since the bitbar.image is a GIF, should i change it to a non-animated format? I'm not sure the reasoning, but the GIF definitely shows it better.

@jessejoe jessejoe force-pushed the jessejoe:master branch from 41a868f to d1d631b Apr 12, 2016
@tresni
Copy link
Collaborator

@tresni tresni commented Apr 13, 2016

We only had png's and jpeg's when we wrote the test tool. I don't see a problem with gifs as long as they are relevant and not seizure inducing. We just need to update the test script to allow them :)

@jessejoe
Copy link
Author

@jessejoe jessejoe commented Apr 13, 2016

@tresni I changed to PNG so it would pass the checks. Let me know if you'd like me to change it back, otherwise I think it could be merged (if no other comments)

@tresni
Copy link
Collaborator

@tresni tresni commented Apr 13, 2016

While I agree with @keithamus that we don't want to do the same thing 6 different ways, I actually don't think your script and the existing vpn script are really the same thing. To me the original script is the "drop dead simple, vpn 101" script, and yours is "vpn for people who want to do more then just a basic check". My personal script looks very similar to yours. As such I would actually keep them as seperate scripts.

So, I'll leave this to @keithamus to merge if he believes it really replaces the existing script. :)

@jessejoe
Copy link
Author

@jessejoe jessejoe commented Apr 14, 2016

@tresni: I agree, and that's why I originally submitted it that way. Of course, my script could be configurable to do both, but it's really up to the merge masters. I'll do whatever they want :)

@tresni
Copy link
Collaborator

@tresni tresni commented Apr 27, 2016

@jessejoe sorry to ask you to do this, but if you resubmit as a new plugin I'll merge ;-)

This script has the following features:
 * Will actually launch a VPN connection and wait for it to complete
 * Configurable and could work with any CLI VPN client
 * Will update the menu item instantly after connecting/disconnecting
@jessejoe jessejoe force-pushed the jessejoe:master branch from d1d631b to 10edc18 Apr 28, 2016
@jessejoe
Copy link
Author

@jessejoe jessejoe commented Apr 28, 2016

@tresni OK, I squashed everything back down to 1 commit and added a few more comments for clarification within the script. Should be good to merge?

@tresni tresni merged commit 31a1a91 into matryer:master Apr 28, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants