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

added a flag to use v2 detection; works better on my mac; need more testing #66

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Mar 28, 2016

added ability to pass --ipdetect2 to use the new ip detection method, still a WIP, we need to test on more architectures

@fperegrinvs
Copy link
Collaborator

I'm not comfortable about having multiple implementations of the same feature., and extra switch would be my last resort. I would rather test if this algo could replace the current one.

@ghost
Copy link
Author

ghost commented Mar 28, 2016

the intention is you can use the flag for testing (without breaking backwards compatibility); and if its proven to work we can replace the main method

@fperegrinvs
Copy link
Collaborator

fperegrinvs commented Mar 28, 2016 via email

@ghost
Copy link
Author

ghost commented Mar 29, 2016

@kakaroto @lstern so I've been testing with this algorithmn as much as I can.

From what I can tell, it will ALWAYS include the proper IP address in the list of IP address returned, BUT it isn't always the first one. I think a dropdown in the GUI or a simple prompt headless would solve this? What do you guys think

@fperegrinvs
Copy link
Collaborator

So we now have 3 pull requests trying to solve the same problem.

  • We should support users with multiple ips anyway. Only @skroll request handles that.
  • We should fix the ip detection code. Using a package sounds like the safest option but I still don't know in which scenarios the old code was broken.
  • Although I feel that a packaged option is better, breaking ip detection again would be disastrous so we should move very carefully with this. Adding the v2 option and reverting the ip code seems like the safest move,

@kakaroto
Copy link
Owner

The old code was only broken in @azrethos 's case where he has two valid external IP addresses but wants to use the non-default one. As he just commented, even with netifaces, his IP is in the list but it may not be the first one in the list, so it doesn't solve his problem. In that very specific use case, setting it manually through GUI or command line option is the only solution.
Don't add the v2 option, but just add the v2 code into the GUI and make the IP entry into a combobox.

@kaitoarief
Copy link

as @kakaroto said, i also have this problem. i want to use the non-default ip for some reason. if only you can make user to input manual the ip address, it would be the best. if only i know some python code and how to compile, i can change it manual.

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