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

Conversation

Projects
None yet
3 participants
@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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

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.

@tresni

This comment has been minimized.

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Collaborator

tresni commented Apr 27, 2016

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

Jesse Jarzynka
Add more advanced VPN status/connection script
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

This comment has been minimized.

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

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