Adding detection of IP address with ifcfg==0.11b6 #1929

benjaoming · 2017-08-01T13:59:25Z

Summary

Please see my upstream changes (>1000 lines of code):

ftao/python-ifcfg#7
ftao/python-ifcfg#8

TODO

Have tests been written for the new code? (sort of, ifcfg is really well-tested)

Reviewer guidance

Please take over the PR in case issues come up - or feel free to mess with the code due to my absence the next 2 weeks.

Issues addressed

#1595

Screenshots (if appropriate)

codecov-io · 2017-08-01T14:31:53Z

Codecov Report

Merging #1929 into release-v0.5.x will decrease coverage by 0.02%.
The diff coverage is 72.22%.

@@                Coverage Diff                 @@
##           release-v0.5.x    #1929      +/-   ##
==================================================
- Coverage           74.01%   73.98%   -0.03%     
==================================================
  Files                 154      155       +1     
  Lines                5610     5639      +29     
  Branches              669      673       +4     
==================================================
+ Hits                 4152     4172      +20     
- Misses               1344     1352       +8     
- Partials              114      115       +1

Impacted Files	Coverage Δ
kolibri/utils/cli.py	`67.4% <70%> (+0.88%)`	⬆️
kolibri/utils/server.py	`48.38% <75%> (+5.05%)`	⬆️
kolibri/content/middleware.py	`81.81% <0%> (-18.19%)`	⬇️
kolibri/content/utils/channels.py	`33.87% <0%> (-1.73%)`	⬇️
kolibri/tasks/api.py	`40% <0%> (-0.7%)`	⬇️
kolibri/content/api.py	`51.91% <0%> (-0.29%)`	⬇️
frontend_build/src/parse_bundle_plugin.js	`89.28% <0%> (ø)`	⬆️
kolibri/deployment/default/settings/translation.py	`0% <0%> (ø)`
kolibri/utils/system.py	`36.58% <0%> (+3.65%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0570b57...df70efb. Read the comment docs.

rtibbles

I can make the small update I am suggesting here.

kolibri/utils/cli.py

+            "Could not detect an IP address that Kolibri binds to, but try "
+            "opening up the following addresses:\n")
+        urls = [
+            "http://{}:{}".format(ip, port) for ip in ("0.0.0.0", "127.0.0.1")


rtibbles · 2017-08-01T15:13:49Z

Will wait on testing feedback from @radinamatic, and then make the small tweak I suggest before merging.

radinamatic · 2017-08-02T18:23:20Z

On Windows 7 & 8 url detection seems to be working OK, 👍. There are some additional urls detected - not sure if that can be polished, or those are due to network adaptes on VMs:

However, Windows 10 has issues starting the server, and I see a bunch of ifcfg errors in the command prompt, so that might be the most likely culprit.

Ah, and kolibri url command is not implemented, yet - for some reason I thought it was supposed to be in this PR.

Anything else more specific that I need to test @rtibbles?

rtibbles · 2017-08-02T18:30:51Z

Looks good, @radinamatic - thanks, very thorough.

rtibbles · 2017-08-02T18:31:09Z

Seems like this might need some upstream work for Windows 10 compatibility?

benjaoming · 2017-08-02T22:45:21Z

@radinamatic can you send the raw output of ipconfig as run from the command line in this Win 10? Seems it contains characters that don't parse as UTF-8, but moreover there's a need for more investigation on how to handle the stdout of some command line output on any given system.

radinamatic · 2017-08-02T23:11:35Z

You should be enjoying Provance and herding sheep, not yearning raw command prompt outputs at this hour... 😛

Microsoft Windows [Version 10.0.14393]
(c) 2016 Microsoft Corporation. All rights reserved.

C:\Users\IEUser>ipconfig /all

Windows IP Configuration

   Host Name . . . . . . . . . . . . : MSEDGEWIN10
   Primary Dns Suffix  . . . . . . . :
   Node Type . . . . . . . . . . . . : Hybrid
   IP Routing Enabled. . . . . . . . : No
   WINS Proxy Enabled. . . . . . . . : No

Ethernet adapter Ethernet:

   Connection-specific DNS Suffix  . :
   Description . . . . . . . . . . . : Intel(R) PRO/1000 MT Desktop Adapter
   Physical Address. . . . . . . . . : 08-00-27-CC-BE-AF
   DHCP Enabled. . . . . . . . . . . : Yes
   Autoconfiguration Enabled . . . . : Yes
   Link-local IPv6 Address . . . . . : fe80::e59b:7afd:78a6:d073%4(Preferred)
   IPv4 Address. . . . . . . . . . . : 10.0.2.15(Preferred)
   Subnet Mask . . . . . . . . . . . : 255.255.255.0
   Lease Obtained. . . . . . . . . . : jueves, 3 de agosto de 2017 1:00:44
   Lease Expires . . . . . . . . . . : viernes, 4 de agosto de 2017 1:00:59
   Default Gateway . . . . . . . . . : 10.0.2.2
   DHCP Server . . . . . . . . . . . : 10.0.2.2
   DHCPv6 IAID . . . . . . . . . . . : 34078759
   DHCPv6 Client DUID. . . . . . . . : 00-01-00-01-1F-22-AC-9F-08-00-27-CC-BE-AF
   DNS Servers . . . . . . . . . . . : 89.150.129.22
                                       89.150.129.10
   NetBIOS over Tcpip. . . . . . . . : Enabled

Ethernet adapter Ethernet 2:

   Connection-specific DNS Suffix  . :
   Description . . . . . . . . . . . : Intel(R) PRO/1000 MT Desktop Adapter #2
   Physical Address. . . . . . . . . : 08-00-27-0D-9A-0B
   DHCP Enabled. . . . . . . . . . . : Yes
   Autoconfiguration Enabled . . . . : Yes
   Link-local IPv6 Address . . . . . : fe80::99e0:2790:ba60:20e1%5(Preferred)
   IPv4 Address. . . . . . . . . . . : 192.168.56.101(Preferred)
   Subnet Mask . . . . . . . . . . . : 255.255.255.0
   Lease Obtained. . . . . . . . . . : jueves, 3 de agosto de 2017 1:00:44
   Lease Expires . . . . . . . . . . : jueves, 3 de agosto de 2017 1:20:59
   Default Gateway . . . . . . . . . :
   DHCP Server . . . . . . . . . . . : 192.168.56.100
   DHCPv6 IAID . . . . . . . . . . . : 151519271
   DHCPv6 Client DUID. . . . . . . . : 00-01-00-01-1F-22-AC-9F-08-00-27-CC-BE-AF
   DNS Servers . . . . . . . . . . . : fec0:0:0:ffff::1%1
                                       fec0:0:0:ffff::2%1
                                       fec0:0:0:ffff::3%1
   NetBIOS over Tcpip. . . . . . . . : Enabled

Tunnel adapter isatap.{ADDEF65B-69BD-40F2-A0E6-4B67361ECE85}:

   Media State . . . . . . . . . . . : Media disconnected
   Connection-specific DNS Suffix  . :
   Description . . . . . . . . . . . : Microsoft ISATAP Adapter
   Physical Address. . . . . . . . . : 00-00-00-00-00-00-00-E0
   DHCP Enabled. . . . . . . . . . . : No
   Autoconfiguration Enabled . . . . : Yes

Tunnel adapter isatap.{BFDBE788-14F2-499A-9D83-CDCD34CDE463}:

   Media State . . . . . . . . . . . : Media disconnected
   Connection-specific DNS Suffix  . :
   Description . . . . . . . . . . . : Microsoft ISATAP Adapter #2
   Physical Address. . . . . . . . . : 00-00-00-00-00-00-00-E0
   DHCP Enabled. . . . . . . . . . . : No
   Autoconfiguration Enabled . . . . : Yes

C:\Users\IEUser>ipconfig /all | clip

C:\Users\IEUser>

benjaoming · 2017-08-17T14:46:28Z

@radinamatic the latest Buildkite assets are available for testing now - could you check if the encoding error on Windows has disappeared?

radinamatic · 2017-08-17T15:11:19Z

@benjaoming Sure, downloading now! 👍

radinamatic · 2017-08-17T17:26:28Z

All is well on Win10 front @benjaoming 💯

radinamatic

LGTM

benjaoming · 2017-08-17T20:57:25Z

Just wanted to share that I learned this insight about coverage reports. When it says:

Merging #1929 into release-v0.5.x will decrease coverage by 0.02%.

Notice that it references 0570b57 - meaning that it does the comparison to an older absolute metric.

My patch actually improves coverage (as you can read from the relative delta) - this often baffled me.

I have tried rebasing before, but as I recall, it doesn't change that CodeCove keeps comparing to the initial commit hash that the target branch had when opening the PR.

Coverage went up from 73.81% to 73.98% :)

Adding detection of IP address with ifcfg==0.11b4

15a9c6f

benjaoming added this to the 0.5.0 milestone Aug 1, 2017

benjaoming assigned rtibbles Aug 1, 2017

benjaoming requested a review from rtibbles August 1, 2017 13:59

rtibbles reviewed Aug 1, 2017

View reviewed changes

benjaoming added 5 commits August 17, 2017 13:29

Bump to more recent ifcfg==0.11b6

22423cf

Replace '0.0.0.0' with 'localhost' -- seems a decent fallback @rtibbles

b9afc83

Improve testing of cli.stop() for odd cases

7b46b1f

Use pytest-style testing, catch expected exceptions the pretty way

0fc90c8

When network interface isn't configured, don't display 'None' as IP

df70efb

benjaoming changed the title ~~Adding detection of IP address with ifcfg==0.11b4~~ Adding detection of IP address with ifcfg==0.11b6 Aug 17, 2017

radinamatic approved these changes Aug 17, 2017

View reviewed changes

rtibbles merged commit 66386a2 into learningequality:release-v0.5.x Aug 17, 2017

rtibbles mentioned this pull request Aug 17, 2017

Incorrect IP to stdout on startup #1595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding detection of IP address with ifcfg==0.11b6 #1929

Adding detection of IP address with ifcfg==0.11b6 #1929

benjaoming commented Aug 1, 2017

codecov-io commented Aug 1, 2017 •

edited

Loading

rtibbles left a comment

This comment was marked as spam.

This comment was marked as spam.

rtibbles commented Aug 1, 2017

radinamatic commented Aug 2, 2017 •

edited

Loading

rtibbles commented Aug 2, 2017

rtibbles commented Aug 2, 2017

benjaoming commented Aug 2, 2017

radinamatic commented Aug 2, 2017

benjaoming commented Aug 17, 2017

radinamatic commented Aug 17, 2017

radinamatic commented Aug 17, 2017

radinamatic left a comment

benjaoming commented Aug 17, 2017

Adding detection of IP address with ifcfg==0.11b6 #1929

Adding detection of IP address with ifcfg==0.11b6 #1929

Conversation

benjaoming commented Aug 1, 2017

Summary

TODO

Reviewer guidance

Issues addressed

Screenshots (if appropriate)

codecov-io commented Aug 1, 2017 • edited Loading

Codecov Report

rtibbles left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

rtibbles commented Aug 1, 2017

radinamatic commented Aug 2, 2017 • edited Loading

rtibbles commented Aug 2, 2017

rtibbles commented Aug 2, 2017

benjaoming commented Aug 2, 2017

radinamatic commented Aug 2, 2017

benjaoming commented Aug 17, 2017

radinamatic commented Aug 17, 2017

radinamatic commented Aug 17, 2017

radinamatic left a comment

Choose a reason for hiding this comment

benjaoming commented Aug 17, 2017

codecov-io commented Aug 1, 2017 •

edited

Loading

radinamatic commented Aug 2, 2017 •

edited

Loading