Skip to content

Conversation

@masnax
Copy link
Contributor

@masnax masnax commented Mar 15, 2024

This PR does 2 things:

  • Moves the DNS question to be after both the ipv4 and ipv6 questions. Right now it's in the middle, which feels awkward.
  • Because of the above, when choosing the default DNS nameservers, we only have the user's chosen IPv4 address at that point. By moving the question to the end, we can include both the chosen IPv4 and IPv6 addresses by default.

masnax added 2 commits March 15, 2024 22:55
…r DNS

Additionally, it moves the DNS question after asking both IPv4 and IPv6
questions, as having it in the middle feels awkward.

Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

LGTM!


gateways := []string{}
for gateway := range ipConfig {
gatewayAddr, _, err := net.ParseCIDR(gateway)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you run net.ParseCIDR here? Is it for the purpose of validating that the gateway is valid? I think the validation is already done in the step before when invoking AskString.

Below you again get the string from the net.IP and append it to the list of gateways. So could you just take the gateway from the map and append it to the list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, you want to extract the IP only without the subnetmask.

Copy link
Contributor

@gabrielmougard gabrielmougard left a comment

Choose a reason for hiding this comment

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

LGTM

@masnax masnax merged commit ed515e0 into canonical:main Apr 2, 2024
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.

3 participants