Add Arista/EOS support - #124
Conversation
loulecrivain
left a comment
loulecrivain
left a comment
There was a problem hiding this comment.
Hello Niklas,
Thank you very much for the contribution, and for the time you took to send it to us. I left a lot of comments so that it's easier for us to know what are the parts that need talking about. Unfortunately, we cannot merge it as-is, because of the following concerns:
- Impact on intermediate format
- Change of output regarding our production systems
- Impact on general business logic
Now, I know these changes were necessary for the port to Arista EOS. The issue with the proposed code is not 100% on you, as we know cosmo is not as extensible and flexible as we'd like.
We will discuss what we can do internally to address the extensibility issues and will come back to you afterwards. In the meantime, please hold on - you do not have to change your code following this review.
| def getInterfaces(self) -> list["InterfaceType"]: | ||
| return self.get("interfaces", []) | ||
|
|
||
| def deriveRouterIdFromLoopbackInterface(self) -> str | None: |
There was a problem hiding this comment.
this changes core functions from cosmo and modifies our production output.
I think it'd have been best to change the native deriveRouterId function, but I imagine you may have specific reasons for not doing so? (genuine question). If yes I'd be happy to give you my thoughts on these
| continue | ||
| device_name = interface["device"]["name"] | ||
| candidate_ip_addresses = [] | ||
| if child_interface: |
There was a problem hiding this comment.
I understand what you are trying to achieve, but business logic should be kept separated from fetching logic. I know we do have some code on top of fetching, but this is mainly for stitching stuff back together and fixing netbox-related data format issues.
| filter( | ||
| lambda l: l["family"]["value"] == 4, child_interface["ip_addresses"] | ||
| ), | ||
| filter(lambda l: l["family"]["value"] == 4, candidate_ip_addresses), |
There was a problem hiding this comment.
same comment as above
| filter( | ||
| lambda l: l["family"]["value"] == 6, child_interface["ip_addresses"] | ||
| ), | ||
| filter(lambda l: l["family"]["value"] == 6, candidate_ip_addresses), |
There was a problem hiding this comment.
same comment as above
| unnumbered_interface, | ||
| loopback_interface, | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Please create a separate arista data file instead of modifying this one on the fly, it's better to separate test data preparation and test code.
| "connected_devices_interface_list", [] | ||
| ) | ||
| elif 'starts_with: "lo"' in q: | ||
| elif 'i_starts_with: "lo"' in q: |
There was a problem hiding this comment.
If this is for compatibility with more recent netbox version, I'd suggest we maintain compatibility with both starts_with and i_starts_with.
| self._device = device | ||
| self._cosmo_config = cosmo_config | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
I understand what you are doing, but this code should belong to AbstractManufacturer isCompatibleWith method. Is there a specific reason why you had to add this workaround? Imo would be better by maybe adding missing getters to Netbox Types and then putting the rest of the code in the correct ABC method.
| and not is_management_interface | ||
| and not parent_interface.isLoopbackOrParentIsLoopback() | ||
| and not any([t.getTagName() == "disable_sampling" for t in parent_interface.getTags()]) | ||
| and not any( |
There was a problem hiding this comment.
you can use "disable_sampling" in parent_interface.getTags(), which is more concise 😉
| ) | ||
|
|
||
| @staticmethod | ||
| def interfaceHasTag(o: InterfaceType, name: str) -> bool: |
There was a problem hiding this comment.
unnecessary, you can use "tag_name" in parent_interface.getTags() construct
| ) | ||
|
|
||
| router_id = parent_device.deriveRouterIdFromLoopbackInterface() | ||
| if not router_id: |
There was a problem hiding this comment.
this code should belong in deriveRouterId. I don't think it's good that we have 2 methods for deriving router ID, this only makes the business logic more complicated.
3 participants
This commit adds Arista EOS Router support
Changes: