)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"03903370d52e7c9f9d6d02a76f0302450c25e039","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"50e9138e_a1b600d8","updated":"2024-03-01 18:16:35.000000000","message":"Is this actually used in the dependent patch? I didn\u0027t see it.","commit_id":"562347172e113633ee2a8fc57eb9b5df7204086c"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"8e741b4fd871b38bacffa86c2d9dae5d2e2358f3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"22f36449_2e69dc7e","updated":"2024-03-01 07:05:08.000000000","message":"recheck pep8 solved by https://review.opendev.org/c/openstack/neutron/+/910583","commit_id":"562347172e113633ee2a8fc57eb9b5df7204086c"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"41f4b7baa441c40e37e09feb5006cdf158978f82","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"b573f456_f750517f","in_reply_to":"50e9138e_a1b600d8","updated":"2024-03-04 08:09:08.000000000","message":"No, I\u0027ll use it in the next patch. But in shake of review clarity, I preferred to push this smaller patch with the proper testing. Actually I realize that I\u0027m missing something:","commit_id":"562347172e113633ee2a8fc57eb9b5df7204086c"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"52520e08bc3e4b0ffe6a00ac560988a54d4df128","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"cba285c8_447bc3e1","updated":"2024-03-04 14:26:20.000000000","message":"recheck openstack-tox-cover","commit_id":"6f0ae857a601b483591f05ac6cb8a4dcbe5f7891"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"04901df3d670374b218f547e41a8e491d1f755d5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"a8c2f59f_75d1a4ad","updated":"2024-03-25 14:26:57.000000000","message":"-1 is just for the question about having the `commands.py` code in this patch.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"9f1dc64b6f880c6a4ea4f00bc7fef66f7e42e62c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"4ff7cce3_ff62c076","updated":"2024-03-25 13:56:43.000000000","message":"In general, what we do with passing txns around as arguments to multiple functions is an anti-pattern in ovsdbapp.\n\nThe main reason is because we often do several different OVSDB read operations in these functions whose data we then use to decide what we are going to do with an eventual write operation. These read operations are their own separate \"transactions\", so the values we get can change by the time we actually do our write operation (so we lose atomicity/consistency). i.e. we could have several reads whose data no longer make sense together.\n\nI really supercharaged this problem by adding \"ReadOnlyCommand\" [1][2] where the correct thing to do would have been to say \"If you need data from the DB from one operation to do another operation, do it at the Command layer.\"\n\nI realize this pattern is all over OVNClient (and I\u0027m sure I\u0027ve followed that pattern myself). But the proper way to do this in ovsdbapp is with custom Command objects that do all reads and writes in the same transaction.\n\nThis is not to say that we can\u0027t add this code. I\u0027d just like to make clear that for the most part, we shouldn\u0027t actually be doing the things that we already do that this code makes prettier.\n\n[1] https://review.opendev.org/c/openstack/ovsdbapp/+/633070\n[2] https://bugs.launchpad.net/networking-ovn/+bug/1813213","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d43f8480a588881cbf4249c00af2a3343d266ef1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"540869f0_da58f021","updated":"2024-03-27 20:31:49.000000000","message":"Thanks Rodolfo, I have one more questions if I understand correctly the decorator.","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"ee3e19f7e16aac052a6ec58adec1d317a6d57a07","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"278e6bd1_f61435cb","updated":"2024-03-28 17:54:15.000000000","message":"LGTM after the pep8 unused import is solved :) Thanks Rodolfo!","commit_id":"e93ce20814bdf1b8009c8886fbc963e56912c9eb"},{"author":{"_account_id":21798,"name":"Bernard Cafarelli","email":"bcafarel@redhat.com","username":"bcafarel"},"change_message_id":"be879c6ba6cc3f0587c7c65e52b0a391ca3beb31","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"a149f16f_a745f757","updated":"2024-03-29 09:15:33.000000000","message":"Just rebased to fix the unused import (pep8)","commit_id":"00b64d289b3340af9e838909077d58c892b76a71"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"cefd77799a624bb977dbfb2e5c94d78ddb642c1d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"e837bcaf_d3de6ad0","updated":"2024-03-29 12:36:04.000000000","message":"Thanks Rodolfo!","commit_id":"00b64d289b3340af9e838909077d58c892b76a71"}],"neutron/common/ovn/utils.py":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"03903370d52e7c9f9d6d02a76f0302450c25e039","unresolved":true,"context_lines":[{"line_number":135,"context_line":""},{"line_number":136,"context_line":"def ovn_context(txn_var_name\u003d\u0027txn\u0027, idl_var_name\u003d\u0027idl\u0027):"},{"line_number":137,"context_line":"    \"\"\"Description"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    \"\"\""},{"line_number":140,"context_line":"    def decorator(f):"},{"line_number":141,"context_line":"        signature \u003d inspect.signature(f)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5bc27267_497be461","line":138,"updated":"2024-03-01 18:16:35.000000000","message":"missing description","commit_id":"562347172e113633ee2a8fc57eb9b5df7204086c"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"41f4b7baa441c40e37e09feb5006cdf158978f82","unresolved":false,"context_lines":[{"line_number":135,"context_line":""},{"line_number":136,"context_line":"def ovn_context(txn_var_name\u003d\u0027txn\u0027, idl_var_name\u003d\u0027idl\u0027):"},{"line_number":137,"context_line":"    \"\"\"Description"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    \"\"\""},{"line_number":140,"context_line":"    def decorator(f):"},{"line_number":141,"context_line":"        signature \u003d inspect.signature(f)"}],"source_content_type":"text/x-python","patch_set":3,"id":"085e4445_d653bf61","line":138,"in_reply_to":"5bc27267_497be461","updated":"2024-03-04 08:09:08.000000000","message":"Right!","commit_id":"562347172e113633ee2a8fc57eb9b5df7204086c"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"fee2be2581a22330b22c22db10b6d5d19a61547d","unresolved":true,"context_lines":[{"line_number":141,"context_line":"    ``ovsdbapp.backend.ovs_idl.transaction.Transaction`` derived object, is"},{"line_number":142,"context_line":"    empty. In that case (an empty transaction), that means the decorated method"},{"line_number":143,"context_line":"    has been called outside a transaction. In this case, the decorator creates"},{"line_number":144,"context_line":"    a transaction form the provided IDL and assigns it to the \u0027txn_var_name\u0027"},{"line_number":145,"context_line":"    variable."},{"line_number":146,"context_line":"    \"\"\""},{"line_number":147,"context_line":"    def decorator(f):"}],"source_content_type":"text/x-python","patch_set":8,"id":"7cefb4da_37d0da86","line":144,"range":{"start_line":144,"start_character":18,"end_line":144,"end_character":22},"updated":"2024-03-25 22:36:48.000000000","message":"from","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4cb9e31ae43740a648de3481a5d79bfd127f62fd","unresolved":false,"context_lines":[{"line_number":141,"context_line":"    ``ovsdbapp.backend.ovs_idl.transaction.Transaction`` derived object, is"},{"line_number":142,"context_line":"    empty. In that case (an empty transaction), that means the decorated method"},{"line_number":143,"context_line":"    has been called outside a transaction. In this case, the decorator creates"},{"line_number":144,"context_line":"    a transaction form the provided IDL and assigns it to the \u0027txn_var_name\u0027"},{"line_number":145,"context_line":"    variable."},{"line_number":146,"context_line":"    \"\"\""},{"line_number":147,"context_line":"    def decorator(f):"}],"source_content_type":"text/x-python","patch_set":8,"id":"76399094_f5f9a104","line":144,"range":{"start_line":144,"start_character":18,"end_line":144,"end_character":22},"in_reply_to":"7cefb4da_37d0da86","updated":"2024-03-26 13:06:09.000000000","message":"Done","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"fee2be2581a22330b22c22db10b6d5d19a61547d","unresolved":true,"context_lines":[{"line_number":148,"context_line":"        signature \u003d inspect.signature(f)"},{"line_number":149,"context_line":"        if (txn_var_name not in signature.parameters or"},{"line_number":150,"context_line":"                idl_var_name not in signature.parameters):"},{"line_number":151,"context_line":"            msg \u003d (_(\u0027Could not find position of variables %s and %s\u0027) %"},{"line_number":152,"context_line":"                   (txn_var_name, idl_var_name))"},{"line_number":153,"context_line":"            raise RuntimeError(msg)"},{"line_number":154,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"0103a5ed_9f0c1de0","line":151,"range":{"start_line":151,"start_character":37,"end_line":151,"end_character":45},"updated":"2024-03-25 22:36:48.000000000","message":"nit: Isn\u0027t this a bit misleading? We\u0027re not looking for positions but if any of the parameters is among the function\u0027s parameters.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4cb9e31ae43740a648de3481a5d79bfd127f62fd","unresolved":false,"context_lines":[{"line_number":148,"context_line":"        signature \u003d inspect.signature(f)"},{"line_number":149,"context_line":"        if (txn_var_name not in signature.parameters or"},{"line_number":150,"context_line":"                idl_var_name not in signature.parameters):"},{"line_number":151,"context_line":"            msg \u003d (_(\u0027Could not find position of variables %s and %s\u0027) %"},{"line_number":152,"context_line":"                   (txn_var_name, idl_var_name))"},{"line_number":153,"context_line":"            raise RuntimeError(msg)"},{"line_number":154,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"be83e4c5_4df08975","line":151,"range":{"start_line":151,"start_character":37,"end_line":151,"end_character":45},"in_reply_to":"0103a5ed_9f0c1de0","updated":"2024-03-26 13:06:09.000000000","message":"Correct, I\u0027ll change it.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"fee2be2581a22330b22c22db10b6d5d19a61547d","unresolved":true,"context_lines":[{"line_number":156,"context_line":"        def wrapped(*args, **kwargs):"},{"line_number":157,"context_line":"            txn_param \u003d signature.parameters[txn_var_name]"},{"line_number":158,"context_line":"            txn_index \u003d pecan_util.getargspec(f).args.index(txn_var_name)"},{"line_number":159,"context_line":"            if txn_param.default \u003d\u003d signature.empty:"},{"line_number":160,"context_line":"                _txn \u003d args[txn_index]"},{"line_number":161,"context_line":"            else:"},{"line_number":162,"context_line":"                _txn \u003d kwargs.get(txn_var_name)"}],"source_content_type":"text/x-python","patch_set":8,"id":"b06d7fa6_41ccdc83","line":159,"updated":"2024-03-25 22:36:48.000000000","message":"So IIUC this is when the parameter doesn\u0027t have a default number. But it doesn\u0027t mean it can\u0027t be passed as a keyword parameter. I think what you wanted to do is:\n\n```\n   try:\n       # parameter was passed as positional\n       _txn \u003d args[txn_index]\n   except IndexError:\n       try:\n           # parameter was passed as keyword\n           _txn \u003d kwargs[txn_var_name]\n       except KeyError:\n            # parameter was not passed but we checked the function defines it so it\n            # must be the default\n            _txn \u003d txn_param.default\n```","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4cb9e31ae43740a648de3481a5d79bfd127f62fd","unresolved":false,"context_lines":[{"line_number":156,"context_line":"        def wrapped(*args, **kwargs):"},{"line_number":157,"context_line":"            txn_param \u003d signature.parameters[txn_var_name]"},{"line_number":158,"context_line":"            txn_index \u003d pecan_util.getargspec(f).args.index(txn_var_name)"},{"line_number":159,"context_line":"            if txn_param.default \u003d\u003d signature.empty:"},{"line_number":160,"context_line":"                _txn \u003d args[txn_index]"},{"line_number":161,"context_line":"            else:"},{"line_number":162,"context_line":"                _txn \u003d kwargs.get(txn_var_name)"}],"source_content_type":"text/x-python","patch_set":8,"id":"b46d8059_3b2ce2f0","line":159,"in_reply_to":"b06d7fa6_41ccdc83","updated":"2024-03-26 13:06:09.000000000","message":"Right, but I changed the order, first checking if it is keyword argument and the positional. It is also needed to check if, regardless of being keyword, is passed as positional. In this case, use it as positional.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d43f8480a588881cbf4249c00af2a3343d266ef1","unresolved":false,"context_lines":[{"line_number":156,"context_line":"        def wrapped(*args, **kwargs):"},{"line_number":157,"context_line":"            txn_param \u003d signature.parameters[txn_var_name]"},{"line_number":158,"context_line":"            txn_index \u003d pecan_util.getargspec(f).args.index(txn_var_name)"},{"line_number":159,"context_line":"            if txn_param.default \u003d\u003d signature.empty:"},{"line_number":160,"context_line":"                _txn \u003d args[txn_index]"},{"line_number":161,"context_line":"            else:"},{"line_number":162,"context_line":"                _txn \u003d kwargs.get(txn_var_name)"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a08b1b1_5c06c347","line":159,"in_reply_to":"b46d8059_3b2ce2f0","updated":"2024-03-27 20:31:49.000000000","message":"No I mean you checked first if the txn parameter has a default defined - and then assumed it was passed as positional. If it was passed as keyword then it raised an IndexError","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"fee2be2581a22330b22c22db10b6d5d19a61547d","unresolved":true,"context_lines":[{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        @functools.wraps(f)"},{"line_number":156,"context_line":"        def wrapped(*args, **kwargs):"},{"line_number":157,"context_line":"            txn_param \u003d signature.parameters[txn_var_name]"},{"line_number":158,"context_line":"            txn_index \u003d pecan_util.getargspec(f).args.index(txn_var_name)"},{"line_number":159,"context_line":"            if txn_param.default \u003d\u003d signature.empty:"},{"line_number":160,"context_line":"                _txn \u003d args[txn_index]"},{"line_number":161,"context_line":"            else:"},{"line_number":162,"context_line":"                _txn \u003d kwargs.get(txn_var_name)"},{"line_number":163,"context_line":"                # Despite being a keyword argument, \"txn\" could have been"},{"line_number":164,"context_line":"                # passed as a positional argument."},{"line_number":165,"context_line":"                try:"},{"line_number":166,"context_line":"                    args[txn_index]"},{"line_number":167,"context_line":"                except IndexError:"},{"line_number":168,"context_line":"                    txn_index \u003d None"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"            idl_param \u003d signature.parameters[idl_var_name]"},{"line_number":171,"context_line":"            if idl_param.default \u003d\u003d signature.empty:"},{"line_number":172,"context_line":"                idl_index \u003d pecan_util.getargspec(f).args.index(idl_var_name)"},{"line_number":173,"context_line":"                _idl \u003d args[idl_index]"},{"line_number":174,"context_line":"            else:"},{"line_number":175,"context_line":"                _idl \u003d kwargs.get(idl_var_name)"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"            if not _txn and not _idl:"},{"line_number":178,"context_line":"                msg \u003d (_(\u0027If no transaction is defined, it is needed at least \u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"34739be5_00164907","line":175,"range":{"start_line":157,"start_character":0,"end_line":175,"end_character":47},"updated":"2024-03-25 22:36:48.000000000","message":"This seems like almost a copy \u0026 paste - would be good to have a function that gets the value of the given parameter from the function object and just call it. It would be also easier to unit-test it if it\u0027s broken down.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4cb9e31ae43740a648de3481a5d79bfd127f62fd","unresolved":false,"context_lines":[{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        @functools.wraps(f)"},{"line_number":156,"context_line":"        def wrapped(*args, **kwargs):"},{"line_number":157,"context_line":"            txn_param \u003d signature.parameters[txn_var_name]"},{"line_number":158,"context_line":"            txn_index \u003d pecan_util.getargspec(f).args.index(txn_var_name)"},{"line_number":159,"context_line":"            if txn_param.default \u003d\u003d signature.empty:"},{"line_number":160,"context_line":"                _txn \u003d args[txn_index]"},{"line_number":161,"context_line":"            else:"},{"line_number":162,"context_line":"                _txn \u003d kwargs.get(txn_var_name)"},{"line_number":163,"context_line":"                # Despite being a keyword argument, \"txn\" could have been"},{"line_number":164,"context_line":"                # passed as a positional argument."},{"line_number":165,"context_line":"                try:"},{"line_number":166,"context_line":"                    args[txn_index]"},{"line_number":167,"context_line":"                except IndexError:"},{"line_number":168,"context_line":"                    txn_index \u003d None"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"            idl_param \u003d signature.parameters[idl_var_name]"},{"line_number":171,"context_line":"            if idl_param.default \u003d\u003d signature.empty:"},{"line_number":172,"context_line":"                idl_index \u003d pecan_util.getargspec(f).args.index(idl_var_name)"},{"line_number":173,"context_line":"                _idl \u003d args[idl_index]"},{"line_number":174,"context_line":"            else:"},{"line_number":175,"context_line":"                _idl \u003d kwargs.get(idl_var_name)"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"            if not _txn and not _idl:"},{"line_number":178,"context_line":"                msg \u003d (_(\u0027If no transaction is defined, it is needed at least \u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"aa4d85df_ef8e01f9","line":175,"range":{"start_line":157,"start_character":0,"end_line":175,"end_character":47},"in_reply_to":"34739be5_00164907","updated":"2024-03-26 13:06:09.000000000","message":"Done","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"fee2be2581a22330b22c22db10b6d5d19a61547d","unresolved":true,"context_lines":[{"line_number":186,"context_line":"                                args[txn_index + 1:])"},{"line_number":187,"context_line":"                    else:"},{"line_number":188,"context_line":"                        kwargs[txn_var_name] \u003d new_txn"},{"line_number":189,"context_line":"                    return f(*args, **kwargs)"},{"line_number":190,"context_line":"            else:"},{"line_number":191,"context_line":"                return f(*args, **kwargs)"},{"line_number":192,"context_line":"        return wrapped"},{"line_number":193,"context_line":"    return decorator"}],"source_content_type":"text/x-python","patch_set":8,"id":"b847199c_30c7e33e","line":190,"range":{"start_line":189,"start_character":0,"end_line":190,"end_character":17},"updated":"2024-03-25 22:36:48.000000000","message":"you don\u0027t need these lines","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4cb9e31ae43740a648de3481a5d79bfd127f62fd","unresolved":false,"context_lines":[{"line_number":186,"context_line":"                                args[txn_index + 1:])"},{"line_number":187,"context_line":"                    else:"},{"line_number":188,"context_line":"                        kwargs[txn_var_name] \u003d new_txn"},{"line_number":189,"context_line":"                    return f(*args, **kwargs)"},{"line_number":190,"context_line":"            else:"},{"line_number":191,"context_line":"                return f(*args, **kwargs)"},{"line_number":192,"context_line":"        return wrapped"},{"line_number":193,"context_line":"    return decorator"}],"source_content_type":"text/x-python","patch_set":8,"id":"c68399d3_3311e212","line":190,"range":{"start_line":189,"start_character":0,"end_line":190,"end_character":17},"in_reply_to":"b847199c_30c7e33e","updated":"2024-03-26 13:06:09.000000000","message":"I do, the first method is called inside the transaction (L183), the second isn\u0027t.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d43f8480a588881cbf4249c00af2a3343d266ef1","unresolved":false,"context_lines":[{"line_number":186,"context_line":"                                args[txn_index + 1:])"},{"line_number":187,"context_line":"                    else:"},{"line_number":188,"context_line":"                        kwargs[txn_var_name] \u003d new_txn"},{"line_number":189,"context_line":"                    return f(*args, **kwargs)"},{"line_number":190,"context_line":"            else:"},{"line_number":191,"context_line":"                return f(*args, **kwargs)"},{"line_number":192,"context_line":"        return wrapped"},{"line_number":193,"context_line":"    return decorator"}],"source_content_type":"text/x-python","patch_set":8,"id":"7145573a_2e08e64a","line":190,"range":{"start_line":189,"start_character":0,"end_line":190,"end_character":17},"in_reply_to":"c68399d3_3311e212","updated":"2024-03-27 20:31:49.000000000","message":"aha, sorry, you\u0027re right!","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d43f8480a588881cbf4249c00af2a3343d266ef1","unresolved":true,"context_lines":[{"line_number":154,"context_line":"            raise RuntimeError(msg)"},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"        def retrieve_parameter(param_name, _args, _kwargs):"},{"line_number":157,"context_line":"            param_index \u003d pecan_util.getargspec(f).args.index(param_name)"},{"line_number":158,"context_line":"            try:"},{"line_number":159,"context_line":"                # Parameter passed as keyword."},{"line_number":160,"context_line":"                value \u003d _kwargs[param_name]"}],"source_content_type":"text/x-python","patch_set":10,"id":"4c49258f_d6d81c0e","line":157,"updated":"2024-03-27 20:31:49.000000000","message":"nit: I would add a comment that this is the position index of where the given param_name is in the list of function\u0027s parameters. I had to try it out myself in interactive python to make sure what it does and I\u0027m sure I will not remember in 2 years from now.","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"6aced4a3ccd39183409e9bbbf333c3352076e8dd","unresolved":false,"context_lines":[{"line_number":154,"context_line":"            raise RuntimeError(msg)"},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"        def retrieve_parameter(param_name, _args, _kwargs):"},{"line_number":157,"context_line":"            param_index \u003d pecan_util.getargspec(f).args.index(param_name)"},{"line_number":158,"context_line":"            try:"},{"line_number":159,"context_line":"                # Parameter passed as keyword."},{"line_number":160,"context_line":"                value \u003d _kwargs[param_name]"}],"source_content_type":"text/x-python","patch_set":10,"id":"f32ce5e1_c612e1e1","line":157,"in_reply_to":"4c49258f_d6d81c0e","updated":"2024-03-28 10:06:39.000000000","message":"I almost don\u0027t remember that 1 week after coding it... so yes, I *must* document it.","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d43f8480a588881cbf4249c00af2a3343d266ef1","unresolved":true,"context_lines":[{"line_number":158,"context_line":"            try:"},{"line_number":159,"context_line":"                # Parameter passed as keyword."},{"line_number":160,"context_line":"                value \u003d _kwargs[param_name]"},{"line_number":161,"context_line":"                # Despite being a keyword argument, \"txn\" could have been"},{"line_number":162,"context_line":"                # passed as a positional argument."},{"line_number":163,"context_line":"                try:"},{"line_number":164,"context_line":"                    _args[param_index]"},{"line_number":165,"context_line":"                except IndexError:"},{"line_number":166,"context_line":"                    param_index \u003d None"},{"line_number":167,"context_line":"            except KeyError:"},{"line_number":168,"context_line":"                # Parameter passed as possitional argument."},{"line_number":169,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":10,"id":"20bca25d_1c9b59d4","line":166,"range":{"start_line":161,"start_character":0,"end_line":166,"end_character":38},"updated":"2024-03-27 20:31:49.000000000","message":"I\u0027m not sure I understand this. How can one parameter be passed as a keyword and positional at the same time? I didn\u0027t know that is possible","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"6aced4a3ccd39183409e9bbbf333c3352076e8dd","unresolved":false,"context_lines":[{"line_number":158,"context_line":"            try:"},{"line_number":159,"context_line":"                # Parameter passed as keyword."},{"line_number":160,"context_line":"                value \u003d _kwargs[param_name]"},{"line_number":161,"context_line":"                # Despite being a keyword argument, \"txn\" could have been"},{"line_number":162,"context_line":"                # passed as a positional argument."},{"line_number":163,"context_line":"                try:"},{"line_number":164,"context_line":"                    _args[param_index]"},{"line_number":165,"context_line":"                except IndexError:"},{"line_number":166,"context_line":"                    param_index \u003d None"},{"line_number":167,"context_line":"            except KeyError:"},{"line_number":168,"context_line":"                # Parameter passed as possitional argument."},{"line_number":169,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":10,"id":"9ac23893_5c86f8f2","line":166,"range":{"start_line":161,"start_character":0,"end_line":166,"end_character":38},"in_reply_to":"20bca25d_1c9b59d4","updated":"2024-03-28 10:06:39.000000000","message":"No hehehe, what I\u0027m saying is that a keyword argument is passed as positional argument. In that case, the parameter will be both in kwargs (because is defined by the method signature) and in args. In this case, what we need to do is to modify the value in args, not in kwargs.\n\nHaving said that, what I can do is to first check if the value is in args, not in kwargs. If the value is in args (despite being a positional or a keyword argument) what we need to modify is, as commented, is the args value.","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"04901df3d670374b218f547e41a8e491d1f755d5","unresolved":true,"context_lines":[{"line_number":479,"context_line":"            if col \u003d\u003d \u0027gateway_chassis\u0027:"},{"line_number":480,"context_line":"                col, val \u003d _add_gateway_chassis(self.api, txn, self.name,"},{"line_number":481,"context_line":"                                                val)"},{"line_number":482,"context_line":"            elif col \u003d\u003d \u0027ha_chassis_group\u0027:"},{"line_number":483,"context_line":"                val \u003d ovsdbapp_utils.get_uuid(val)"},{"line_number":484,"context_line":"            setattr(lrouter_port, col, val)"},{"line_number":485,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"881d62df_f1d09134","line":482,"updated":"2024-03-25 14:26:57.000000000","message":"Is this supposed to be part of this patch? Is it just to support the test? It feels a little weird to have it here in case this patch ever got reverted and then some other code relied on this bit.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4cb9e31ae43740a648de3481a5d79bfd127f62fd","unresolved":false,"context_lines":[{"line_number":479,"context_line":"            if col \u003d\u003d \u0027gateway_chassis\u0027:"},{"line_number":480,"context_line":"                col, val \u003d _add_gateway_chassis(self.api, txn, self.name,"},{"line_number":481,"context_line":"                                                val)"},{"line_number":482,"context_line":"            elif col \u003d\u003d \u0027ha_chassis_group\u0027:"},{"line_number":483,"context_line":"                val \u003d ovsdbapp_utils.get_uuid(val)"},{"line_number":484,"context_line":"            setattr(lrouter_port, col, val)"},{"line_number":485,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"b105b728_bf6d7cdc","line":482,"in_reply_to":"881d62df_f1d09134","updated":"2024-03-26 13:06:09.000000000","message":"Yes, this is only for the testing part. This is also implemented in the LSP, so I can remove this and implement the test using a LSP instead of a LRP.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"}],"neutron/tests/functional/common/ovn/test_utils.py":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"03903370d52e7c9f9d6d02a76f0302450c25e039","unresolved":true,"context_lines":[{"line_number":200,"context_line":""},{"line_number":201,"context_line":"    @utils.ovn_context()"},{"line_number":202,"context_line":"    def create_hcg_1(self, hcg_name, idl, txn):"},{"line_number":203,"context_line":"        return txn.add(idl.ha_chassis_group_add(hcg_name))"},{"line_number":204,"context_line":""},{"line_number":205,"context_line":"    @utils.ovn_context()"},{"line_number":206,"context_line":"    def create_hcg_2(self, hcg_name, idl\u003dNone, txn\u003dNone):"}],"source_content_type":"text/x-python","patch_set":3,"id":"e6cda6a8_42e078cf","line":203,"updated":"2024-03-01 18:16:35.000000000","message":"This and below always take uuidutils.generate_uuid() as a name argument from what I can tell, should probably just do that here instead since noone seems to check it.","commit_id":"562347172e113633ee2a8fc57eb9b5df7204086c"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"41f4b7baa441c40e37e09feb5006cdf158978f82","unresolved":false,"context_lines":[{"line_number":200,"context_line":""},{"line_number":201,"context_line":"    @utils.ovn_context()"},{"line_number":202,"context_line":"    def create_hcg_1(self, hcg_name, idl, txn):"},{"line_number":203,"context_line":"        return txn.add(idl.ha_chassis_group_add(hcg_name))"},{"line_number":204,"context_line":""},{"line_number":205,"context_line":"    @utils.ovn_context()"},{"line_number":206,"context_line":"    def create_hcg_2(self, hcg_name, idl\u003dNone, txn\u003dNone):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bb7cff94_abc2deed","line":203,"in_reply_to":"e6cda6a8_42e078cf","updated":"2024-03-04 08:09:08.000000000","message":"Yes but I wanted the second method with both positional and keyword arguments.\n\nActually in the next PS I\u0027ve added new method with kwargs but the variables passed as positional arguments.\n\nI\u0027ve also added in PS4 new methods to check that the command returned by the context decorator can be used inside the same transaction to update another register (as is currently done with the LSP.ha_chassis_group).","commit_id":"562347172e113633ee2a8fc57eb9b5df7204086c"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"fee2be2581a22330b22c22db10b6d5d19a61547d","unresolved":true,"context_lines":[{"line_number":199,"context_line":"class TestOvnContext(base.TestOVNFunctionalBase):"},{"line_number":200,"context_line":""},{"line_number":201,"context_line":"    @utils.ovn_context()"},{"line_number":202,"context_line":"    def create_lr(self, idl, txn\u003dNone):"},{"line_number":203,"context_line":"        r_name \u003d uuidutils.generate_uuid()"},{"line_number":204,"context_line":"        txn.add(idl.lr_add(r_name))"},{"line_number":205,"context_line":"        return r_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba55eeb0_bd331c51","line":202,"updated":"2024-03-25 22:36:48.000000000","message":"I don\u0027t think we need to test that it works with various commands. Instead the tests should check all variations of passed parameters even on a single command, like:\n```\n  @utils.ovn_context()\n  def method_with_idl_and_default_txn(self, idl, txn\u003dNone):\n      txn.add(idl.ls_add(\u0027test_ls\u0027))\n      \n  @utils.ovn_context()\n  def method_with_idl_and_txn(self, idl, txn):\n      txn.add(idl.ls_add(\u0027test_ls\u0027))\n      \n  @utils.ovn_context(\"custom_txn\", \"custom_idl\")\n  def method_with_custom_idl_and_custom_txn(self, custom_idl, custom_txn):\n      txn.add(idl.ls_add(\u0027test_ls\u0027))\n```\n\netc.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4cb9e31ae43740a648de3481a5d79bfd127f62fd","unresolved":false,"context_lines":[{"line_number":199,"context_line":"class TestOvnContext(base.TestOVNFunctionalBase):"},{"line_number":200,"context_line":""},{"line_number":201,"context_line":"    @utils.ovn_context()"},{"line_number":202,"context_line":"    def create_lr(self, idl, txn\u003dNone):"},{"line_number":203,"context_line":"        r_name \u003d uuidutils.generate_uuid()"},{"line_number":204,"context_line":"        txn.add(idl.lr_add(r_name))"},{"line_number":205,"context_line":"        return r_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"c341e132_7316fa3e","line":202,"in_reply_to":"ba55eeb0_bd331c51","updated":"2024-03-26 13:06:09.000000000","message":"Correct. But I also need to check that the transaction parameter passed to the decorator works fine, for example updating a resource created in the same txn (``test_with_transaction``). I\u0027ll update the tests.","commit_id":"7990543795ed313344d8e823c5e4fb1abb2292c2"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"e454b83bc01197743d55272a2fa2fb319aa6fd26","unresolved":true,"context_lines":[{"line_number":223,"context_line":"class TestOvnContext(base.TestOVNFunctionalBase):"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":"    @ddt.unpack"},{"line_number":226,"context_line":"    @ddt.data({\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":227,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027], \u0027_kwargs\u0027: [\u0027txn\u0027]},"},{"line_number":228,"context_line":"              {\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":229,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027, \u0027txn\u0027], \u0027_kwargs\u0027: []},"}],"source_content_type":"text/x-python","patch_set":10,"id":"a412aa47_10a85919","line":226,"range":{"start_line":226,"start_character":9,"end_line":226,"end_character":13},"updated":"2024-03-27 20:56:57.000000000","message":"Is there a way to name each test? Currently it enumerates the tests which is not clear what the test is doing from the results https://b8fb61a0611e5c25c4e9-18cd90fd66f52a2b6d2ea93ae9a27d9a.ssl.cf5.rackcdn.com/910538/10/check/neutron-functional-with-uwsgi/bfbd367/testr_results.html","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"8ee6ac27c18301e48afd028e169ef0ffccde8692","unresolved":false,"context_lines":[{"line_number":223,"context_line":"class TestOvnContext(base.TestOVNFunctionalBase):"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":"    @ddt.unpack"},{"line_number":226,"context_line":"    @ddt.data({\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":227,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027], \u0027_kwargs\u0027: [\u0027txn\u0027]},"},{"line_number":228,"context_line":"              {\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":229,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027, \u0027txn\u0027], \u0027_kwargs\u0027: []},"}],"source_content_type":"text/x-python","patch_set":10,"id":"675f0497_d14f8789","line":226,"range":{"start_line":226,"start_character":9,"end_line":226,"end_character":13},"in_reply_to":"a412aa47_10a85919","updated":"2024-03-28 11:01:06.000000000","message":"Let me find this out.","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"e454b83bc01197743d55272a2fa2fb319aa6fd26","unresolved":true,"context_lines":[{"line_number":223,"context_line":"class TestOvnContext(base.TestOVNFunctionalBase):"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":"    @ddt.unpack"},{"line_number":226,"context_line":"    @ddt.data({\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":227,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027], \u0027_kwargs\u0027: [\u0027txn\u0027]},"},{"line_number":228,"context_line":"              {\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":229,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027, \u0027txn\u0027], \u0027_kwargs\u0027: []},"},{"line_number":230,"context_line":"              {\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":231,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027], \u0027_kwargs\u0027: []},"},{"line_number":232,"context_line":"              {\u0027method\u0027: method_with_idl_and_txn,"},{"line_number":233,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027, \u0027txn\u0027], \u0027_kwargs\u0027: []},"},{"line_number":234,"context_line":"              {\u0027method\u0027: method_with_custom_idl_and_custom_txn,"},{"line_number":235,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027custom_idl\u0027, \u0027custom_txn\u0027],"},{"line_number":236,"context_line":"               \u0027_kwargs\u0027: []},"},{"line_number":237,"context_line":"              )"},{"line_number":238,"context_line":"    def test_with_transaction(self, method, _args, _kwargs):"},{"line_number":239,"context_line":"        ls_name \u003d uuidutils.generate_uuid()"}],"source_content_type":"text/x-python","patch_set":10,"id":"ac4a9606_bdaaad65","line":236,"range":{"start_line":226,"start_character":14,"end_line":236,"end_character":30},"updated":"2024-03-27 20:56:57.000000000","message":"This is the same as L254-264? Can we do\n```\nclass TestOvn...():\n    scenarios \u003d {...}\n    \n    @ddt.unpack\n    @ddt.data(scenarios)\n```\n?\n\nI also think the tests should cover all combination of arguments that are supported. We treat idl and txn a little bit differently so would be good to have `idl` as keyword parameter too and a test with method_with_idl_and_txn and txn as a keyword and both as a keyword and only idl as a keyword because IIRC method_with_idl_and_txn would fail with txn\u003dNone as keyword?","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"8ee6ac27c18301e48afd028e169ef0ffccde8692","unresolved":false,"context_lines":[{"line_number":223,"context_line":"class TestOvnContext(base.TestOVNFunctionalBase):"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":"    @ddt.unpack"},{"line_number":226,"context_line":"    @ddt.data({\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":227,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027], \u0027_kwargs\u0027: [\u0027txn\u0027]},"},{"line_number":228,"context_line":"              {\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":229,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027, \u0027txn\u0027], \u0027_kwargs\u0027: []},"},{"line_number":230,"context_line":"              {\u0027method\u0027: method_with_idl_and_default_txn,"},{"line_number":231,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027], \u0027_kwargs\u0027: []},"},{"line_number":232,"context_line":"              {\u0027method\u0027: method_with_idl_and_txn,"},{"line_number":233,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027idl\u0027, \u0027txn\u0027], \u0027_kwargs\u0027: []},"},{"line_number":234,"context_line":"              {\u0027method\u0027: method_with_custom_idl_and_custom_txn,"},{"line_number":235,"context_line":"               \u0027_args\u0027: [\u0027ls_name\u0027, \u0027custom_idl\u0027, \u0027custom_txn\u0027],"},{"line_number":236,"context_line":"               \u0027_kwargs\u0027: []},"},{"line_number":237,"context_line":"              )"},{"line_number":238,"context_line":"    def test_with_transaction(self, method, _args, _kwargs):"},{"line_number":239,"context_line":"        ls_name \u003d uuidutils.generate_uuid()"}],"source_content_type":"text/x-python","patch_set":10,"id":"613654f6_e95c2336","line":236,"range":{"start_line":226,"start_character":14,"end_line":236,"end_character":30},"in_reply_to":"ac4a9606_bdaaad65","updated":"2024-03-28 11:01:06.000000000","message":"Exactly, I can reuse the scenarios in both tests.\n\nAnd yes, I\u0027m missing some of them. I\u0027ll add new testing methods.","commit_id":"e15952262e0c7643a0a9c53a328a3d7457d3e0cd"}]}