Allow configuring ICMP redirects on a Vlan Interface #51
Conversation
|
I have mixed feelings about the TODOs in switch_base but felt this was the right place for this. I guess I could alternatively create an issue on github for the nomenclature violations. What do you guys think? I wanted to go and change the names (while keeping backwards compatibility), but I would rather do that in a different pull request. |
| self.real_switch.set_vlan_icmp_redirects_state(vlan_number, state) | ||
| try: | ||
| self.vlans_cache[vlan_number].icmp_redirects = state | ||
| except ValueError: |
There was a problem hiding this comment.
This try except is not needed
|
About the TODOs, every call not covered by the compliance test is "to do" and implementing it should force us to format the method name accordingly. That being said, we could have an issue as you said, make one pass adding the new method names and deprecating the other right now for later. I think raising the issue can't hurt |
|
But they're definately not part of this PR i'd appreciate if they were removed |
| @@ -26,3 +26,7 @@ def __init__(self, number=None, name=None, ips=None, vrrp_groups=None, vrf_forwa | |||
| self.ips = ips or [] | |||
| self.vrrp_groups = vrrp_groups or [] | |||
| self.dhcp_relay_servers = dhcp_relay_servers or [] | |||
| if icmp_redirects is not None: | |||
There was a problem hiding this comment.
This is an interesting point. The hardware we know have an icmp redirects state defaulting to True. But that might not always be the case...
The adapter which instantiate the vlan is responsible to know that, but it should now leave this empty, even if instantiated empty for ease of code.
Therefore i recommend not having a default value here, let the adapter set a value, the compliance test will ensure it does
|
There's a new attribute to the vlan, you should add it to the get_vlan compliance test |
| ### Unsetting | ||
| * Should be called unset_* | ||
| * Should not raise if not set | ||
|
|
There was a problem hiding this comment.
Is the unsetting really useful? Looking below, I see that we pass the state along side the set
ie: set_vlan_icmp_redirects_state(vlan_number, state) where state can be True or False. So what would be the use, in this Boolean context, of an unsetting method?
There was a problem hiding this comment.
To reset to vendor's default behavior
There was a problem hiding this comment.
We also have a bunch of "remove_" where what they really do is unset. See issue #52
|
👍 |
|
Appreciate the good feedback :) @stephanerobert what do you think? |
Instead, VLAN creation adds said VLAN to list of items to be refreshed.
|
👍 |
1 similar comment
|
👍 |
Allow configuring ICMP redirects on a Vlan Interface
No description provided.