)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"3bd80248805515854c9a00fe821729966a401789","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"2f42d374_42585f03","updated":"2022-11-16 16:40:37.000000000","message":"Some nits and questions.","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"b21f6f4f46d62516fa7bf74a368f0261271ad0cc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"a18927ae_b620666c","updated":"2022-11-18 13:56:48.000000000","message":"Thanks for the reviews. I updated a new patch addressing the issues.","commit_id":"4d6df4bb6fe942bd6e05088d030b49a65dd7056f"}],"neutron/services/logapi/drivers/ovn/driver.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"3bd80248805515854c9a00fe821729966a401789","unresolved":true,"context_lines":[{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        \"\"\""},{"line_number":315,"context_line":""},{"line_number":316,"context_line":"        if not log_obj.enabled:"},{"line_number":317,"context_line":"            pgs \u003d self._pgs_from_log_obj(context, log_obj)"},{"line_number":318,"context_line":"            other_logs \u003d [log for log in self._get_logs(context)"},{"line_number":319,"context_line":"                          if log.id !\u003d log_obj.id and log.enabled]"}],"source_content_type":"text/x-python","patch_set":3,"id":"4fc0069f_dbae97b2","line":316,"range":{"start_line":316,"start_character":8,"end_line":316,"end_character":31},"updated":"2022-11-16 16:40:37.000000000","message":"To avoid indenting the whole method, this could be:\n\nif log_obj.enabled:\n    return True\n    \n    \nActually this is another question I have: if log_obj.enabled, what should be the output? Please, in the method description add the output parameter description. If the output is True/False, make it explicit in if log_obj.enabled.","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"b21f6f4f46d62516fa7bf74a368f0261271ad0cc","unresolved":false,"context_lines":[{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        \"\"\""},{"line_number":315,"context_line":""},{"line_number":316,"context_line":"        if not log_obj.enabled:"},{"line_number":317,"context_line":"            pgs \u003d self._pgs_from_log_obj(context, log_obj)"},{"line_number":318,"context_line":"            other_logs \u003d [log for log in self._get_logs(context)"},{"line_number":319,"context_line":"                          if log.id !\u003d log_obj.id and log.enabled]"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb2b426_1ba50ff1","line":316,"range":{"start_line":316,"start_character":8,"end_line":316,"end_character":31},"in_reply_to":"4fc0069f_dbae97b2","updated":"2022-11-18 13:56:48.000000000","message":"I have updated the docstring with the answer: :returns: True if there were other logs enabled, otherwise False.","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"3bd80248805515854c9a00fe821729966a401789","unresolved":true,"context_lines":[{"line_number":317,"context_line":"            pgs \u003d self._pgs_from_log_obj(context, log_obj)"},{"line_number":318,"context_line":"            other_logs \u003d [log for log in self._get_logs(context)"},{"line_number":319,"context_line":"                          if log.id !\u003d log_obj.id and log.enabled]"},{"line_number":320,"context_line":"            if other_logs:"},{"line_number":321,"context_line":""},{"line_number":322,"context_line":"                if log_obj.event \u003d\u003d log_const.ALL_EVENT:"},{"line_number":323,"context_line":"                    acls_to_check \u003d pgs[0][\"acls\"].copy()"}],"source_content_type":"text/x-python","patch_set":3,"id":"8c3f542a_9cb644de","line":320,"range":{"start_line":320,"start_character":12,"end_line":320,"end_character":26},"updated":"2022-11-16 16:40:37.000000000","message":"Same here:\n\nif not other_logs:\n    return False","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"b21f6f4f46d62516fa7bf74a368f0261271ad0cc","unresolved":false,"context_lines":[{"line_number":317,"context_line":"            pgs \u003d self._pgs_from_log_obj(context, log_obj)"},{"line_number":318,"context_line":"            other_logs \u003d [log for log in self._get_logs(context)"},{"line_number":319,"context_line":"                          if log.id !\u003d log_obj.id and log.enabled]"},{"line_number":320,"context_line":"            if other_logs:"},{"line_number":321,"context_line":""},{"line_number":322,"context_line":"                if log_obj.event \u003d\u003d log_const.ALL_EVENT:"},{"line_number":323,"context_line":"                    acls_to_check \u003d pgs[0][\"acls\"].copy()"}],"source_content_type":"text/x-python","patch_set":3,"id":"0f155926_a2b5c463","line":320,"range":{"start_line":320,"start_character":12,"end_line":320,"end_character":26},"in_reply_to":"8c3f542a_9cb644de","updated":"2022-11-18 13:56:48.000000000","message":"thanks!","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"3bd80248805515854c9a00fe821729966a401789","unresolved":true,"context_lines":[{"line_number":324,"context_line":"                    for log in other_logs:"},{"line_number":325,"context_line":"                        for acl in self._pgs_from_log_obj(context,"},{"line_number":326,"context_line":"                                                          log)[0][\"acls\"]:"},{"line_number":327,"context_line":"                            if not acls_to_check:"},{"line_number":328,"context_line":"                                return True"},{"line_number":329,"context_line":"                            elif acl in acls_to_check:"},{"line_number":330,"context_line":"                                acls_to_check.remove(acl)"}],"source_content_type":"text/x-python","patch_set":3,"id":"a4e7f481_ba6dffe6","line":327,"range":{"start_line":327,"start_character":28,"end_line":327,"end_character":49},"updated":"2022-11-16 16:40:37.000000000","message":"Why don\u0027t you make this check just after assigning the value? After L323.","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"b21f6f4f46d62516fa7bf74a368f0261271ad0cc","unresolved":true,"context_lines":[{"line_number":324,"context_line":"                    for log in other_logs:"},{"line_number":325,"context_line":"                        for acl in self._pgs_from_log_obj(context,"},{"line_number":326,"context_line":"                                                          log)[0][\"acls\"]:"},{"line_number":327,"context_line":"                            if not acls_to_check:"},{"line_number":328,"context_line":"                                return True"},{"line_number":329,"context_line":"                            elif acl in acls_to_check:"},{"line_number":330,"context_line":"                                acls_to_check.remove(acl)"}],"source_content_type":"text/x-python","patch_set":3,"id":"22042e69_596e40c6","line":327,"range":{"start_line":327,"start_character":28,"end_line":327,"end_character":49},"in_reply_to":"a4e7f481_ba6dffe6","updated":"2022-11-18 13:56:48.000000000","message":"Because I\u0027m removing items on that list to make this check work (Line 331). And I thought it was not worth it to duplicate it. But I can put it in both sides if that\u0027s better.","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"3bd80248805515854c9a00fe821729966a401789","unresolved":true,"context_lines":[{"line_number":354,"context_line":"        LOG.debug(\"Update_log %s\", log_obj)"},{"line_number":355,"context_line":""},{"line_number":356,"context_line":"        with self.ovn_nb.transaction(check_error\u003dTrue) as ovn_txn:"},{"line_number":357,"context_line":"            disabled_log \u003d self._unset_disabled_acls(context, log_obj, ovn_txn)"},{"line_number":358,"context_line":""},{"line_number":359,"context_line":"            if not disabled_log:"},{"line_number":360,"context_line":"                pgs \u003d self._pgs_from_log_obj(context, log_obj)"},{"line_number":361,"context_line":"                actions_enabled \u003d self._acl_actions_enabled(log_obj)"},{"line_number":362,"context_line":"                self._set_acls_log(pgs, ovn_txn, actions_enabled,"}],"source_content_type":"text/x-python","patch_set":3,"id":"183e93ba_242fe180","line":359,"range":{"start_line":357,"start_character":12,"end_line":359,"end_character":32},"updated":"2022-11-16 16:40:37.000000000","message":"nit::\n\nif not self._unset_disabled_acls(context, log_obj, ovn_txn):\n   xxxxx","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"b21f6f4f46d62516fa7bf74a368f0261271ad0cc","unresolved":false,"context_lines":[{"line_number":354,"context_line":"        LOG.debug(\"Update_log %s\", log_obj)"},{"line_number":355,"context_line":""},{"line_number":356,"context_line":"        with self.ovn_nb.transaction(check_error\u003dTrue) as ovn_txn:"},{"line_number":357,"context_line":"            disabled_log \u003d self._unset_disabled_acls(context, log_obj, ovn_txn)"},{"line_number":358,"context_line":""},{"line_number":359,"context_line":"            if not disabled_log:"},{"line_number":360,"context_line":"                pgs \u003d self._pgs_from_log_obj(context, log_obj)"},{"line_number":361,"context_line":"                actions_enabled \u003d self._acl_actions_enabled(log_obj)"},{"line_number":362,"context_line":"                self._set_acls_log(pgs, ovn_txn, actions_enabled,"}],"source_content_type":"text/x-python","patch_set":3,"id":"460f5978_43d63700","line":359,"range":{"start_line":357,"start_character":12,"end_line":359,"end_character":32},"in_reply_to":"183e93ba_242fe180","updated":"2022-11-18 13:56:48.000000000","message":"Done","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"3dfbf377a7f7226eaa8fda6421ec699cfa8ca35d","unresolved":true,"context_lines":[{"line_number":327,"context_line":"                for acl in self._pgs_from_log_obj(context,"},{"line_number":328,"context_line":"                                                  log)[0][\"acls\"]:"},{"line_number":329,"context_line":"                    if not acls_to_check:"},{"line_number":330,"context_line":"                        return True"},{"line_number":331,"context_line":"                    elif acl in acls_to_check:"},{"line_number":332,"context_line":"                        acls_to_check.remove(acl)"},{"line_number":333,"context_line":"            acls_to_remove \u003d [{\"name\": pgs[0][\"name\"],"}],"source_content_type":"text/x-python","patch_set":4,"id":"b5bc9cdb_d7b63563","line":330,"updated":"2022-11-22 11:18:51.000000000","message":"here You are returining True but actually nothing was removed/unset yet. Is that correct?\nAlso, this check don\u0027t need to be in the for loop, it can be in L326 probably","commit_id":"4d6df4bb6fe942bd6e05088d030b49a65dd7056f"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"5a97fa4eec299f55815c6282b9e61985ad416345","unresolved":true,"context_lines":[{"line_number":327,"context_line":"                for acl in self._pgs_from_log_obj(context,"},{"line_number":328,"context_line":"                                                  log)[0][\"acls\"]:"},{"line_number":329,"context_line":"                    if not acls_to_check:"},{"line_number":330,"context_line":"                        return True"},{"line_number":331,"context_line":"                    elif acl in acls_to_check:"},{"line_number":332,"context_line":"                        acls_to_check.remove(acl)"},{"line_number":333,"context_line":"            acls_to_remove \u003d [{\"name\": pgs[0][\"name\"],"}],"source_content_type":"text/x-python","patch_set":4,"id":"e46f4b70_14ee3bb4","line":330,"in_reply_to":"b5bc9cdb_d7b63563","updated":"2022-11-22 11:45:14.000000000","message":"I return true if the log object was set to disabled, not if there was something unset. This is because we can encounter moments when we do NOT want the disabled object to disable any ACLs (because there are other log objects using them). That is why the return is inside the loop. If you see L332, I use the loop to remove ACLs that are in acls_to_check.\n\nThe return True is placed there to stop checking each acl in each log if we already know that this should not change anything (because all acls are related to other objects) If in the end there are ACLs remaning, those are the ones that will be disabled.","commit_id":"4d6df4bb6fe942bd6e05088d030b49a65dd7056f"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"ce28f1f4ef629e47038e49028c5e0e7a3de65f50","unresolved":true,"context_lines":[{"line_number":327,"context_line":"                for acl in self._pgs_from_log_obj(context,"},{"line_number":328,"context_line":"                                                  log)[0][\"acls\"]:"},{"line_number":329,"context_line":"                    if not acls_to_check:"},{"line_number":330,"context_line":"                        return True"},{"line_number":331,"context_line":"                    elif acl in acls_to_check:"},{"line_number":332,"context_line":"                        acls_to_check.remove(acl)"},{"line_number":333,"context_line":"            acls_to_remove \u003d [{\"name\": pgs[0][\"name\"],"}],"source_content_type":"text/x-python","patch_set":4,"id":"eacadf4d_152e4b75","line":330,"in_reply_to":"e46f4b70_14ee3bb4","updated":"2022-11-24 08:48:55.000000000","message":"I did the same comment in PS2. But I think now I understand it. Only if we have acls in the port groups AND there are no ACLs to check, we can exit.\n\nBut if there no logs, we should remove any \"acls_to_check\".\n\nIf that is the case, please check in L333 if you have any \"acls_to_check\". If not, there is no need to call \"_remove_acls_log\" (despite this method will correctly handle empty ACLs list, but writing an unneeded log)","commit_id":"4d6df4bb6fe942bd6e05088d030b49a65dd7056f"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"323005303bd973872cc22c2d765b8ee3ed2b2064","unresolved":false,"context_lines":[{"line_number":327,"context_line":"                for acl in self._pgs_from_log_obj(context,"},{"line_number":328,"context_line":"                                                  log)[0][\"acls\"]:"},{"line_number":329,"context_line":"                    if not acls_to_check:"},{"line_number":330,"context_line":"                        return True"},{"line_number":331,"context_line":"                    elif acl in acls_to_check:"},{"line_number":332,"context_line":"                        acls_to_check.remove(acl)"},{"line_number":333,"context_line":"            acls_to_remove \u003d [{\"name\": pgs[0][\"name\"],"}],"source_content_type":"text/x-python","patch_set":4,"id":"ac35724a_5a350788","line":330,"in_reply_to":"eacadf4d_152e4b75","updated":"2022-11-24 13:32:08.000000000","message":"You are right, I made the change.","commit_id":"4d6df4bb6fe942bd6e05088d030b49a65dd7056f"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"46ff57e78da739efbb0adb35ccc80961ba9bed57","unresolved":true,"context_lines":[{"line_number":322,"context_line":"            return False"},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"        if log_obj.event \u003d\u003d log_const.ALL_EVENT:"},{"line_number":325,"context_line":"            acls_to_check \u003d pgs[0][\"acls\"].copy()"},{"line_number":326,"context_line":"            for log in other_logs:"},{"line_number":327,"context_line":"                for acl in self._pgs_from_log_obj(context, log)[0][\"acls\"]:"},{"line_number":328,"context_line":"                    if acl in acls_to_check:"}],"source_content_type":"text/x-python","patch_set":5,"id":"7e58d24a_3896f7ea","line":325,"updated":"2022-11-25 14:14:12.000000000","message":"nit: I think You can add check:\n\n    if not acls_to_check:\n        return True\n        \nhere to avoid going through any loop if acls_to_check will be empty","commit_id":"dfd34d8e64c99eedc5573317d4daa1d0130c8be8"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"f5a137c66406608294a109fc1b424387a49c668a","unresolved":false,"context_lines":[{"line_number":322,"context_line":"            return False"},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"        if log_obj.event \u003d\u003d log_const.ALL_EVENT:"},{"line_number":325,"context_line":"            acls_to_check \u003d pgs[0][\"acls\"].copy()"},{"line_number":326,"context_line":"            for log in other_logs:"},{"line_number":327,"context_line":"                for acl in self._pgs_from_log_obj(context, log)[0][\"acls\"]:"},{"line_number":328,"context_line":"                    if acl in acls_to_check:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fd23684_39afe0ef","line":325,"in_reply_to":"7e58d24a_3896f7ea","updated":"2022-11-25 15:43:00.000000000","message":"Done","commit_id":"dfd34d8e64c99eedc5573317d4daa1d0130c8be8"}],"neutron/tests/functional/services/logapi/drivers/ovn/test_driver.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"3bd80248805515854c9a00fe821729966a401789","unresolved":true,"context_lines":[{"line_number":384,"context_line":""},{"line_number":385,"context_line":"        # Disable all log object and check all acls are still enabled (because"},{"line_number":386,"context_line":"        # of the other objects)"},{"line_number":387,"context_line":"        log_data3[\u0027log\u0027][\u0027enabled\u0027] \u003d False"},{"line_number":388,"context_line":"        self.log_plugin.update_log(self.ctxt, log_obj3[\u0027id\u0027], log_data3)"},{"line_number":389,"context_line":"        self._check_sgrs(sgrs\u003dsgrs, is_enabled\u003dTrue)"},{"line_number":390,"context_line":"        self._check_acl_log_drop(is_enabled\u003dTrue)"},{"line_number":391,"context_line":""},{"line_number":392,"context_line":"        # Disable accept log object and only drop traffic gets logged"},{"line_number":393,"context_line":"        log_data1[\u0027log\u0027][\u0027enabled\u0027] \u003d False"}],"source_content_type":"text/x-python","patch_set":3,"id":"d102d623_497e865e","line":390,"range":{"start_line":387,"start_character":8,"end_line":390,"end_character":49},"updated":"2022-11-16 16:40:37.000000000","message":"I would need some advise here. In this case, we should be calling [1] with \"acls_to_remove\" containing the ACLs corresponding to log_data3. Is that correct? If so, I\u0027ve tested this but I don\u0027t see this method removing the ACLs.\n\n[1]https://review.opendev.org/c/openstack/neutron/+/864152/3/neutron/services/logapi/drivers/ovn/driver.py#333","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"b21f6f4f46d62516fa7bf74a368f0261271ad0cc","unresolved":true,"context_lines":[{"line_number":384,"context_line":""},{"line_number":385,"context_line":"        # Disable all log object and check all acls are still enabled (because"},{"line_number":386,"context_line":"        # of the other objects)"},{"line_number":387,"context_line":"        log_data3[\u0027log\u0027][\u0027enabled\u0027] \u003d False"},{"line_number":388,"context_line":"        self.log_plugin.update_log(self.ctxt, log_obj3[\u0027id\u0027], log_data3)"},{"line_number":389,"context_line":"        self._check_sgrs(sgrs\u003dsgrs, is_enabled\u003dTrue)"},{"line_number":390,"context_line":"        self._check_acl_log_drop(is_enabled\u003dTrue)"},{"line_number":391,"context_line":""},{"line_number":392,"context_line":"        # Disable accept log object and only drop traffic gets logged"},{"line_number":393,"context_line":"        log_data1[\u0027log\u0027][\u0027enabled\u0027] \u003d False"}],"source_content_type":"text/x-python","patch_set":3,"id":"edde5234_2020fa55","line":390,"range":{"start_line":387,"start_character":8,"end_line":390,"end_character":49},"in_reply_to":"d102d623_497e865e","updated":"2022-11-18 13:56:48.000000000","message":"I don\u0027t think we should remove the ACL, we should just ensure the log property is set to off. The ACL is created for the security group control, and not for the logging itself.","commit_id":"c50eebc3f2fab424c79036cf2029244b25ca4ed6"}]}