Fix: Description key not considered with connected endpoints #2745

pvinci-arista · 2023-04-19T20:43:51Z

Change Summary

description key under connected endpoints not used.

Related Issue(s)

Fixes #2744

Component(s) name

arista.avd.eos_designs

Proposed changes

How to test

molecule test

Checklist

User Checklist

N/A

Repository Checklist

My code has been rebased from devel before I start
I have read the CONTRIBUTING document.
My change requires a change to the documentation and documentation have been updated accordingly.
I have updated molecule CI testing accordingly. (check the box if not applicable)

gmuloc

Great catch - could you also please implement the same for Port-channels?

gmuloc · 2023-04-20T10:19:23Z

...tions/arista/avd/roles/eos_designs/python_modules/connected_endpoints/ethernet_interfaces.py

@@ -75,7 +75,7 @@ def _get_ethernet_interface_cfg(self, adapter: dict, node_index: int, connected_
            "peer_interface": peer_interface,
            "peer_type": connected_endpoint["type"],
            "port_profile": adapter.get("profile"),
-            "description": self.shared_utils.interface_descriptions.connected_endpoints_ethernet_interfaces(peer, peer_interface),
+            "description": adapter.get("description") or self.shared_utils.interface_descriptions.connected_endpoints_ethernet_interfaces(peer, peer_interface),


Suggested change

"description": adapter.get("description") or self.shared_utils.interface_descriptions.connected_endpoints_ethernet_interfaces(peer, peer_interface),

"description": default(adapter.get("description"), self.shared_utils.interface_descriptions.connected_endpoints_ethernet_interfaces(peer, peer_interface)),

gmuloc · 2023-04-20T10:20:01Z

...tions/arista/avd/molecule/eos_designs_unit_tests/inventory/host_vars/connected_endpoints.yml

+loopback_ipv4_pool: 192.168.0.0/24
+
+port_profiles:
+  common:
+    spanning_tree_portfast: edge
+    spanning_tree_bpduguard: true
+    storm_control:
+      all:
+        level: 1
+  desktop_phone-1234-2345:
+    parent_profile: common
+    mode: trunk phone
+    structured_config:
+      # profile: dot1x
+      phone:
+        trunk: tagged
+        vlan: "2345"
+      native_vlan: "1234"
+
+type: l2leaf
+l2leaf:
+  defaults:
+  nodes:
+    connected_endpoints:
+
+servers:
+  OLD_SW-1/4:
+    adapters:
+      - switches: [connected_endpoints]
+        switch_ports: [Ethernet4]
+        profile: desktop_phone-1234-2345
+        description: VVxx:abcd123
+        native_vlan: 2020
+        raw_eos_cli: |-
+          !! VoIP/PC


can you limit even further the test?

ClausHolbechArista · 2023-04-20T15:33:49Z

It was implemented as part of network_ports, where we want a more generic description, but makes perfect sense to include here as well. I would prefer to move the logic to the description module, so anyone customizing those keep full control. Similar to how it is done with port_channel_description.

gmuloc · 2023-04-20T15:49:40Z

It was implemented as part of network_ports, where we want a more generic description, but makes perfect sense to include here as well. I would prefer to move the logic to the description module, so anyone customizing those keep full control. Similar to how it is done with port_channel_description.

On this topic @ClausHolbechArista - should port-channel description also take into account this special description at the adapter level as currently it is doing <adapter_description>_<oprtional_port_channel_description>

ClausHolbechArista · 2023-04-20T16:18:32Z

It was implemented as part of network_ports, where we want a more generic description, but makes perfect sense to include here as well. I would prefer to move the logic to the description module, so anyone customizing those keep full control. Similar to how it is done with port_channel_description.

On this topic @ClausHolbechArista - should port-channel description also take into account this special description at the adapter level as currently it is doing <adapter_description>_<oprtional_port_channel_description>

That would be good

pvinci-arista · 2023-04-21T13:18:23Z

I would prefer to move the logic to the description module, so anyone customizing those keep full control. Similar to how it is done with port_channel_description.

If I follow, won't we need to modify the method signature in the base class and all the subclasses?

gmuloc · 2023-04-21T13:29:33Z

Hi @pvinci-arista so basically the suggestion is to add as an optional keyword argument the adapter.description key so that you don't break possible subclasses (though for now nobody has been using this functionality as it has not been officially released anyway).

Let's take the opportunity in this PR to also document better how the descriptions are built!

Let me know if you need some help

carlbuchmann

The fix looks good - As mentioned in one of the comments, we should add an explanation of how the descriptions are built for physical ports and port-channels.

pvinci-arista · 2023-04-27T20:04:47Z

we should add an explanation of how the descriptions are built for physical ports and port-channels.

Docstrings were added to the methods. Do you want something additional? See, as an example: https://github.com/aristanetworks/ansible-avd/pull/2745/files#diff-bbfc03125089c9310eb2bebb44079e7849bbf1e5031bae6df3024d280c0f306dR94

carlbuchmann · 2023-04-28T13:19:37Z

we should add an explanation of how the descriptions are built for physical ports and port-channels.

Docstrings were added to the methods. Do you want something additional? See, as an example: https://github.com/aristanetworks/ansible-avd/pull/2745/files#diff-bbfc03125089c9310eb2bebb44079e7849bbf1e5031bae6df3024d280c0f306dR94

We have added addition information in schema/documentation.

carlbuchmann

LGTM

…etworks#2745)

…2816) Co-authored-by: pvinci-arista <102165259+pvinci-arista@users.noreply.github.com>

pvinci-arista added 2 commits April 19, 2023 12:45

update connected endpoint description molecule test

bd50a3b

Description key not considered with connected endpoints

b7f4af9

pvinci-arista requested a review from a team as a code owner April 19, 2023 20:43

github-actions bot assigned pvinci-arista Apr 19, 2023

github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Apr 19, 2023

revert first test

b0cd30d

pvinci-arista changed the title ~~Description key not considered with connected endpoints~~ Fix: Description key not considered with connected endpoints Apr 19, 2023

fix pre-comit

6386418

pvinci-arista added the cherry-pick-for-3.8.x PR to be cherry-picked to releases/v3.8.x brach later label Apr 19, 2023

fix ethernet_interfaces.py

dc82d77

github-actions bot added the role: eos_designs issue related to eos_designs role label Apr 20, 2023

pvinci-arista added the type: bug Something isn't working label Apr 20, 2023

gmuloc reviewed Apr 20, 2023

View reviewed changes

code review comments; add tests

6eaba21

pvinci-arista and others added 2 commits April 20, 2023 14:32

Merge branch 'devel' into connected_descriptions

e4a84b1

fix precommit

a60b0ce

gmuloc closed this Apr 21, 2023

gmuloc reopened this Apr 21, 2023

pvinci-arista and others added 3 commits April 21, 2023 11:41

move logic into method

fd9a6b2

Merge branch 'devel' into connected_descriptions

9b75fda

update docstring

c687ce7

gmuloc mentioned this pull request Apr 24, 2023

Fix(eos_designs): Fix wrong error message for duplicates network_ports #2756

Merged

4 tasks

pvinci-arista and others added 2 commits April 24, 2023 18:39

pull in change for structure_config updates

e74cc13

Merge branch 'devel' into connected_descriptions

24e7b89

pvinci-arista requested a review from gmuloc April 26, 2023 13:20

gmuloc requested a review from a team April 27, 2023 16:11

carlbuchmann added this to the v4.0.0-dev11 milestone Apr 27, 2023

gmuloc approved these changes Apr 27, 2023

View reviewed changes

gmuloc requested a review from a team April 27, 2023 16:14

carlbuchmann requested changes Apr 27, 2023

View reviewed changes

add documention on key behavior

36d658a

github-actions bot added the state: Documentation role Updated label Apr 28, 2023

carlbuchmann self-requested a review April 28, 2023 13:19

carlbuchmann approved these changes Apr 28, 2023

View reviewed changes

gmuloc merged commit 13210ba into aristanetworks:devel Apr 28, 2023
36 checks passed

github-actions bot added the rn: Fix label Apr 28, 2023

carlbuchmann pushed a commit to carlbuchmann/avd that referenced this pull request May 12, 2023

Fix: Description key not considered with connected endpoints (aristan…

b8de14b

…etworks#2745)

carlbuchmann mentioned this pull request May 12, 2023

Fix: Description key not considered with connected endpoints (#2745) #2816

Merged

carlbuchmann removed the cherry-pick-for-3.8.x PR to be cherry-picked to releases/v3.8.x brach later label May 12, 2023

gmuloc pushed a commit that referenced this pull request May 12, 2023

Fix: Description key not considered with connected endpoints (#2745) (#…

6dded23

…2816) Co-authored-by: pvinci-arista <102165259+pvinci-arista@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Description key not considered with connected endpoints #2745

Fix: Description key not considered with connected endpoints #2745

pvinci-arista commented Apr 19, 2023 •

edited by carlbuchmann

gmuloc left a comment

gmuloc Apr 20, 2023

gmuloc Apr 20, 2023

ClausHolbechArista commented Apr 20, 2023

gmuloc commented Apr 20, 2023

ClausHolbechArista commented Apr 20, 2023

pvinci-arista commented Apr 21, 2023

gmuloc commented Apr 21, 2023 •

edited

carlbuchmann left a comment

pvinci-arista commented Apr 27, 2023

carlbuchmann commented Apr 28, 2023

carlbuchmann left a comment

	"description": adapter.get("description") or self.shared_utils.interface_descriptions.connected_endpoints_ethernet_interfaces(peer, peer_interface),
	"description": default(adapter.get("description"), self.shared_utils.interface_descriptions.connected_endpoints_ethernet_interfaces(peer, peer_interface)),

Fix: Description key not considered with connected endpoints #2745

Fix: Description key not considered with connected endpoints #2745

Conversation

pvinci-arista commented Apr 19, 2023 • edited by carlbuchmann

Change Summary

Related Issue(s)

Component(s) name

Proposed changes

How to test

Checklist

User Checklist

Repository Checklist

gmuloc left a comment

Choose a reason for hiding this comment

gmuloc Apr 20, 2023

Choose a reason for hiding this comment

gmuloc Apr 20, 2023

Choose a reason for hiding this comment

ClausHolbechArista commented Apr 20, 2023

gmuloc commented Apr 20, 2023

ClausHolbechArista commented Apr 20, 2023

pvinci-arista commented Apr 21, 2023

gmuloc commented Apr 21, 2023 • edited

carlbuchmann left a comment

Choose a reason for hiding this comment

pvinci-arista commented Apr 27, 2023

carlbuchmann commented Apr 28, 2023

carlbuchmann left a comment

Choose a reason for hiding this comment

pvinci-arista commented Apr 19, 2023 •

edited by carlbuchmann

gmuloc commented Apr 21, 2023 •

edited