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

Prefer ipv4 for default_gateway over ipv6 on windows #1231

Conversation

Nimesh-Msys
Copy link
Contributor

@Nimesh-Msys Nimesh-Msys commented Aug 10, 2018

  • Also added a minor fix to select default_interface based on the least metric value
  • Added Test cases
  • Fixes: MSYS-875

Signed-off-by: Nimesh [email protected]

Description

According to previous logic, there was an iteration over all the interfaces and the value of default_gateway and default_interface was getting reset in each loop. Hence the last interface was chosen as default_interface and so its default_gateway.
default_gateway was selected as the first one out from the list of default_ip_gateway of an interface, hence there was a possibility that if this list contain IPV6 address at former place, then it was selected.

In case of multiple Interfaces, priority for default_interface should be given to the interface with least metric value. Also, we need to prefer IPV4 value of default_gateway over IPV6

Issues Resolved

#746

Check List

@Nimesh-Msys Nimesh-Msys requested a review from a team August 10, 2018 12:20
@Nimesh-Msys Nimesh-Msys changed the title Prefer ipv4 for default_gateway over ipv6 on windows [In Progress] Prefer ipv4 for default_gateway over ipv6 on windows Aug 13, 2018
@Nimesh-Msys Nimesh-Msys force-pushed the nimesh/MSYS-875_prefer_ipv4_default_gateway_windows branch 4 times, most recently from d894697 to 34fa857 Compare August 16, 2018 07:22
@Nimesh-Msys Nimesh-Msys changed the title [In Progress] Prefer ipv4 for default_gateway over ipv6 on windows Prefer ipv4 for default_gateway over ipv6 on windows Aug 16, 2018
Copy link

@rhass rhass left a comment

Choose a reason for hiding this comment

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

This looks great!

thumbs up


# it "Failed example !!!" do
# expect(plugin.favored_ip).to eq('b')
# end
Copy link

Choose a reason for hiding this comment

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

Unless there is a compelling reason to keep this, we should remove the commented out example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. That was a typo. Removing.

@Nimesh-Msys Nimesh-Msys force-pushed the nimesh/MSYS-875_prefer_ipv4_default_gateway_windows branch from 34fa857 to 4c825be Compare August 17, 2018 06:07
@@ -53,6 +53,42 @@ def msft_adapter_data
data
end

# Returns the interface code in Hexadecimal Format
Copy link
Contributor

Choose a reason for hiding this comment

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

YARD format would be helpful here in the future

Copy link
Contributor Author

@Nimesh-Msys Nimesh-Msys Aug 21, 2018

Choose a reason for hiding this comment

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

I've updated them. Could you please take a look.
Thanks

Copy link
Contributor

@tas50 tas50 left a comment

Choose a reason for hiding this comment

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

Can you go back and update the method comments to be in YARD format so we can get proper documentation in websites that use YARD.

https://gist.github.com/chetan/1827484

- Minor fix to select default_interface based on the least metric value
- Fix to prefer default_fateway in IPV4 Format
- DRY-UP to get interface_code
- Commented as per YARD format
- Added test cases
- Fixes: MSYS-875

Signed-off-by: Nimesh <[email protected]>
@Nimesh-Msys Nimesh-Msys force-pushed the nimesh/MSYS-875_prefer_ipv4_default_gateway_windows branch from 4c825be to a908f9f Compare August 21, 2018 08:52
@tas50 tas50 merged commit 42e7566 into chef:master Aug 21, 2018
@tas50
Copy link
Contributor

tas50 commented Aug 21, 2018

Thanks for the comments @Nimesh-Msys

@lock
Copy link

lock bot commented Jan 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants