)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"1becf6b42e0872dd1a30db905772e8af753f8b5a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3530e2f6_2ee4ca75","updated":"2024-11-11 07:55:46.000000000","message":"I\u0027m planning to backport this patch and the Neutron related one [1] up to 2023.1 before the unmaintenance cut.\n\n[1]https://review.opendev.org/c/openstack/neutron/+/934409","commit_id":"f72e2b6a7699bd85b295ad21164d55b34fa0080b"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b5b0759e2b2b0ec3b60850dd09916b25d9645cd4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"a3766088_6cab17d2","updated":"2024-11-18 20:54:12.000000000","message":"Still -1 from me because api.py needs to be updated with the API changes and documentation mentioned in my first review.\n\nI\u0027m ok with the weirdness w/ if_exist referring to the parent object since we do that in other places. It\u0027s not ideal, but if it\u0027s documented we can live with it.","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"6ef7741098145ecbe064b21f285c40324c646cf3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"284d64d9_36a158ee","updated":"2024-11-15 10:28:33.000000000","message":"ping fellow reviewers","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"7c5ff0705f48e2bbcb0b9100c5294087780501e0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3d87ee4b_761995d2","updated":"2024-11-18 13:35:17.000000000","message":"ping fellow reviewers","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"e410184ccd6d25300b9e7d008068425847316de2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3e517903_1e5e5348","in_reply_to":"a3766088_6cab17d2","updated":"2024-11-19 08:53:17.000000000","message":"Done","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"a61c5e77d8f5f54c37449754bec2eff10bcefcfd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"313f48a0_98f2fb77","updated":"2024-11-19 13:57:35.000000000","message":"looks ok","commit_id":"e2d8d39fa13fad175d60ec1affc1de280a396fb6"}],"ovsdbapp/schema/ovn_northbound/api.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"a92cb66c117ca17f59a2ec15b771350c71421ab1","unresolved":true,"context_lines":[{"line_number":146,"context_line":"        :param match:     The match rule"},{"line_number":147,"context_line":"        :type match:      string"},{"line_number":148,"context_line":"        :param if_exists: If True, don\u0027t fail if the parent logical switch"},{"line_number":149,"context_line":"                          containing the ACL doesn\u0027t exist"},{"line_number":150,"context_line":"        :type if_exists:  boolean"},{"line_number":151,"context_line":"        :returns:         :class:`Command` with no result"},{"line_number":152,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"7bdafa46_700bce3c","line":149,"updated":"2024-11-19 16:22:02.000000000","message":"Shall we document that passing `False` will not raise an exception in case there is an attempt to delete a non-existing ACL then?","commit_id":"e2d8d39fa13fad175d60ec1affc1de280a396fb6"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"a22fb2327ef908601a0c6d096aed5508318d62c1","unresolved":false,"context_lines":[{"line_number":146,"context_line":"        :param match:     The match rule"},{"line_number":147,"context_line":"        :type match:      string"},{"line_number":148,"context_line":"        :param if_exists: If True, don\u0027t fail if the parent logical switch"},{"line_number":149,"context_line":"                          containing the ACL doesn\u0027t exist"},{"line_number":150,"context_line":"        :type if_exists:  boolean"},{"line_number":151,"context_line":"        :returns:         :class:`Command` with no result"},{"line_number":152,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"34189be4_1f1895b5","line":149,"in_reply_to":"15a186c2_804b0224","updated":"2024-11-20 13:36:10.000000000","message":"Right, so I think if we provided such information to the callers it may be useful since there clearly is a discrepancy in how the parameter behaves for different calls? I\u0027m thinking about it in a way if I want to use that method and I look at the docstring it the behavior should be as clear as possible. Being explicit is better than assuming how things work implicitly. Anyways, just a suggestion.","commit_id":"e2d8d39fa13fad175d60ec1affc1de280a396fb6"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"167e7f146f93f022b8583ff1378a441214bfb4b6","unresolved":false,"context_lines":[{"line_number":146,"context_line":"        :param match:     The match rule"},{"line_number":147,"context_line":"        :type match:      string"},{"line_number":148,"context_line":"        :param if_exists: If True, don\u0027t fail if the parent logical switch"},{"line_number":149,"context_line":"                          containing the ACL doesn\u0027t exist"},{"line_number":150,"context_line":"        :type if_exists:  boolean"},{"line_number":151,"context_line":"        :returns:         :class:`Command` with no result"},{"line_number":152,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"57024353_3475e62b","line":149,"in_reply_to":"34189be4_1f1895b5","updated":"2024-11-21 09:41:33.000000000","message":"I\u0027m pushing an follow-up patch to update the docstrings of the API methods.","commit_id":"e2d8d39fa13fad175d60ec1affc1de280a396fb6"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"80a056a6a08c0ce09a2e788a3fab161cd3295a9f","unresolved":false,"context_lines":[{"line_number":146,"context_line":"        :param match:     The match rule"},{"line_number":147,"context_line":"        :type match:      string"},{"line_number":148,"context_line":"        :param if_exists: If True, don\u0027t fail if the parent logical switch"},{"line_number":149,"context_line":"                          containing the ACL doesn\u0027t exist"},{"line_number":150,"context_line":"        :type if_exists:  boolean"},{"line_number":151,"context_line":"        :returns:         :class:`Command` with no result"},{"line_number":152,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"15a186c2_804b0224","line":149,"in_reply_to":"7bdafa46_700bce3c","updated":"2024-11-20 08:03:47.000000000","message":"I don\u0027t think we should mix concepts here, to avoid confusions. This flag applies only to the parent resource, not the ACL itself. That should be explained in the method description.\n\nActually, as Terry mentioned, the behaviour of the API is not consistent. There are some deletion commands where the \"if_exists\" applies to the child resource (LrLbDelCommand), others where the same flag applies to the parent and child resource (HAChassisGroupDelChassisCommand); some deletion commands are idempotent (_AclDelHelper, QoSDelCommand), other will fail if the child resource doesn\u0027t exist (LrLbDelCommand, LrpDelGatewayChassisCommand)","commit_id":"e2d8d39fa13fad175d60ec1affc1de280a396fb6"}],"ovsdbapp/schema/ovn_northbound/commands.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d50ec0d7a41c44f79e7a09adc33d2f3157787f1a","unresolved":true,"context_lines":[{"line_number":226,"context_line":""},{"line_number":227,"context_line":"    def __init__(self, api, switch, direction\u003dNone,"},{"line_number":228,"context_line":"                 priority\u003dNone, match\u003dNone, if_exists\u003dFalse):"},{"line_number":229,"context_line":"        # NOTE: we\u0027re overriding the constructor here to not break any"},{"line_number":230,"context_line":"        # existing callers before we introduced Port Groups."},{"line_number":231,"context_line":"        super().__init__(api, switch, direction, priority, match, if_exists)"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"f7e63282_27fc85e2","line":230,"range":{"start_line":229,"start_character":0,"end_line":230,"end_character":60},"updated":"2024-11-08 12:57:36.000000000","message":"I don\u0027t understand why we override here - perhaps the supercall looked different before and now it\u0027s safe to remove instead of passing the parameter to the superclass?","commit_id":"ec480ad9ea907895911a3684a8d6da13951f3e72"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"b9054f39707233ebcd981ff78e3bc26b26190a62","unresolved":false,"context_lines":[{"line_number":226,"context_line":""},{"line_number":227,"context_line":"    def __init__(self, api, switch, direction\u003dNone,"},{"line_number":228,"context_line":"                 priority\u003dNone, match\u003dNone, if_exists\u003dFalse):"},{"line_number":229,"context_line":"        # NOTE: we\u0027re overriding the constructor here to not break any"},{"line_number":230,"context_line":"        # existing callers before we introduced Port Groups."},{"line_number":231,"context_line":"        super().__init__(api, switch, direction, priority, match, if_exists)"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"62781fbb_8a134f38","line":230,"range":{"start_line":229,"start_character":0,"end_line":230,"end_character":60},"in_reply_to":"f7e63282_27fc85e2","updated":"2024-11-08 15:13:37.000000000","message":"Both ``AclDelCommand`` and ``PgAclDelCommand`` have now the same ``__init__`` signature (actually I think that also happened in the original implementation if I\u0027m not wrong [1]). We can remove this super call.\n\n[1]https://review.opendev.org/c/openstack/ovsdbapp/+/571651","commit_id":"ec480ad9ea907895911a3684a8d6da13951f3e72"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"7bd7f6b3ae7c8d57f1a58fa4afb9889eac63943f","unresolved":true,"context_lines":[{"line_number":210,"context_line":"        try:"},{"line_number":211,"context_line":"            entity \u003d self.api.lookup(self.lookup_table, self.entity)"},{"line_number":212,"context_line":"        except idlutils.RowNotFound as e:"},{"line_number":213,"context_line":"            if self.if_exists:"},{"line_number":214,"context_line":"                return"},{"line_number":215,"context_line":"            msg \u003d \"%s %s does not exist\" % (self.lookup_table, self.entity)"},{"line_number":216,"context_line":"            raise RuntimeError(msg) from e"}],"source_content_type":"text/x-python","patch_set":3,"id":"1067c06a_0c187789","line":213,"updated":"2024-11-11 17:32:22.000000000","message":"It seems a little confusing to have acl_del(if_exists\u003dTrue) mean \"Delete this ACL if the underlying port group or logical switch exists\" as opposed to \"delete this ACL if it exists\".\n\nI guess we can assume that if the port group or switch doesn\u0027t exist, the ACL also doesn\u0027t exist, and the existing behavior with respect to ACLs on these entities already behaves as if_exists were True (we don\u0027t raise an error if there is no match). But in the case of the new default value of if_exists\u003dFalse, does that mean that if we go through the list of entity.acls and we don\u0027t match any ACLs that we *should* throw an error?\n\nI\u0027m open to this change, just checking to make sure we\u0027ve covered everything. Regardless, `ovsdbapp/schema/ovn_northbound/api.py` needs to be updated to match the impl and have the behavior documented.","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"3c90e77b801ac9548cbfe08735e551ce5e3dbe28","unresolved":true,"context_lines":[{"line_number":210,"context_line":"        try:"},{"line_number":211,"context_line":"            entity \u003d self.api.lookup(self.lookup_table, self.entity)"},{"line_number":212,"context_line":"        except idlutils.RowNotFound as e:"},{"line_number":213,"context_line":"            if self.if_exists:"},{"line_number":214,"context_line":"                return"},{"line_number":215,"context_line":"            msg \u003d \"%s %s does not exist\" % (self.lookup_table, self.entity)"},{"line_number":216,"context_line":"            raise RuntimeError(msg) from e"}],"source_content_type":"text/x-python","patch_set":3,"id":"c42df17d_01611e0d","line":213,"in_reply_to":"1067c06a_0c187789","updated":"2024-11-11 17:36:29.000000000","message":"oh .. i missed that. is it intentional?","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"e410184ccd6d25300b9e7d008068425847316de2","unresolved":false,"context_lines":[{"line_number":210,"context_line":"        try:"},{"line_number":211,"context_line":"            entity \u003d self.api.lookup(self.lookup_table, self.entity)"},{"line_number":212,"context_line":"        except idlutils.RowNotFound as e:"},{"line_number":213,"context_line":"            if self.if_exists:"},{"line_number":214,"context_line":"                return"},{"line_number":215,"context_line":"            msg \u003d \"%s %s does not exist\" % (self.lookup_table, self.entity)"},{"line_number":216,"context_line":"            raise RuntimeError(msg) from e"}],"source_content_type":"text/x-python","patch_set":3,"id":"d262e8e2_80c0e32c","line":213,"in_reply_to":"559141aa_eb2b7f92","updated":"2024-11-19 08:53:17.000000000","message":"And that would be correct. If the PG doesn\u0027t exist, then the ACL cannot exist neither. Then the delete command will succeed because the ACL (and the parent PG in this case) is no longer in the system.","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"d7996cc3f2b03cbcd7e3b9804e6c7ad4a915d8dc","unresolved":false,"context_lines":[{"line_number":210,"context_line":"        try:"},{"line_number":211,"context_line":"            entity \u003d self.api.lookup(self.lookup_table, self.entity)"},{"line_number":212,"context_line":"        except idlutils.RowNotFound as e:"},{"line_number":213,"context_line":"            if self.if_exists:"},{"line_number":214,"context_line":"                return"},{"line_number":215,"context_line":"            msg \u003d \"%s %s does not exist\" % (self.lookup_table, self.entity)"},{"line_number":216,"context_line":"            raise RuntimeError(msg) from e"}],"source_content_type":"text/x-python","patch_set":3,"id":"d34e4962_25a96aa2","line":213,"in_reply_to":"c42df17d_01611e0d","updated":"2024-11-12 07:34:03.000000000","message":"This method was idempotent initially. The loop in L218 we match the existing ACLs with the provided conditions (direction, priority and match). We only delete these ACLs matching them.\n\nThis check makes sense, as in other commands, if we first search (and fail) the parent resource. The flag \"if_exists\" tries basically to prevent raising an exception when executing the command.\n\nThere are other commands where the \"if_exists\" flag applies to the parent resource. For example LSGetLocalnetPortsCommand, QoSDelCommand, PgAddPortCommand, PgDelPortCommand (this is only in the NB commands).","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"eeaf831ed0de635449126176812cc411cc755fa2","unresolved":false,"context_lines":[{"line_number":210,"context_line":"        try:"},{"line_number":211,"context_line":"            entity \u003d self.api.lookup(self.lookup_table, self.entity)"},{"line_number":212,"context_line":"        except idlutils.RowNotFound as e:"},{"line_number":213,"context_line":"            if self.if_exists:"},{"line_number":214,"context_line":"                return"},{"line_number":215,"context_line":"            msg \u003d \"%s %s does not exist\" % (self.lookup_table, self.entity)"},{"line_number":216,"context_line":"            raise RuntimeError(msg) from e"}],"source_content_type":"text/x-python","patch_set":3,"id":"559141aa_eb2b7f92","line":213,"in_reply_to":"d34e4962_25a96aa2","updated":"2024-11-18 14:26:24.000000000","message":"I think what Terry is saying is that if you pass `if_exists\u003dFalse` and you pass conditions for an ACL that does not exist then you will not get any exception and it will \"succeed\" even though the ACL does not exist and `if_exists\u003dFalse`","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"}],"ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"5a9bc101c434d779481d1597f0bc52faff38e2b9","unresolved":true,"context_lines":[{"line_number":262,"context_line":"        self.assertNotIn(r1.uuid, self.api.tables[\u0027ACL\u0027].rows)"},{"line_number":263,"context_line":"        self.assertEqual([], self.port_group.acls)"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def test_pg_acl_del_if_exists(self):"},{"line_number":266,"context_line":"        cmd \u003d self.api.pg_acl_del(\u0027port_group2\u0027)"},{"line_number":267,"context_line":"        self.assertRaises(RuntimeError, cmd.execute, check_error\u003dTrue)"},{"line_number":268,"context_line":"        self.assertIsNone("}],"source_content_type":"text/x-python","patch_set":2,"id":"4c5ae2ae_6245240e","line":265,"range":{"start_line":265,"start_character":8,"end_line":265,"end_character":33},"updated":"2024-11-11 13:54:47.000000000","message":"nit: It might be a good idea to split this into two tests so in case the error is not raised without `if_exists` we would still cover the case where `if_exists` was passed and also it would be clearer in the test output which case failed.","commit_id":"f72e2b6a7699bd85b295ad21164d55b34fa0080b"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"fa565076b7c41869e0b034e50173354a2544d74f","unresolved":false,"context_lines":[{"line_number":262,"context_line":"        self.assertNotIn(r1.uuid, self.api.tables[\u0027ACL\u0027].rows)"},{"line_number":263,"context_line":"        self.assertEqual([], self.port_group.acls)"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def test_pg_acl_del_if_exists(self):"},{"line_number":266,"context_line":"        cmd \u003d self.api.pg_acl_del(\u0027port_group2\u0027)"},{"line_number":267,"context_line":"        self.assertRaises(RuntimeError, cmd.execute, check_error\u003dTrue)"},{"line_number":268,"context_line":"        self.assertIsNone("}],"source_content_type":"text/x-python","patch_set":2,"id":"78b1cdaf_0eb5504a","line":265,"range":{"start_line":265,"start_character":8,"end_line":265,"end_character":33},"in_reply_to":"4c5ae2ae_6245240e","updated":"2024-11-11 16:22:00.000000000","message":"Done","commit_id":"f72e2b6a7699bd85b295ad21164d55b34fa0080b"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"9932f28623c119a6096afbbfc4b8f54ae3daaba0","unresolved":true,"context_lines":[{"line_number":217,"context_line":"        self.assertRaises(TypeError, self.api.acl_del, self.switch.uuid,"},{"line_number":218,"context_line":"                          priority\u003d0)"},{"line_number":219,"context_line":""},{"line_number":220,"context_line":"    def test_acl_del_if_exists_false(self):"},{"line_number":221,"context_line":"        cmd \u003d self.api.acl_del(\u0027lswitch2\u0027, \u0027from-lport\u0027, 0, \u0027match\u0027)"},{"line_number":222,"context_line":"        self.assertRaises(RuntimeError, cmd.execute, check_error\u003dTrue)"},{"line_number":223,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"daf902c2_478d7faa","line":220,"updated":"2024-11-18 14:28:07.000000000","message":"I think to cover the scenario we are discussing, you should have another test method pretty much the same as this one but pass `self.switch.uuid` instead of `lswitch2` and it should raise an error.","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"58fa965e55a0175685682e14da1a8d5cad34ea14","unresolved":false,"context_lines":[{"line_number":217,"context_line":"        self.assertRaises(TypeError, self.api.acl_del, self.switch.uuid,"},{"line_number":218,"context_line":"                          priority\u003d0)"},{"line_number":219,"context_line":""},{"line_number":220,"context_line":"    def test_acl_del_if_exists_false(self):"},{"line_number":221,"context_line":"        cmd \u003d self.api.acl_del(\u0027lswitch2\u0027, \u0027from-lport\u0027, 0, \u0027match\u0027)"},{"line_number":222,"context_line":"        self.assertRaises(RuntimeError, cmd.execute, check_error\u003dTrue)"},{"line_number":223,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bb001076_0ea96124","line":220,"in_reply_to":"daf902c2_478d7faa","updated":"2024-11-19 09:22:22.000000000","message":"That\u0027s not correct. The \"if_exists\" flag applies to the parent resource (PG or LS). Before and after this patch, if the PG or the LS exist but not the ACL, the command succeeds with no error.\n\nI\u0027ll create a new test but testing the condition I\u0027ve described.","commit_id":"fae0618669ad1d7433ae29e0868116eac067fbb0"}]}