)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"0c663b664bf3678cd1f91ee1d411489af10df3da","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Steve Relf \u003cs.relf@eschercloud.ai\u003e"},{"line_number":5,"context_line":"CommitDate: 2023-05-02 12:15:38 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Ensures that if a switch plugin has an empty plug/unplug bond to network"},{"line_number":8,"context_line":"function that the old code path of plug/unplug port to network is"},{"line_number":9,"context_line":"followed, otherwise this can result in a no-op happening"},{"line_number":10,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"f1841642_de395572","line":7,"updated":"2023-05-11 11:18:49.000000000","message":"Ideally the first line is a quick summary, like:\n\n\"Fix fallback to plug_port_to_network on 802_3ad bonds\"","commit_id":"ad583e7170b8ca06c838486d4989095e3e0cf45f"},{"author":{"_account_id":35976,"name":"Graeme Moss","display_name":"Graeme Moss","email":"graeme.moss@nscale.com","username":"gmoss"},"change_message_id":"e9a44210d7139630e93bcd3fff4a2885514803f6","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Steve Relf \u003cs.relf@eschercloud.ai\u003e"},{"line_number":5,"context_line":"CommitDate: 2023-05-02 12:15:38 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Ensures that if a switch plugin has an empty plug/unplug bond to network"},{"line_number":8,"context_line":"function that the old code path of plug/unplug port to network is"},{"line_number":9,"context_line":"followed, otherwise this can result in a no-op happening"},{"line_number":10,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"98cfd937_a42d82ec","line":7,"in_reply_to":"f1841642_de395572","updated":"2023-05-23 09:15:18.000000000","message":"Done","commit_id":"ad583e7170b8ca06c838486d4989095e3e0cf45f"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"0c663b664bf3678cd1f91ee1d411489af10df3da","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"036f70a7_f230c96e","updated":"2023-05-11 11:18:49.000000000","message":"Looks close, some nit picks and ideally we add a unit test for this please.","commit_id":"ad583e7170b8ca06c838486d4989095e3e0cf45f"},{"author":{"_account_id":35976,"name":"Graeme Moss","display_name":"Graeme Moss","email":"graeme.moss@nscale.com","username":"gmoss"},"change_message_id":"1388caf629a3ab8491070cdd91f36fcaf6f6fc15","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"a638d526_e2fcd80e","updated":"2023-05-23 15:03:29.000000000","message":"Gone over the test units and can find tests for the drivers that use bond and can find tests for the plug_bond_to_network. that was added in https://review.opendev.org/c/openstack/networking-generic-switch/+/844062","commit_id":"bbc95942634836a1076597edaf83d5373d1850cf"},{"author":{"_account_id":35976,"name":"Graeme Moss","display_name":"Graeme Moss","email":"graeme.moss@nscale.com","username":"gmoss"},"change_message_id":"89ea4268bd5d79fc9dcb783b662a3b9c2a182667","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"bb99b7f7_b0e469ff","updated":"2023-11-15 13:22:10.000000000","message":"Abandoning this due to it been fixed in the following merge https://review.opendev.org/c/openstack/networking-generic-switch/+/899460","commit_id":"7f77d1e4232b617e109d0b4c4fb1c2ab9b337cc3"}],"networking_generic_switch/generic_switch_mech.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"0c663b664bf3678cd1f91ee1d411489af10df3da","unresolved":true,"context_lines":[{"line_number":464,"context_line":"                           \u0027segmentation_id\u0027: segmentation_id})"},{"line_number":465,"context_line":"                # Move port to network"},{"line_number":466,"context_line":"                if is_802_3ad and hasattr(switch, \u0027plug_bond_to_network\u0027) \\"},{"line_number":467,"context_line":"                        and switch.PLUG_BOND_TO_NETWORK:"},{"line_number":468,"context_line":"                    switch.plug_bond_to_network(port_id, segmentation_id)"},{"line_number":469,"context_line":"                else:"},{"line_number":470,"context_line":"                    switch.plug_port_to_network(port_id, segmentation_id)"}],"source_content_type":"text/x-python","patch_set":1,"id":"23d3a569_ad8b0b1b","line":467,"updated":"2023-05-11 11:18:49.000000000","message":"hmm, yes, every drier has this method, so we need something on top:\nhttps://github.com/openstack/networking-generic-switch/blob/ec91cbb93ecc1eca52431f31ef0ae1317d98e87b/networking_generic_switch/devices/__init__.py#LL194C62-L194C62\n\nI think we should remove the hasattr check as it doesn\u0027t mean anything?\n\nIdeally we need to add a unit test to prove we call plug_port_to_network in this case.","commit_id":"ad583e7170b8ca06c838486d4989095e3e0cf45f"},{"author":{"_account_id":35976,"name":"Graeme Moss","display_name":"Graeme Moss","email":"graeme.moss@nscale.com","username":"gmoss"},"change_message_id":"1388caf629a3ab8491070cdd91f36fcaf6f6fc15","unresolved":false,"context_lines":[{"line_number":464,"context_line":"                           \u0027segmentation_id\u0027: segmentation_id})"},{"line_number":465,"context_line":"                # Move port to network"},{"line_number":466,"context_line":"                if is_802_3ad and hasattr(switch, \u0027plug_bond_to_network\u0027) \\"},{"line_number":467,"context_line":"                        and switch.PLUG_BOND_TO_NETWORK:"},{"line_number":468,"context_line":"                    switch.plug_bond_to_network(port_id, segmentation_id)"},{"line_number":469,"context_line":"                else:"},{"line_number":470,"context_line":"                    switch.plug_port_to_network(port_id, segmentation_id)"}],"source_content_type":"text/x-python","patch_set":1,"id":"e9dd63c6_cc5e7bed","line":467,"in_reply_to":"1b17fbc0_0be2fe76","updated":"2023-05-23 15:03:29.000000000","message":"So I\u0027ve gone over the test units and the function requested to test is in the test_generic_switch_mech.py\n\nfor example https://github.com/openstack/networking-generic-switch/blob/ec91cbb93ecc1eca52431f31ef0ae1317d98e87b/networking_generic_switch/tests/unit/test_generic_switch_mech.py#L551","commit_id":"ad583e7170b8ca06c838486d4989095e3e0cf45f"},{"author":{"_account_id":35976,"name":"Graeme Moss","display_name":"Graeme Moss","email":"graeme.moss@nscale.com","username":"gmoss"},"change_message_id":"e9a44210d7139630e93bcd3fff4a2885514803f6","unresolved":true,"context_lines":[{"line_number":464,"context_line":"                           \u0027segmentation_id\u0027: segmentation_id})"},{"line_number":465,"context_line":"                # Move port to network"},{"line_number":466,"context_line":"                if is_802_3ad and hasattr(switch, \u0027plug_bond_to_network\u0027) \\"},{"line_number":467,"context_line":"                        and switch.PLUG_BOND_TO_NETWORK:"},{"line_number":468,"context_line":"                    switch.plug_bond_to_network(port_id, segmentation_id)"},{"line_number":469,"context_line":"                else:"},{"line_number":470,"context_line":"                    switch.plug_port_to_network(port_id, segmentation_id)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1b17fbc0_0be2fe76","line":467,"in_reply_to":"23d3a569_ad8b0b1b","updated":"2023-05-23 09:15:18.000000000","message":"Tests units have a bond test in the test_culumus and it should have failed tests for all other drivers.\nIf anything this proves unit tests are broken as test_culumus tests for bond port by verifying the command but still passes with the wrong output. To fix the test units this should be a new review rather then adding to this fix.","commit_id":"ad583e7170b8ca06c838486d4989095e3e0cf45f"},{"author":{"_account_id":9542,"name":"Pavlo Shchelokovskyy","email":"pshchelokovskyy@mirantis.com","username":"pshchelo"},"change_message_id":"2bc93863028d066e018e11c266de17d82a520dda","unresolved":true,"context_lines":[{"line_number":463,"context_line":"                          {\u0027port_id\u0027: port_id, \u0027switch_info\u0027: switch_info,"},{"line_number":464,"context_line":"                           \u0027segmentation_id\u0027: segmentation_id})"},{"line_number":465,"context_line":"                # Move port to network"},{"line_number":466,"context_line":"                if is_802_3ad and switch.PLUG_BOND_TO_NETWORK:"},{"line_number":467,"context_line":"                    switch.plug_bond_to_network(port_id, segmentation_id)"},{"line_number":468,"context_line":"                else:"},{"line_number":469,"context_line":"                    switch.plug_port_to_network(port_id, segmentation_id)"}],"source_content_type":"text/x-python","patch_set":4,"id":"c0990e1a_381837b3","line":466,"updated":"2023-06-07 19:39:06.000000000","message":"this will work only for subclasses of NetmikoSwitch, while the current framework\nis not restricted to netmiko-based device plugins only.\n\nhttps://docs.openstack.org/networking-generic-switch/latest/contributing.html#creating-new-device-plugins\n\nFor non-NetmikoSwitch classes (if there are such out there in the wild) this will raise AttributeError.\n\nThe base class of GenericSwitch already has plug_bond_to_network realized\nhttps://opendev.org/openstack/networking-generic-switch/src/commit/5246705d24224d9c5cb6d9befd0542162c4ff737/networking_generic_switch/devices/__init__.py#L194-L200\nso indeed the current hasattr check seems to be useless, every device class should have it.\n\nMaking it a getattr instead is also not good as it effectively also binds implementation to NetmikoSwitch devices only.\n\nSo instead I\u0027d suggest in the NetmikoSwitch plug and unplug bond methods add a specific check for command provided, and if not - run the port plugging command as the base class does, e.g. smth like that:\n```\nif not self.PLUG_BOND_TO_NETWORK:\n   self.plug_port_to_network(...)\n```\nAnd here, replace the check with simple\n``\nif is_802_3ad:\n   swith.plug_bond_to_network()\n...\n```\nso that if the device plugin does not actually implemented a custom plug_bond OR the command is empty - the port plugging will be called instead still.\n\nNot sure how much sense calling the port plugging instead of bond plugging does though... if not, we\u0027d have to go a bit longer route, also adding a class attribute to the GenericSwith method indicating that the device supports bond (un)plugging and check for this attribute here.","commit_id":"7f77d1e4232b617e109d0b4c4fb1c2ab9b337cc3"}]}