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

Use ip command for VPN segment - fix #1125 #1126

Merged
merged 16 commits into from Feb 25, 2019

Conversation

Projects
3 participants
@Shini31
Copy link
Contributor

Shini31 commented Dec 30, 2018

Thank you so much for opening a PR for P9k! Many of our best features and segments have come from the community, and we are excited to see your contribution.

To help you make the best PR, here are some guidelines:

  • The master branch is our stable branch, and the next branch is our development branch. If you are submitting a bug fix, please file your PR against master. If it is a new feature, enhancement, segment, or something similar, please submit it against next. For more information, please see our Developer's Guide.
  • We maintain unit tests for segments and features in the test directory. Please add unit tests for anything new you have developed! If you aren't sure how to do this, go ahead and file your PR and ask for help!
  • For running manual tests in different environments, we have Vagrant and Docker configurations. Please see the Test README and make sure your new feature is working as expected!
  • If your PR requires user configuration, please make sure that it includes an update to the README describing this.
  • P9k maintains a lot of useful information in our Wiki. Depending on the content of your PR, we might ask you to update the Wiki (or provide text for us to use) to document your work. Most PRs don't require this.
  • Please make your commit messages useful! Here is a great short guide on useful commit messages.

Once you have submitted your PR, P9k core contributors will review the code and work with you to get it merged. During this process, we might request changes to your code and discuss different ways of doing things. This is all part of the open source process, and our goal is to help you create the best contribution possible for P9k =).

Please follow this template for creating your PR:

Title

Please make the title of your PR descriptive! If appropriate, please prefix the title with one of these tags:

  • [Bugfix]
  • [New Segment]
  • [Docs]
  • [Enhancement]

Description

Please describe the contribution your PR makes! Screenshots are especially helpful, here, if it's a new segment.

If your PR is addressing an issue, please reference the Issue number here.

Questions

Is there something in your PR you're not sure about or need help with? Is there a particular piece of code you would like feedback on? Let us know here!

@dritter dritter added this to In progress in v0.7.0 via automation Jan 17, 2019

@dritter dritter removed this from In progress in v0.7.0 Jan 18, 2019

@dritter dritter added this to In progress in v0.6.7 via automation Jan 18, 2019

dritter added some commits Jan 21, 2019

Parse IPs properly
This is done if we want to show a public IP, internal IP, or a VPN.
In the VPN case, what we actually want is to display an indicator
that a VPN is active, instead of the VPN IP itself. We parse the
IP here anyway, because we want to save some specific code there.
@dritter

This comment has been minimized.

Copy link
Collaborator

dritter commented Feb 3, 2019

Hi @Shini31 ,

I made some changes to your code ninja-style. The code now runs without externals and is now the same for checking for VPN, as well as parsing public and internal IPs. Could you double check if everything still works for you and report here back? That would be awesome.

Thanks

@dritter dritter force-pushed the Shini31:master branch from 883b810 to 06151ee Feb 3, 2019

@Syphdias
Copy link
Collaborator

Syphdias left a comment

Looks good to me. I just had a few minor nitpicks.

Show resolved Hide resolved test/segments/ip.spec Outdated
Show resolved Hide resolved test/segments/ip.spec Outdated
Show resolved Hide resolved test/segments/public_ip.spec Outdated
Show resolved Hide resolved test/segments/public_ip.spec Outdated
Show resolved Hide resolved test/segments/vpn_ip.spec Outdated
Show resolved Hide resolved test/segments/vpn_ip.spec Outdated
@dritter

This comment has been minimized.

Copy link
Collaborator

dritter commented Feb 21, 2019

@Syphdias Thanks for the review. As I am not the originator of this PR, it seems like I cannot apply your suggestions. Do you mind if I just copy them over, or do you want to create a separate branch where I can cherry-pick them?

@Shini31
Copy link
Contributor Author

Shini31 left a comment

LGTM

@dritter dritter merged commit 613b798 into bhilburn:master Feb 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

v0.6.7 automation moved this from In progress to Done Feb 25, 2019

@dritter

This comment has been minimized.

Copy link
Collaborator

dritter commented Feb 25, 2019

Thx @Shini31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.