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.

Sign up for GitHub

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

Jump to bottom

Issue 23: Add method to get and display IP address on vebose #48

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

Issue 23: Add method to get and display IP address on vebose #48

wants to merge 5 commits into from

Conversation

vinmay
Copy link
Contributor

@vinmay vinmay commented Oct 16, 2020

#23

@gridhead gridhead linked an issue Oct 17, 2020 that may be closed by this pull request
Fetch and display the IPv4/IPv6 address on the verbose #23
Closed
@gridhead gridhead self-requested a review October 17, 2020 04:03
@gridhead gridhead self-assigned this Oct 17, 2020
gridhead
gridhead requested changes Oct 17, 2020
View reviewed changes
Copy link
Owner

@gridhead gridhead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IP address specified here for both version 4 and version 6 won't be very helpful.

image

You would want to try another way of fetching the IP address - and not using the socket library.

@vinmay
Copy link
Contributor Author

vinmay commented Oct 17, 2020

@t0xic0der I can use another way to fetch the IP Address. I didn't understand why the above won't be helpful. Are you saying we want to display both IPv4 and IPv6 on verbose?

@gridhead
Copy link
Owner

@t0xic0der I can use another way to fetch the IP Address. I didn't understand why the above won't be helpful. Are you saying we want to display both IPv4 and IPv6 on verbose?

No no. I mean that in both the cases (for IPv4 and IPv6) - the address which is obtained by the socket module is 127.0.1.1 which in fact cannot be used by an external entity to connect as this is an internal address. Please use some other way to get valid IPv4 and IPv6 addresses.

@vinmay
Copy link
Contributor Author

vinmay commented Oct 19, 2020

@t0xic0der Ahh got it ! I am looking at this right now.

@gridhead
Copy link
Owner

@vinmay You better finish doing this before @shivangswain does 🤣

@vinmay
Copy link
Contributor Author

vinmay commented Oct 21, 2020

@t0xic0der Hey I checked in a solution for the ipV4, could you just check if that is what you are looking for ?

@gridhead
Copy link
Owner

Hi @vinmay, please rebase the branch of your fork to keep up with the latest changes made to main - and then make those changes. Let me know once you are done with that so that I can get into reviewing it.

@vinmay
Copy link
Contributor Author

vinmay commented Oct 21, 2020

@t0xic0der Done. Another question I had while working on this was, is the intention here to display the public IP address that the system is connected to ?

@gridhead
Copy link
Owner

@t0xic0der Done. Another question I had while working on this was, is the intention here to display the public IP address that the system is connected to ?

I don't quite follow. The purpose is to simply display the IP address of the system where the service is hosted from, so that folks joining the service would get to know which address to connect to.

@gridhead gridhead self-requested a review October 23, 2020 03:00
@gridhead gridhead assigned shivangswain and unassigned gridhead Oct 23, 2020
gridhead
gridhead requested changes Oct 23, 2020
View reviewed changes
Copy link
Owner

@gridhead gridhead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at the suggested changes.

main.py
Comment on lines +91 to +94
def getipaddr():
s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
s.connect(("8.8.8.8", 80))
return s.getsockname()[0]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IP address provided here would be restricted to only IPv4 addresses.

Take a look at this.

image

Even when IPv6 is selected, only the IPv4 address is displayed.

Also, the displayed IP address is not very useful when communicating across dissimilar networks as the address is applicable only in the intranet.

image

@gridhead
Copy link
Owner

Any updates, @vinmay?

@vinmay
Copy link
Contributor Author

vinmay commented Oct 30, 2020

@t0xic0der I haven't gone back to this yet. If someone else has a better solution in between, please feel free to accept theirs. I will be getting back on this from Monday for sure.

@gridhead
Copy link
Owner

Sure thing. @shivangswain is working on it. Take your time. 😄

@vinmay
Copy link
Contributor Author

vinmay commented Nov 6, 2020

@t0xic0der Should I just close this?

@shivangswain
Copy link
Collaborator

@t0xic0der Done. Another question I had while working on this was, is the intention here to display the public IP address that the system is connected to?

This was exactly the intention of the issue. Currently your implementation displays the IP assigned to the device running the host by the local router or switch and not the public IP address of the server. You can out check my implementation if you want to know how I did it.

@shivangswain shivangswain assigned vinmay and unassigned shivangswain Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gridhead gridhead gridhead requested changes

Assignees

@vinmay vinmay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fetch and display the IPv4/IPv6 address on the verbose
3 participants
@vinmay @gridhead @shivangswain