[bug] Resolve bug and implement feature related to subnet division #842 #859
Conversation
Okay @nemesifier will add. |
| ) | ||
| } | ||
| ) | ||
| return |
There was a problem hiding this comment.
This is not the right approach: we must not cover up problematic code with more code. We must instead analyze, understand and fix the existing code which is causing the issue, which lies below.
| size=32, number_of_ips=1, number_of_subnets=1 | ||
| ) | ||
| rule.full_clean() | ||
| rule.save() |
There was a problem hiding this comment.
Calling save and full clean is not needed because those methods are already called by the logic in _get_vpn_subdivision_rule, please always read the logic of the methods and functions you call.
This is sufficient:
rule = self._get_vpn_subdivision_rule(
size=32, number_of_ips=1, number_of_subnets=1
)| rule.full_clean() | ||
| rule.save() | ||
| self.assertEqual(rule.size, 32) | ||
| self.assertEqual(rule.number_of_ips, 1) |
There was a problem hiding this comment.
These assertions here aren't really useful. The entire test suite of this submodule relies on _get_vpn_subdivision_rule to create subnet division rules so we can assume that size and number_of_ips will be correctly assigned.
In this test we need to add some code which triggers the execution of this rule at least once to ensure we successfully get a new /32 subnet with 1 ip address in it.
|
While reviewing this PR I found out quite a few more issues with the code and I realized that the time I would have needed to explain everything was a lot higher than just fixing it myself so I went ahead and took this over. @praptisharma28, please see my changes #860 and ask questions for any part you don't understand. |
3 participants
Fixes #842