)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"ff32b468783c57bf743da763da534ab53089a356","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"f134870c_507cb38a","updated":"2023-09-28 19:08:24.000000000","message":"Thanks! Just some fairly minor comments/questions below.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":33871,"name":"Anton Vazhnetsov","display_name":"Anton Vazhnetsov","email":"dragen15051@gmail.com","username":"0x5b"},"change_message_id":"f6e6170a059a2582220e1e8713a7f140f4963365","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"64c4d49e_d5e276f0","in_reply_to":"f134870c_507cb38a","updated":"2023-09-28 21:34:33.000000000","message":"Hello! Thank you for your comments.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"2075f02b949a1ec4c421527a7d10365674ac37ce","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"c070be3c_9a94e606","updated":"2023-10-04 22:56:23.000000000","message":"Looks good to me, thanks!","commit_id":"dae455b6eea3dcae931d07d4a81c03190e8259ed"}],"ovsdbapp/schema/ovn_northbound/api.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"ff32b468783c57bf743da763da534ab53089a356","unresolved":true,"context_lines":[{"line_number":734,"context_line":"        \"\"\""},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop,"},{"line_number":738,"context_line":"                     port\u003dNone, policy\u003d\u0027dst-ip\u0027,"},{"line_number":739,"context_line":"                     ecmp\u003dFalse, route_table\u003dNone,"},{"line_number":740,"context_line":"                     may_exist\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"133fdb8e_5857ce86","line":737,"updated":"2023-09-28 19:08:24.000000000","message":"There are several whitespace-only changes in this patch (breaking after nexthob, breaking before the \u0027e.g. in the prefix definition, reordering parameters like may_exist and ecmp, and their definitions, etc.). It just makes backports potentially tricker and reviewing a bit harder.\n\nSame thing happens in other files as well, but I won\u0027t comment on each individually.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":33871,"name":"Anton Vazhnetsov","display_name":"Anton Vazhnetsov","email":"dragen15051@gmail.com","username":"0x5b"},"change_message_id":"f6e6170a059a2582220e1e8713a7f140f4963365","unresolved":true,"context_lines":[{"line_number":734,"context_line":"        \"\"\""},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop,"},{"line_number":738,"context_line":"                     port\u003dNone, policy\u003d\u0027dst-ip\u0027,"},{"line_number":739,"context_line":"                     ecmp\u003dFalse, route_table\u003dNone,"},{"line_number":740,"context_line":"                     may_exist\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"d5814bad_b6b0c3b8","line":737,"in_reply_to":"133fdb8e_5857ce86","updated":"2023-09-28 21:34:33.000000000","message":"yeah, I didn\u0027t think about the backports... It seemed to me that the arguments are arranged logically: first mandatory, then optional on different lines, so that when adding new arguments there will be fewer changes (not like this time) and \u0027may_exist\u0027 is always at the end.\n\nThen how about I add `route_table` argument before `may_exist` and move `ecmp` before `route_table` in a separate PR? I can\u0027t accept that 100 methods are written the same and one is not like all of them.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":33871,"name":"Anton Vazhnetsov","display_name":"Anton Vazhnetsov","email":"dragen15051@gmail.com","username":"0x5b"},"change_message_id":"02ec180eab0b2062a419bb422e8c1e450a826ec4","unresolved":true,"context_lines":[{"line_number":734,"context_line":"        \"\"\""},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop,"},{"line_number":738,"context_line":"                     port\u003dNone, policy\u003d\u0027dst-ip\u0027,"},{"line_number":739,"context_line":"                     ecmp\u003dFalse, route_table\u003dNone,"},{"line_number":740,"context_line":"                     may_exist\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"a1dba6d3_2c82904a","line":737,"in_reply_to":"165c422d_3661c6da","updated":"2023-10-01 07:44:45.000000000","message":"No problem, I get it, I\u0027ll arrange the arguments as you say. \nJust one comment: yes, passing arguments by position is a very bad habit, but it seems to me that the case of someone passing an \u0027may_exist\u0027 by position last and making a mistake because the \u0027route_table\u0027 is there is more likely. \nThis project has been Pyton3-only for a long time, so I suggest using features that are now available, like Keyword-Only Arguments (https://peps.python.org/pep-3102/). I can do this for all methods and submit a PR. In 2023, we shouldn\u0027t be wasting time discussing \"How to arrange KEYWORD arguments\".","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"d275689fb314ec73c2ddb807a55c62da9a952312","unresolved":true,"context_lines":[{"line_number":734,"context_line":"        \"\"\""},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop,"},{"line_number":738,"context_line":"                     port\u003dNone, policy\u003d\u0027dst-ip\u0027,"},{"line_number":739,"context_line":"                     ecmp\u003dFalse, route_table\u003dNone,"},{"line_number":740,"context_line":"                     may_exist\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"d645e01e_265dc204","line":737,"in_reply_to":"a1dba6d3_2c82904a","updated":"2023-10-04 13:50:54.000000000","message":"\u003e the case of someone passing an \u0027may_exist\u0027 by position last and making a mistake because the \u0027route_table\u0027 is there is more likely.\n\nI\u0027m not following. may_exist may be passed \"last\" by position, but \"last\" would mean \"6th\". By defining an optional route_table after it in the 7th position, it wouldn\u0027t break, right? What am I missing? \n\n\u003e This project has been Pyton3-only for a long time, so I suggest using features that are now available, like Keyword-Only Arguments (https://peps.python.org/pep-3102/). \n\nFair enough. The problem is that the API predates that and forcing existing arguments to be keyword only would be an API-breaking change. We could do it, but I\u0027m not sure the cost-benefit calculation on it would ultimately be beneficial enough for users to warrant potential breakage. Old code sucks sometimes.\n\nThere are still some instances of inserting extra line breaks and inserting arguments in the middle (also in the parent patch). I know it seems picky, and I feel bad about it. But I really do think we should avoid changing argument order t for now (unless I\u0027m missing something above re: may_exist being last, etc.).\n\nI don\u0027t want to be a pain, and I\u0027m happy to go in and adjust the whitespace issues myself. I just don\u0027t want to do it w/o your permission.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"e37430f416be187ff3a4730692f9e797a61b4b9b","unresolved":true,"context_lines":[{"line_number":734,"context_line":"        \"\"\""},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop,"},{"line_number":738,"context_line":"                     port\u003dNone, policy\u003d\u0027dst-ip\u0027,"},{"line_number":739,"context_line":"                     ecmp\u003dFalse, route_table\u003dNone,"},{"line_number":740,"context_line":"                     may_exist\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"165c422d_3661c6da","line":737,"in_reply_to":"d5814bad_b6b0c3b8","updated":"2023-09-29 22:17:11.000000000","message":"\u003e first mandatory, then optional on different lines, so that when adding new arguments there will be fewer changes (not like this time) and \u0027may_exist\u0027 is always at the end.\n\nBut that isn\u0027t how *any* of the other methods here work--they all put default/non-defaulted args on the same lines.\n\nThe rule is basically \"**Never** change the order of arguments unless absolutely necessary\" because people may be passing those arguments by position, not by name (not a good habit, but that\u0027s just how things go). Changing the order can break code. There\u0027s no benefit to putting arguments with defaults on a separate line from ones w/o defaults in this case, because you shouldn\u0027t ever be doing anything but appending new parameters unless making an API-breaking change (something we try very hard not to do).","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":33871,"name":"Anton Vazhnetsov","display_name":"Anton Vazhnetsov","email":"dragen15051@gmail.com","username":"0x5b"},"change_message_id":"b6ce23eddc3e48371802e640339702fc14043e2d","unresolved":true,"context_lines":[{"line_number":734,"context_line":"        \"\"\""},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop,"},{"line_number":738,"context_line":"                     port\u003dNone, policy\u003d\u0027dst-ip\u0027,"},{"line_number":739,"context_line":"                     ecmp\u003dFalse, route_table\u003dNone,"},{"line_number":740,"context_line":"                     may_exist\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"eda9e875_cf8472da","line":737,"in_reply_to":"d645e01e_265dc204","updated":"2023-10-04 15:34:35.000000000","message":"\u003e I\u0027m not following. may_exist may be passed \"last\" by position, but \"last\" would mean \"6th\". By defining an optional route_table after it in the 7th position, it wouldn\u0027t break, right? What am I missing?\n\nI was referring to new users with bad habits of passing arguments by position. For current users, adding new arguments to the end is exactly safe.\n\n\u003e We could do it, but I\u0027m not sure the cost-benefit calculation on it would ultimately be beneficial enough for users to warrant potential breakage.\n\nYes, the main benefit of such changes is to avoid such discussions, it will speed up development and merging of PR. Someday you\u0027ll still have to make incompatible changes, up a major version and write detailed release notes.\n\nI fixed everything.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"2075f02b949a1ec4c421527a7d10365674ac37ce","unresolved":false,"context_lines":[{"line_number":734,"context_line":"        \"\"\""},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop,"},{"line_number":738,"context_line":"                     port\u003dNone, policy\u003d\u0027dst-ip\u0027,"},{"line_number":739,"context_line":"                     ecmp\u003dFalse, route_table\u003dNone,"},{"line_number":740,"context_line":"                     may_exist\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"898055ef_7ebf588e","line":737,"in_reply_to":"eda9e875_cf8472da","updated":"2023-10-04 22:56:23.000000000","message":"Thank you. I appreciate your patience on this. We do definitely need to improve the speed of patch merging in general, regardless of minor formatting issues, etc. and I\u0027m a bottleneck that really shouldn\u0027t exist. There are lots of people able to approve commits to ovsdbapp, but not a lot do, but python-ovs is a weird beast and for the most part people wait on me to be one of the +2s just in case. I need to be quicker initially getting to reviews and to complete the \"ovsdbapp best practices\" doc I\u0027m working on as well.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"}],"ovsdbapp/schema/ovn_northbound/commands.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"ff32b468783c57bf743da763da534ab53089a356","unresolved":true,"context_lines":[{"line_number":1152,"context_line":"        self.port \u003d port"},{"line_number":1153,"context_line":"        self.policy \u003d policy"},{"line_number":1154,"context_line":"        self.ecmp \u003d ecmp"},{"line_number":1155,"context_line":"        self.route_table \u003d route_table or \"\""},{"line_number":1156,"context_line":"        self.may_exist \u003d may_exist"},{"line_number":1157,"context_line":""},{"line_number":1158,"context_line":"    def run_idl(self, txn):"}],"source_content_type":"text/x-python","patch_set":1,"id":"0e292ba8_596ee35b","line":1155,"updated":"2023-09-28 19:08:24.000000000","message":"In `LrRouteListCommand` there seems to be a distinction between `route_table\u003dNone` and `route_table\u003d\"\"` (which I realize the distinction wouldn\u0027t make much sense here). I\u0027m wondering if it makes sense to add some constants to the ovn_northbound api like `GLOBAL_STATIC_ROUTES` and `ALL_STATIC_ROUTES` and set those defaults just to make it absolutely clear what the defaults are?\n\nIn any case, it seems like defaulting to `route_table\u003d\"\"` (or `route_table\u003d\"GLOBAL_STATIC_ROUTES\"`) would be fine and then this could just be unconditionally `self.route_table \u003d route_table`.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":33871,"name":"Anton Vazhnetsov","display_name":"Anton Vazhnetsov","email":"dragen15051@gmail.com","username":"0x5b"},"change_message_id":"f6e6170a059a2582220e1e8713a7f140f4963365","unresolved":false,"context_lines":[{"line_number":1152,"context_line":"        self.port \u003d port"},{"line_number":1153,"context_line":"        self.policy \u003d policy"},{"line_number":1154,"context_line":"        self.ecmp \u003d ecmp"},{"line_number":1155,"context_line":"        self.route_table \u003d route_table or \"\""},{"line_number":1156,"context_line":"        self.may_exist \u003d may_exist"},{"line_number":1157,"context_line":""},{"line_number":1158,"context_line":"    def run_idl(self, txn):"}],"source_content_type":"text/x-python","patch_set":1,"id":"49c541ed_9a77871c","line":1155,"in_reply_to":"0e292ba8_596ee35b","updated":"2023-09-28 21:34:33.000000000","message":"Yes, adding a constant for the main routing table is a good idea. But I would call it `MAIN_ROUTE_TABLE` instead of `GLOBAL_STATIC_ROUTES`, because that is what it is called in the `ovn-nbctl lr-route-list` output.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"ff32b468783c57bf743da763da534ab53089a356","unresolved":true,"context_lines":[{"line_number":1202,"context_line":"        self.router \u003d router"},{"line_number":1203,"context_line":"        self.prefix \u003d prefix"},{"line_number":1204,"context_line":"        self.nexthop \u003d nexthop"},{"line_number":1205,"context_line":"        self.route_table \u003d route_table or \"\""},{"line_number":1206,"context_line":"        self.if_exists \u003d if_exists"},{"line_number":1207,"context_line":""},{"line_number":1208,"context_line":"    def run_idl(self, txn):"}],"source_content_type":"text/x-python","patch_set":1,"id":"e5d1fab9_7b040387","line":1205,"updated":"2023-09-28 19:08:24.000000000","message":"Same question about constants.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":33871,"name":"Anton Vazhnetsov","display_name":"Anton Vazhnetsov","email":"dragen15051@gmail.com","username":"0x5b"},"change_message_id":"f6e6170a059a2582220e1e8713a7f140f4963365","unresolved":false,"context_lines":[{"line_number":1202,"context_line":"        self.router \u003d router"},{"line_number":1203,"context_line":"        self.prefix \u003d prefix"},{"line_number":1204,"context_line":"        self.nexthop \u003d nexthop"},{"line_number":1205,"context_line":"        self.route_table \u003d route_table or \"\""},{"line_number":1206,"context_line":"        self.if_exists \u003d if_exists"},{"line_number":1207,"context_line":""},{"line_number":1208,"context_line":"    def run_idl(self, txn):"}],"source_content_type":"text/x-python","patch_set":1,"id":"fbc90ffe_68300bf1","line":1205,"in_reply_to":"e5d1fab9_7b040387","updated":"2023-09-28 21:34:33.000000000","message":"Ack","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"ff32b468783c57bf743da763da534ab53089a356","unresolved":true,"context_lines":[{"line_number":1228,"context_line":""},{"line_number":1229,"context_line":""},{"line_number":1230,"context_line":"class LrRouteListCommand(cmd.ReadOnlyCommand):"},{"line_number":1231,"context_line":"    def __init__(self, api, router, route_table):"},{"line_number":1232,"context_line":"        super().__init__(api)"},{"line_number":1233,"context_line":"        self.router \u003d router"},{"line_number":1234,"context_line":"        # NOTE: pass \"\" to get routes of global route table only"}],"source_content_type":"text/x-python","patch_set":1,"id":"d4990cdb_01eafd8c","line":1231,"updated":"2023-09-28 19:08:24.000000000","message":"We can probably go ahead and default `route_table\u003dNone` like the api definition, just to avoid any (very remote) possibility of breakage.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":33871,"name":"Anton Vazhnetsov","display_name":"Anton Vazhnetsov","email":"dragen15051@gmail.com","username":"0x5b"},"change_message_id":"f6e6170a059a2582220e1e8713a7f140f4963365","unresolved":false,"context_lines":[{"line_number":1228,"context_line":""},{"line_number":1229,"context_line":""},{"line_number":1230,"context_line":"class LrRouteListCommand(cmd.ReadOnlyCommand):"},{"line_number":1231,"context_line":"    def __init__(self, api, router, route_table):"},{"line_number":1232,"context_line":"        super().__init__(api)"},{"line_number":1233,"context_line":"        self.router \u003d router"},{"line_number":1234,"context_line":"        # NOTE: pass \"\" to get routes of global route table only"}],"source_content_type":"text/x-python","patch_set":1,"id":"a42eb8a9_63f7cfad","line":1231,"in_reply_to":"d4990cdb_01eafd8c","updated":"2023-09-28 21:34:33.000000000","message":"Ack","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"ff32b468783c57bf743da763da534ab53089a356","unresolved":true,"context_lines":[{"line_number":1231,"context_line":"    def __init__(self, api, router, route_table):"},{"line_number":1232,"context_line":"        super().__init__(api)"},{"line_number":1233,"context_line":"        self.router \u003d router"},{"line_number":1234,"context_line":"        # NOTE: pass \"\" to get routes of global route table only"},{"line_number":1235,"context_line":"        self.route_table \u003d route_table"},{"line_number":1236,"context_line":""},{"line_number":1237,"context_line":"    def run_idl(self, txn):"}],"source_content_type":"text/x-python","patch_set":1,"id":"4a143baf_77725368","line":1234,"updated":"2023-09-28 19:08:24.000000000","message":"I think I understand this, but just to make sure: this says to pass `\"\"` for global table, but the api command that returns this defaults to `None` and line 1239 specifically checks against `is not None`. Am I understanding things correctly that `\"\"` would be the default value in OVN for this and would be the global table, but if we pass `None` the idea is to return all routes as before?","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"},{"author":{"_account_id":33871,"name":"Anton Vazhnetsov","display_name":"Anton Vazhnetsov","email":"dragen15051@gmail.com","username":"0x5b"},"change_message_id":"f6e6170a059a2582220e1e8713a7f140f4963365","unresolved":false,"context_lines":[{"line_number":1231,"context_line":"    def __init__(self, api, router, route_table):"},{"line_number":1232,"context_line":"        super().__init__(api)"},{"line_number":1233,"context_line":"        self.router \u003d router"},{"line_number":1234,"context_line":"        # NOTE: pass \"\" to get routes of global route table only"},{"line_number":1235,"context_line":"        self.route_table \u003d route_table"},{"line_number":1236,"context_line":""},{"line_number":1237,"context_line":"    def run_idl(self, txn):"}],"source_content_type":"text/x-python","patch_set":1,"id":"ef63ea55_2f0db95a","line":1234,"in_reply_to":"4a143baf_77725368","updated":"2023-09-28 21:34:33.000000000","message":"Yes, you\u0027re right, the idea is to keep the old behavior and support the new argument.","commit_id":"1727c23eaadd721c629ff36a5dfa23aa1d7e42d2"}]}