-
Notifications
You must be signed in to change notification settings - Fork 820
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
test: Add v2 test coverage to test_net.py #5247
Conversation
v4_and_v6 v1_ipv4_and_ipv6_static v2_ipv4_and_ipv6_static
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.) |
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just getting back to this after a long while, sorry for the delay.
Overall this looks good, thanks for the PR! I just have a couple of questions inline.
@@ -2265,41 +3495,41 @@ | |||
bond-downdelay 10 | |||
bond-fail-over-mac active | |||
bond-master bond0 | |||
bond_miimon 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change intentional? (repeat the same question for the next several changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Github unfortunately renders this very poorly. You're looking at the v2 addition, and when using v2, things get rendered alphabetically. This is a change between v1 and v2 rendering, but it results in no functional difference so it is fine.
This may better demonstrate the difference here. Notice the left has expanded the expected v1 whereas the right has expanded the expected v2:
@@ -2586,22 +3729,22 @@ | |||
method=manual | |||
may-fail=false | |||
address1=192.168.0.2/24 | |||
gateway=192.168.0.1 | |||
route1=10.1.3.0/24,192.168.0.3 | |||
route1=0.0.0.0/0,192.168.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change intentional? (repeat for the next several changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) | ||
self._compare_files_to_expected(entry[self.expected_name], found) | ||
self._assert_headers(found) | ||
expected_msg = ( | ||
"Network config: ignoring iface0 device-level mtu:8999" | ||
" because ipv4 subnet-level mtu:9000 provided." | ||
) | ||
assert expected_msg in caplog.text | ||
if yaml_file == "yaml_v1": | ||
assert expected_msg in caplog.text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No test for yaml_v2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We get no warning in v2 because we wind up testing slightly different things. v2 has no concept of interface level vs subnet level MTU, so that's not something we can test for in v2.
yaml v1:
version: 1
config:
- type: 'physical'
name: 'iface0'
mtu: 8999
subnets:
- type: static
address: 192.168.14.2/24
mtu: 9000
- type: static
address: 2001:1::1/64
mtu: 1500
yaml v2:
version: 2
ethernets:
iface0:
addresses:
- 192.168.14.2/24
- 2001:1::1/64
mtu: 9000
entry = NETWORK_CONFIGS[expected_name] | ||
yaml_name = "yaml" if "yaml" in entry else "yaml_v2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change intentional
I assume not, so setting this to "Request changes".
@holmanb , I believe I answered all of your questions. Ready for re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for responding to my questions @TheRealFalcon. Looks good to me!
Proposed Commit Message
Additional Context
I left the individual commits as they may (or may not) be more useful in reviewing. This PR should be squashed though.
Merge type