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

fix(app): display robot ip not robot ip subnet base #4411

Merged
merged 2 commits into from
Nov 8, 2019

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Nov 8, 2019

pr #4372 (ac74c12) added the netmask
package (https://www.npmjs.com/package/netmask) to parse the robot's networking
response of CIDR-format ip/subnetbits into a separate ip and subnet mask.
Unfortunately, that PR was building a Netmask object and then using its base
member to represent the IP address. However, a Netmask object only represents
information about the subnet, not about a specific ip; the base attribute is
the base address of the subnet, e.g. ip & subnetmask.

Instead, split off the CIDR suffix and use the result as the IP address.

Before:
Screen Shot 2019-11-08 at 2 10 11 PM

Note the IP is displayed as 10.10.0.0, the subnet base

After:
Screen Shot 2019-11-08 at 2 11 07 PM

Correct IP shown

pr #4372 (ac74c12) added the netmask
package (https://www.npmjs.com/package/netmask) to parse the robot's networking
response of CIDR-format ip/subnetbits into a separate ip and subnet mask.
Unfortunately, that PR was building a Netmask object and then using its base
member to represent the IP address. However, a Netmask object only represents
information _about the subnet_, not about a specific ip; the base attribute is
the base address of the subnet, e.g. ip & subnetmask.

Instead, split off the CIDR suffix and use the result as the IP address.
@sfoster1 sfoster1 added app Affects the `app` project fix PR fixes a bug labels Nov 8, 2019
@sfoster1 sfoster1 requested a review from a team November 8, 2019 19:17
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

👍 pending lint fixes

(sorry for being an idiot)

@codecov
Copy link

codecov bot commented Nov 8, 2019

Codecov Report

Merging #4411 into edge will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #4411   +/-   ##
=======================================
  Coverage   55.95%   55.95%           
=======================================
  Files         899      899           
  Lines       25522    25522           
=======================================
  Hits        14282    14282           
  Misses      11240    11240
Impacted Files Coverage Δ
app/src/http-api-client/networking.js 52.05% <0%> (ø) ⬆️

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 e1017f7...dc4ee7a. Read the comment docs.

@sfoster1
Copy link
Member Author

sfoster1 commented Nov 8, 2019

👍 pending lint fixes

(sorry for being an idiot)

no worries, should have seen it in review anyway

@sfoster1 sfoster1 merged commit 57cdfee into edge Nov 8, 2019
@sfoster1 sfoster1 deleted the app_correct-netmask-output branch November 8, 2019 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants