)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"fcb2b4a961c826153edaa7856791adaaf87857f0","unresolved":false,"context_lines":[{"line_number":10,"context_line":"ovs-ofctl is used with the right OF protocol version: the"},{"line_number":11,"context_line":"highest of all versions configured with use_at_least_protocol()."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This use_atl_east_protocol() method keeps track of the highest"},{"line_number":14,"context_line":"version requested, and also calls add_protocols to ensure that"},{"line_number":15,"context_line":"this version is actually available on the bridge."},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"9f436f4f_69013f38","line":13,"range":{"start_line":13,"start_character":5,"end_line":13,"end_character":26},"updated":"2017-08-15 21:12:41.000000000","message":"use_at_least_protocol","commit_id":"600a5c196888498dfb2e3bc12289e3027f75873a"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"61ce0a7f0b89ed4162a943d815c89a5752e45fd6","unresolved":false,"context_lines":[{"line_number":10,"context_line":"ovs-ofctl is used with the right OF protocol version: the"},{"line_number":11,"context_line":"highest of all versions configured with use_at_least_protocol()."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This use_atl_east_protocol() method keeps track of the highest"},{"line_number":14,"context_line":"version requested, and also calls add_protocols to ensure that"},{"line_number":15,"context_line":"this version is actually available on the bridge."},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"9f436f4f_f9704b42","line":13,"range":{"start_line":13,"start_character":5,"end_line":13,"end_character":26},"in_reply_to":"9f436f4f_69013f38","updated":"2017-08-16 13:17:24.000000000","message":"Done","commit_id":"600a5c196888498dfb2e3bc12289e3027f75873a"}],"neutron/agent/common/ovs_lib.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"e9ce66098e9ceb3e8e9d272bc5e5592d0091f59a","unresolved":false,"context_lines":[{"line_number":238,"context_line":"    def add_protocols(self, *protocols):"},{"line_number":239,"context_line":"        for protocol in protocols:"},{"line_number":240,"context_line":"            self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":241,"context_line":"                                                protocol)"},{"line_number":242,"context_line":"        self.ovsdb.db_add(\u0027Bridge\u0027, self.br_name,"},{"line_number":243,"context_line":"                          \u0027protocols\u0027, *protocols).execute(check_error\u003dTrue)"},{"line_number":244,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_516a3a80","line":241,"updated":"2017-04-06 08:49:52.000000000","message":"Nit: Maybe it\u0027s just a preference but I think we can avoid setting highest_protocol all the time by picking the max directly:\n\n protocol_max \u003d max(protocol)\n self._highest_protocol_needed \u003d max(self._highest_protocol_needed, protocol_max)","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"16f4b6ad926ee46ee229bcbba0b267a57f440adc","unresolved":false,"context_lines":[{"line_number":238,"context_line":"    def add_protocols(self, *protocols):"},{"line_number":239,"context_line":"        for protocol in protocols:"},{"line_number":240,"context_line":"            self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":241,"context_line":"                                                protocol)"},{"line_number":242,"context_line":"        self.ovsdb.db_add(\u0027Bridge\u0027, self.br_name,"},{"line_number":243,"context_line":"                          \u0027protocols\u0027, *protocols).execute(check_error\u003dTrue)"},{"line_number":244,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_9169e226","line":241,"in_reply_to":"bff0334d_516a3a80","updated":"2017-04-06 09:21:50.000000000","message":"Since indeed max takes more than 1 parameter, we can even just do:\n\nself._highest_protocol_needed \u003d max(self._highest_protocol_needed, *protocols)\n\nThanks for hinting the right direction...!","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"a526cb18d0ddf1d5c92ea3488722de5b16b50a6d","unresolved":false,"context_lines":[{"line_number":238,"context_line":"    def add_protocols(self, *protocols):"},{"line_number":239,"context_line":"        for protocol in protocols:"},{"line_number":240,"context_line":"            self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":241,"context_line":"                                                protocol)"},{"line_number":242,"context_line":"        self.ovsdb.db_add(\u0027Bridge\u0027, self.br_name,"},{"line_number":243,"context_line":"                          \u0027protocols\u0027, *protocols).execute(check_error\u003dTrue)"},{"line_number":244,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_a6882b1f","line":241,"in_reply_to":"bff0334d_9169e226","updated":"2017-04-06 09:35:04.000000000","message":"Even better! :)","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"e9ce66098e9ceb3e8e9d272bc5e5592d0091f59a","unresolved":false,"context_lines":[{"line_number":393,"context_line":"        flows \u003d self.run_ofctl(\"dump-flows\", [flow_str])"},{"line_number":394,"context_line":"        if flows:"},{"line_number":395,"context_line":"            retval \u003d \u0027\\n\u0027.join(item for item in flows.splitlines()"},{"line_number":396,"context_line":"                               if \u0027NXST\u0027 not in item and \u0027OFPST\u0027 not in item)"},{"line_number":397,"context_line":"        return retval"},{"line_number":398,"context_line":""},{"line_number":399,"context_line":"    def dump_all_flows(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_51b59a30","line":396,"range":{"start_line":396,"start_character":57,"end_line":396,"end_character":64},"updated":"2017-04-06 08:49:52.000000000","message":"Would it make sense to put a comment here explaining what OFPST extension is for noobs like me? :)","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"a526cb18d0ddf1d5c92ea3488722de5b16b50a6d","unresolved":false,"context_lines":[{"line_number":393,"context_line":"        flows \u003d self.run_ofctl(\"dump-flows\", [flow_str])"},{"line_number":394,"context_line":"        if flows:"},{"line_number":395,"context_line":"            retval \u003d \u0027\\n\u0027.join(item for item in flows.splitlines()"},{"line_number":396,"context_line":"                               if \u0027NXST\u0027 not in item and \u0027OFPST\u0027 not in item)"},{"line_number":397,"context_line":"        return retval"},{"line_number":398,"context_line":""},{"line_number":399,"context_line":"    def dump_all_flows(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_26741b35","line":396,"range":{"start_line":396,"start_character":57,"end_line":396,"end_character":64},"in_reply_to":"bff0334d_512b5a93","updated":"2017-04-06 09:35:04.000000000","message":"It\u0027s just that when I\u0027m reading this, I have no idea why do we need to add it.","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"16f4b6ad926ee46ee229bcbba0b267a57f440adc","unresolved":false,"context_lines":[{"line_number":393,"context_line":"        flows \u003d self.run_ofctl(\"dump-flows\", [flow_str])"},{"line_number":394,"context_line":"        if flows:"},{"line_number":395,"context_line":"            retval \u003d \u0027\\n\u0027.join(item for item in flows.splitlines()"},{"line_number":396,"context_line":"                               if \u0027NXST\u0027 not in item and \u0027OFPST\u0027 not in item)"},{"line_number":397,"context_line":"        return retval"},{"line_number":398,"context_line":""},{"line_number":399,"context_line":"    def dump_all_flows(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_512b5a93","line":396,"range":{"start_line":396,"start_character":57,"end_line":396,"end_character":64},"in_reply_to":"bff0334d_51b59a30","updated":"2017-04-06 09:21:50.000000000","message":"Well, to be honest OFPST is not more familiar to me than NXST, I don\u0027t no much more about one than the other.  It\u0027s just that they happen to be string convenient to match the non-interesting lines of ovs-ofctl.\n\nI\u0027ll wrap the test (done in two places) in a function and explain that.","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"142c5f08e3df46843e62f89b626298b4cccc64fa","unresolved":false,"context_lines":[{"line_number":761,"context_line":"        return cookie"},{"line_number":762,"context_line":""},{"line_number":763,"context_line":""},{"line_number":764,"context_line":"def dump_flows_filter(line):"},{"line_number":765,"context_line":"    # we filter out the non-interesting lines (first line at least) of"},{"line_number":766,"context_line":"    # ovs-ofctl dump-flows, which is conveniently done by matching on NXST"},{"line_number":767,"context_line":"    # (for OF1.0) or OFPST (for later versions)"}],"source_content_type":"text/x-python","patch_set":6,"id":"7ffa3b31_36056cc3","line":764,"updated":"2017-04-16 04:12:13.000000000","message":"we should call this something that indicates it returns boolean (e.g. is_flow_valid)","commit_id":"28b147883893d97f102c41dfb2a92710bd98ccb2"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"142c5f08e3df46843e62f89b626298b4cccc64fa","unresolved":false,"context_lines":[{"line_number":762,"context_line":""},{"line_number":763,"context_line":""},{"line_number":764,"context_line":"def dump_flows_filter(line):"},{"line_number":765,"context_line":"    # we filter out the non-interesting lines (first line at least) of"},{"line_number":766,"context_line":"    # ovs-ofctl dump-flows, which is conveniently done by matching on NXST"},{"line_number":767,"context_line":"    # (for OF1.0) or OFPST (for later versions)"},{"line_number":768,"context_line":"    return \u0027NXST\u0027 not in line and \u0027OFPST\u0027 not in line"}],"source_content_type":"text/x-python","patch_set":6,"id":"7ffa3b31_160a70d1","line":765,"updated":"2017-04-16 04:12:13.000000000","message":"this needs explanation of why \u0027NXST\u0027 and \u0027OFPST\u0027 are not \u0027interesting\u0027","commit_id":"28b147883893d97f102c41dfb2a92710bd98ccb2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"517a6519a6642f880e45d298fe931030ac003d9b","unresolved":false,"context_lines":[{"line_number":906,"context_line":"    #  cookie\u003d0xb7dff131a697c6a5, duration\u003d2411726.786s, table\u003d23, ..."},{"line_number":907,"context_line":"    #  cookie\u003d0xb7dff131a697c6a5, duration\u003d2411726.760s, table\u003d24, ..."},{"line_number":908,"context_line":"    #"},{"line_number":909,"context_line":"    return \u0027NXST\u0027 not in line and \u0027OFPST\u0027 not in line"}],"source_content_type":"text/x-python","patch_set":9,"id":"1f1a1f67_1df64ad8","line":909,"range":{"start_line":909,"start_character":11,"end_line":909,"end_character":53},"updated":"2017-07-19 16:27:42.000000000","message":"nit: return [\u0027NXST\u0027, \u0027OFPST\u0027] not in line","commit_id":"37413c8b81c287ebac3fe701e2b80fc39c2519b9"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"3e8211a7c7ee6f791927a276aa7336c0d07c35fa","unresolved":false,"context_lines":[{"line_number":906,"context_line":"    #  cookie\u003d0xb7dff131a697c6a5, duration\u003d2411726.786s, table\u003d23, ..."},{"line_number":907,"context_line":"    #  cookie\u003d0xb7dff131a697c6a5, duration\u003d2411726.760s, table\u003d24, ..."},{"line_number":908,"context_line":"    #"},{"line_number":909,"context_line":"    return \u0027NXST\u0027 not in line and \u0027OFPST\u0027 not in line"}],"source_content_type":"text/x-python","patch_set":9,"id":"1f1a1f67_7d39beef","line":909,"range":{"start_line":909,"start_character":11,"end_line":909,"end_character":53},"in_reply_to":"1f1a1f67_1df64ad8","updated":"2017-07-19 16:34:24.000000000","message":"My bad, previous one is not correct\n\nreturn not any(i in line for i in [\u0027NXST\u0027, \u0027OFPST\u0027])\n\nAnyway, are only two strings.","commit_id":"37413c8b81c287ebac3fe701e2b80fc39c2519b9"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"7cf0b3c087bc1d54aca4ddbedb48efe7f12cd07d","unresolved":false,"context_lines":[{"line_number":237,"context_line":""},{"line_number":238,"context_line":"    def add_protocols(self, *protocols):"},{"line_number":239,"context_line":"        self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":240,"context_line":"                                            *protocols)"},{"line_number":241,"context_line":"        self.ovsdb.db_add(\u0027Bridge\u0027, self.br_name,"},{"line_number":242,"context_line":"                          \u0027protocols\u0027, *protocols).execute(check_error\u003dTrue)"},{"line_number":243,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"ff346bd7_60c5858e","line":240,"updated":"2017-07-23 23:50:54.000000000","message":"add_protocols configures the switch.\ni don\u0027t think it\u0027s a good idea to make it configure our controller as a side effect.\nit\u0027s also confusing that it behaves differently than other methods. (set_protocols, set_db_attribute)\n\nif you want to make it explicit about the version to use for ovs-ofctl, it ought to be a separate configuration i guess.","commit_id":"798ca1401f7a1b0f0153d4bc27cb495464ccb6c8"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"9df99514d75a4fbf8abb786e114ef47de137eaf8","unresolved":false,"context_lines":[{"line_number":237,"context_line":""},{"line_number":238,"context_line":"    def add_protocols(self, *protocols):"},{"line_number":239,"context_line":"        self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":240,"context_line":"                                            *protocols)"},{"line_number":241,"context_line":"        self.ovsdb.db_add(\u0027Bridge\u0027, self.br_name,"},{"line_number":242,"context_line":"                          \u0027protocols\u0027, *protocols).execute(check_error\u003dTrue)"},{"line_number":243,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"bf3e73b7_e60e26e4","line":240,"in_reply_to":"ff346bd7_60c5858e","updated":"2017-08-07 15:48:33.000000000","message":"I agree that it is better to better separate the functionality here.\n\nI\u0027ve posted a new PS that does not change the behavior of add_protocol calls.\n\nInstead of overloading add_protocols behavior, a new method is introduced \u0027use_at_least_protocol\u0027 that controls which version will be used by ovs-ofctl calls, but also calls add_protocols to make sure the requested version is actually usable.","commit_id":"798ca1401f7a1b0f0153d4bc27cb495464ccb6c8"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"491dc7ff4a02e3c1b53ccca768eb8947078f491e","unresolved":false,"context_lines":[{"line_number":895,"context_line":"        return cookie"},{"line_number":896,"context_line":""},{"line_number":897,"context_line":""},{"line_number":898,"context_line":"def is_a_flow_line(line):"},{"line_number":899,"context_line":"    # this is used to filter out from ovs-ofctl dump-flows the lines that"},{"line_number":900,"context_line":"    # are not flow descriptions but mere indications of the type of openflow"},{"line_number":901,"context_line":"    # message that was used ; e.g.:"}],"source_content_type":"text/x-python","patch_set":11,"id":"ff346bd7_604a2528","line":898,"range":{"start_line":898,"start_character":4,"end_line":898,"end_character":18},"updated":"2017-07-23 23:53:23.000000000","message":"i like this. probably this part ought to be a separate patch.","commit_id":"798ca1401f7a1b0f0153d4bc27cb495464ccb6c8"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"9df99514d75a4fbf8abb786e114ef47de137eaf8","unresolved":false,"context_lines":[{"line_number":895,"context_line":"        return cookie"},{"line_number":896,"context_line":""},{"line_number":897,"context_line":""},{"line_number":898,"context_line":"def is_a_flow_line(line):"},{"line_number":899,"context_line":"    # this is used to filter out from ovs-ofctl dump-flows the lines that"},{"line_number":900,"context_line":"    # are not flow descriptions but mere indications of the type of openflow"},{"line_number":901,"context_line":"    # message that was used ; e.g.:"}],"source_content_type":"text/x-python","patch_set":11,"id":"bf3e73b7_c6e1e200","line":898,"range":{"start_line":898,"start_character":4,"end_line":898,"end_character":18},"in_reply_to":"ff346bd7_604a2528","updated":"2017-08-07 15:48:33.000000000","message":"It is part of this patch because, the result of using a version higher than OF10 is that ovs-ofctl dump-flows returns an \"OFPST\" line instead of an \"NXST\" line.","commit_id":"798ca1401f7a1b0f0153d4bc27cb495464ccb6c8"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"fcb2b4a961c826153edaa7856791adaaf87857f0","unresolved":false,"context_lines":[{"line_number":242,"context_line":"    def use_at_least_protocol(self, protocol):"},{"line_number":243,"context_line":"        \"\"\"Calls to ovs-ofctl will use a protocol version \u003e\u003d \u0027protocol\u0027\"\"\""},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        self.add_protocols(protocol)"},{"line_number":246,"context_line":"        self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":247,"context_line":"                                            protocol)"},{"line_number":248,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9f436f4f_cc071918","line":245,"updated":"2017-08-15 21:12:41.000000000","message":"The commit message states: \"calls add_protocols to ensure that this version is actually available on the bridge\". What happens if the protocol is not available on the bridge?","commit_id":"600a5c196888498dfb2e3bc12289e3027f75873a"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"0707f076a3b336a2d9330eede9999ea66ced1a77","unresolved":false,"context_lines":[{"line_number":242,"context_line":"    def use_at_least_protocol(self, protocol):"},{"line_number":243,"context_line":"        \"\"\"Calls to ovs-ofctl will use a protocol version \u003e\u003d \u0027protocol\u0027\"\"\""},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        self.add_protocols(protocol)"},{"line_number":246,"context_line":"        self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":247,"context_line":"                                            protocol)"},{"line_number":248,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9f436f4f_9cb00ddf","line":245,"in_reply_to":"9f436f4f_cc071918","updated":"2017-08-16 13:40:21.000000000","message":"Presumably add_protocol will raise an Exception, and the call to use_at_least_protocol will fail.","commit_id":"600a5c196888498dfb2e3bc12289e3027f75873a"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"124acfadb6a6235a4fef73b725252f1486fc961f","unresolved":false,"context_lines":[{"line_number":243,"context_line":"        \"\"\"Calls to ovs-ofctl will use a protocol version \u003e\u003d \u0027protocol\u0027\"\"\""},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        self.add_protocols(protocol)"},{"line_number":246,"context_line":"        self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":247,"context_line":"                                            protocol)"},{"line_number":248,"context_line":""},{"line_number":249,"context_line":"    def create(self, secure_mode\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf3e73b7_e7973690","line":246,"range":{"start_line":246,"start_character":40,"end_line":246,"end_character":43},"updated":"2017-08-08 00:00:35.000000000","message":"these version constants are strings like \"OpenFlow14\".\nis it intentional to compare them with max()?\ni\u0027m not sure if it works for future versions.","commit_id":"600a5c196888498dfb2e3bc12289e3027f75873a"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"61ce0a7f0b89ed4162a943d815c89a5752e45fd6","unresolved":false,"context_lines":[{"line_number":243,"context_line":"        \"\"\"Calls to ovs-ofctl will use a protocol version \u003e\u003d \u0027protocol\u0027\"\"\""},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        self.add_protocols(protocol)"},{"line_number":246,"context_line":"        self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":247,"context_line":"                                            protocol)"},{"line_number":248,"context_line":""},{"line_number":249,"context_line":"    def create(self, secure_mode\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9f436f4f_39c93358","line":246,"range":{"start_line":246,"start_character":40,"end_line":246,"end_character":43},"in_reply_to":"9f436f4f_8c3eb1be","updated":"2017-08-16 13:17:24.000000000","message":"Yes, something like this.\n\nAnd we need to raise an exception on a unrecognized version string.","commit_id":"600a5c196888498dfb2e3bc12289e3027f75873a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"fcb2b4a961c826153edaa7856791adaaf87857f0","unresolved":false,"context_lines":[{"line_number":243,"context_line":"        \"\"\"Calls to ovs-ofctl will use a protocol version \u003e\u003d \u0027protocol\u0027\"\"\""},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        self.add_protocols(protocol)"},{"line_number":246,"context_line":"        self._highest_protocol_needed \u003d max(self._highest_protocol_needed,"},{"line_number":247,"context_line":"                                            protocol)"},{"line_number":248,"context_line":""},{"line_number":249,"context_line":"    def create(self, secure_mode\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9f436f4f_8c3eb1be","line":246,"range":{"start_line":246,"start_character":40,"end_line":246,"end_character":43},"in_reply_to":"bf3e73b7_e7973690","updated":"2017-08-15 21:12:41.000000000","message":"Maybe use a dictionary to map protocol version strings to integers, so the comparison can be made","commit_id":"600a5c196888498dfb2e3bc12289e3027f75873a"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"6b3bd1632c881435d7db1bde03353eecf3e7c255","unresolved":false,"context_lines":[{"line_number":191,"context_line":""},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"OF_PROTOCOL_TO_VERSION \u003d {"},{"line_number":194,"context_line":"    constants.OPENFLOW10: 1.0,"},{"line_number":195,"context_line":"    constants.OPENFLOW11: 1.1,"},{"line_number":196,"context_line":"    constants.OPENFLOW12: 1.2,"},{"line_number":197,"context_line":"    constants.OPENFLOW13: 1.3,"}],"source_content_type":"text/x-python","patch_set":13,"id":"7f287b81_d53f3cb3","line":194,"range":{"start_line":194,"start_character":26,"end_line":194,"end_character":30},"updated":"2017-09-04 04:43:08.000000000","message":"i\u0027d suggest to use on-wire version numbers.\nie. OPENFLOW10\u003d1, OPENFLOW11\u003d2, ...","commit_id":"b6f7053b95589b3e1f4e981af00a3fae31de3209"}],"neutron/tests/unit/agent/common/test_ovs_lib.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"e9ce66098e9ceb3e8e9d272bc5e5592d0091f59a","unresolved":false,"context_lines":[{"line_number":425,"context_line":"                          self.br.mod_flow,"},{"line_number":426,"context_line":"                          **params)"},{"line_number":427,"context_line":""},{"line_number":428,"context_line":"    def test_ofctl_of_version(self):"},{"line_number":429,"context_line":"        self.br.add_flow(in_port\u003d1, actions\u003d\"drop\")"},{"line_number":430,"context_line":"        self.execute.assert_has_calls(["},{"line_number":431,"context_line":"            mock.call([\u0027ovs-ofctl\u0027, \u0027add-flows\u0027, \u0027-O\u0027, p_const.OPENFLOW10,"}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_318fb624","line":428,"range":{"start_line":428,"start_character":7,"end_line":428,"end_character":29},"updated":"2017-04-06 08:49:52.000000000","message":"I would split the test into three tests:\n\n 1) Make sure that -O \"highes_protocol\" is passed to ovs-ofctl\n\n 2) Test that calling add_protocols with higher protocol than current changes the current\n\n 3) Test that calling add_protocol with lower protocol than current doesn\u0027t change the current","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"a526cb18d0ddf1d5c92ea3488722de5b16b50a6d","unresolved":false,"context_lines":[{"line_number":425,"context_line":"                          self.br.mod_flow,"},{"line_number":426,"context_line":"                          **params)"},{"line_number":427,"context_line":""},{"line_number":428,"context_line":"    def test_ofctl_of_version(self):"},{"line_number":429,"context_line":"        self.br.add_flow(in_port\u003d1, actions\u003d\"drop\")"},{"line_number":430,"context_line":"        self.execute.assert_has_calls(["},{"line_number":431,"context_line":"            mock.call([\u0027ovs-ofctl\u0027, \u0027add-flows\u0027, \u0027-O\u0027, p_const.OPENFLOW10,"}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_26a9bb7a","line":428,"range":{"start_line":428,"start_character":7,"end_line":428,"end_character":29},"in_reply_to":"bff0334d_0362293f","updated":"2017-04-06 09:35:04.000000000","message":"Checking that correct value is passed on a next call is good too. Important thing is to make sure we always keep the highest value.","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"16f4b6ad926ee46ee229bcbba0b267a57f440adc","unresolved":false,"context_lines":[{"line_number":425,"context_line":"                          self.br.mod_flow,"},{"line_number":426,"context_line":"                          **params)"},{"line_number":427,"context_line":""},{"line_number":428,"context_line":"    def test_ofctl_of_version(self):"},{"line_number":429,"context_line":"        self.br.add_flow(in_port\u003d1, actions\u003d\"drop\")"},{"line_number":430,"context_line":"        self.execute.assert_has_calls(["},{"line_number":431,"context_line":"            mock.call([\u0027ovs-ofctl\u0027, \u0027add-flows\u0027, \u0027-O\u0027, p_const.OPENFLOW10,"}],"source_content_type":"text/x-python","patch_set":5,"id":"bff0334d_0362293f","line":428,"range":{"start_line":428,"start_character":7,"end_line":428,"end_character":29},"in_reply_to":"bff0334d_318fb624","updated":"2017-04-06 09:21:50.000000000","message":"I agree to have two separate test cases for 2 and 3, but would prefer in these test to check what version is passed to ovs-ofctl rather than looking at the _highest_protocol_version in the internals of OVSBridge. So in practice this means that 1 would be done in 2 and 3.\n\nIs it ok, or do you think there is a good reason to not look at ovs-ofctl in 2 and 3 and that having the unit test look at an implementation detail is a good idea ?","commit_id":"313e1438dbe50c55531ac8b87fdf84bd2744452c"}]}