)]}'
{"quantum/agent/l3_agent.py":[{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"unresolved":false,"context_lines":[{"line_number":416,"context_line":""},{"line_number":417,"context_line":"        gw_ip \u003d ex_gw_port[\u0027subnet\u0027][\u0027gateway_ip\u0027]"},{"line_number":418,"context_line":"        if ex_gw_port[\u0027subnet\u0027][\u0027gateway_ip\u0027]:"},{"line_number":419,"context_line":"            namespace \u003d ri.ns_name() if self.conf.use_namespaces else None"},{"line_number":420,"context_line":"            ip \u003d ip_lib.IPWrapper(self.root_helper, namespace)"},{"line_number":421,"context_line":"            ip.route.add_gateway(gw_ip)"},{"line_number":422,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAOH%2F%2FtgQ%3D","line":419,"updated":"2013-04-02 01:14:31.000000000","message":"(Suggestion only) Given that namespaces are either enabled or disabled for a given process - it just needs to be configurable at run- or test-time - can you think of a way to configure ip_lib directly and avoid the checks in code for use_namespaces?","commit_id":"865fe18cc3d01158f4f1911954bd198449c3fe31"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":416,"context_line":""},{"line_number":417,"context_line":"        gw_ip \u003d ex_gw_port[\u0027subnet\u0027][\u0027gateway_ip\u0027]"},{"line_number":418,"context_line":"        if ex_gw_port[\u0027subnet\u0027][\u0027gateway_ip\u0027]:"},{"line_number":419,"context_line":"            namespace \u003d ri.ns_name() if self.conf.use_namespaces else None"},{"line_number":420,"context_line":"            ip \u003d ip_lib.IPWrapper(self.root_helper, namespace)"},{"line_number":421,"context_line":"            ip.route.add_gateway(gw_ip)"},{"line_number":422,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAOH%2F%2Fs%2BA%3D","line":419,"in_reply_to":"AAAAOH%2F%2FtgQ%3D","updated":"2013-04-02 13:41:39.000000000","message":"Done. Actually, ns_name() method returns None if namespaces aren\u0027t used. I think we can use this fact to avoid the checks in code for use_namespaces.","commit_id":"865fe18cc3d01158f4f1911954bd198449c3fe31"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"unresolved":false,"context_lines":[{"line_number":639,"context_line":"    def after_start(self):"},{"line_number":640,"context_line":"        LOG.info(_(\"L3 agent started\"))"},{"line_number":641,"context_line":""},{"line_number":642,"context_line":"    def _update_routing_table(self, ri, operation, route):"},{"line_number":643,"context_line":"        ip \u003d ip_lib.IPWrapper(self.conf.root_helper, ri.ns_name())"},{"line_number":644,"context_line":""},{"line_number":645,"context_line":"        if operation \u003d\u003d \u0027replace\u0027:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOH%2F%2FszU%3D","line":642,"updated":"2013-04-02 14:39:32.000000000","message":"Consider dropping this method and doing the following:\n\n1. Call ip \u003d ip_lib.IPWrapper(self.conf.root_helper, ri.ns_name()) on line 656\n2. Then invoke the route.add or route.delete.\n\nThis will save a number of function calls and if\u0027s","commit_id":"12f9cbd25b3a5ea212f370616dc61d827a17eeb9"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":639,"context_line":"    def after_start(self):"},{"line_number":640,"context_line":"        LOG.info(_(\"L3 agent started\"))"},{"line_number":641,"context_line":""},{"line_number":642,"context_line":"    def _update_routing_table(self, ri, operation, route):"},{"line_number":643,"context_line":"        ip \u003d ip_lib.IPWrapper(self.conf.root_helper, ri.ns_name())"},{"line_number":644,"context_line":""},{"line_number":645,"context_line":"        if operation \u003d\u003d \u0027replace\u0027:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOX%2F%2F%2FpI%3D","line":642,"in_reply_to":"AAAAOH%2F%2Fso8%3D","updated":"2013-04-03 09:41:07.000000000","message":"Done","commit_id":"12f9cbd25b3a5ea212f370616dc61d827a17eeb9"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":639,"context_line":"    def after_start(self):"},{"line_number":640,"context_line":"        LOG.info(_(\"L3 agent started\"))"},{"line_number":641,"context_line":""},{"line_number":642,"context_line":"    def _update_routing_table(self, ri, operation, route):"},{"line_number":643,"context_line":"        ip \u003d ip_lib.IPWrapper(self.conf.root_helper, ri.ns_name())"},{"line_number":644,"context_line":""},{"line_number":645,"context_line":"        if operation \u003d\u003d \u0027replace\u0027:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOH%2F%2Fso8%3D","line":642,"in_reply_to":"AAAAOH%2F%2FszU%3D","updated":"2013-04-02 15:51:05.000000000","message":"Good suggestion! Will fix that in the next patch set.","commit_id":"12f9cbd25b3a5ea212f370616dc61d827a17eeb9"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"unresolved":false,"context_lines":[{"line_number":652,"context_line":"            for del_route in removes:"},{"line_number":653,"context_line":"                if route[\u0027destination\u0027] \u003d\u003d del_route[\u0027destination\u0027]:"},{"line_number":654,"context_line":"                    removes.remove(del_route)"},{"line_number":655,"context_line":"            #replace success even if there is no existing route"},{"line_number":656,"context_line":"            ip.route.add(route[\u0027destination\u0027], route[\u0027nexthop\u0027])"},{"line_number":657,"context_line":"        for route in removes:"},{"line_number":658,"context_line":"            LOG.debug(_(\"Removed route entry is \u0027%s\u0027\"), route)"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAOX%2F%2F%2FXw%3D","line":655,"updated":"2013-04-03 13:41:06.000000000","message":"This comment describes why \"route replace\" can be used, so it is better to move this comment to ip_lib too.","commit_id":"fd80ddaab53b0f580a9b589978b0971908bfa1a0"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":652,"context_line":"            for del_route in removes:"},{"line_number":653,"context_line":"                if route[\u0027destination\u0027] \u003d\u003d del_route[\u0027destination\u0027]:"},{"line_number":654,"context_line":"                    removes.remove(del_route)"},{"line_number":655,"context_line":"            #replace success even if there is no existing route"},{"line_number":656,"context_line":"            ip.route.add(route[\u0027destination\u0027], route[\u0027nexthop\u0027])"},{"line_number":657,"context_line":"        for route in removes:"},{"line_number":658,"context_line":"            LOG.debug(_(\"Removed route entry is \u0027%s\u0027\"), route)"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAOX%2F%2F%2FRE%3D","line":655,"in_reply_to":"AAAAOX%2F%2F%2FXw%3D","updated":"2013-04-03 14:22:17.000000000","message":"Done","commit_id":"fd80ddaab53b0f580a9b589978b0971908bfa1a0"}],"quantum/agent/linux/ip_lib.py":[{"author":{"_account_id":2592,"name":"Mark McClain","email":"mark@mcclain.xyz","username":"markmcclain"},"change_message_id":"d43d6f3ac7e5398fc5ec59c9fb5b505ab62e2551","unresolved":false,"context_lines":[{"line_number":393,"context_line":"                                  \u0027dev\u0027, device)"},{"line_number":394,"context_line":""},{"line_number":395,"context_line":""},{"line_number":396,"context_line":"class IpRouteDeviceCommand(IpDeviceCommandBase, IpRouteCommand):"},{"line_number":397,"context_line":"    \"\"\"The wrapper for ip route actions which are bound to a specific device\"\"\""},{"line_number":398,"context_line":""},{"line_number":399,"context_line":"    def add_gateway(self, gw, metric\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOH%2F%2F3rg%3D","line":396,"updated":"2013-03-22 18:12:05.000000000","message":"I think a common base might make sense.  When you inherit IpRouteCommand, the add_route and delete_route commands are included which impact all devices.","commit_id":"61f872679795da64d085135d59d1e6f732747f63"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":393,"context_line":"                                  \u0027dev\u0027, device)"},{"line_number":394,"context_line":""},{"line_number":395,"context_line":""},{"line_number":396,"context_line":"class IpRouteDeviceCommand(IpDeviceCommandBase, IpRouteCommand):"},{"line_number":397,"context_line":"    \"\"\"The wrapper for ip route actions which are bound to a specific device\"\"\""},{"line_number":398,"context_line":""},{"line_number":399,"context_line":"    def add_gateway(self, gw, metric\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOH%2F%2F2Cc%3D","line":396,"in_reply_to":"AAAAOH%2F%2F3rg%3D","updated":"2013-03-25 12:18:31.000000000","message":"Replaced inheritance with aggregation. A little bit more code, but looks nicer.","commit_id":"61f872679795da64d085135d59d1e6f732747f63"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"unresolved":false,"context_lines":[{"line_number":384,"context_line":"                    self._as_root(\u0027append\u0027, subnet, \u0027proto\u0027, \u0027kernel\u0027,"},{"line_number":385,"context_line":"                                  \u0027dev\u0027, device)"},{"line_number":386,"context_line":""},{"line_number":387,"context_line":"    def add_route(self, destination, nexthop, dev\u003dNone):"},{"line_number":388,"context_line":"        args \u003d [\u0027replace\u0027, \u0027to\u0027, destination, \u0027via\u0027, nexthop]"},{"line_number":389,"context_line":"        if dev:"},{"line_number":390,"context_line":"            args +\u003d [\u0027dev\u0027, dev]"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAOH%2F%2Ftf8%3D","line":387,"updated":"2013-04-02 01:14:31.000000000","message":"(Suggestion only) Certainly the _gateway suffix is necessary, but given that the object type is ~ \u0027route\u0027, having a \u0027_route\u0027 suffix would seem redundant.  Consider removing the suffix from add_route and delete_route and doing the same for the methods in IpRouteDeviceCommand.\n\nThere was a great talk at pycon on this very topic, if you\u0027re both extremely bored and enjoy Lewis Carroll: http://pyvideo.org/video/1676/the-naming-of-ducks-where-dynamic-types-meet-sma","commit_id":"865fe18cc3d01158f4f1911954bd198449c3fe31"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":384,"context_line":"                    self._as_root(\u0027append\u0027, subnet, \u0027proto\u0027, \u0027kernel\u0027,"},{"line_number":385,"context_line":"                                  \u0027dev\u0027, device)"},{"line_number":386,"context_line":""},{"line_number":387,"context_line":"    def add_route(self, destination, nexthop, dev\u003dNone):"},{"line_number":388,"context_line":"        args \u003d [\u0027replace\u0027, \u0027to\u0027, destination, \u0027via\u0027, nexthop]"},{"line_number":389,"context_line":"        if dev:"},{"line_number":390,"context_line":"            args +\u003d [\u0027dev\u0027, dev]"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAOH%2F%2Fs9k%3D","line":387,"in_reply_to":"AAAAOH%2F%2Ftf8%3D","updated":"2013-04-02 13:41:39.000000000","message":"Done","commit_id":"865fe18cc3d01158f4f1911954bd198449c3fe31"}],"quantum/tests/unit/test_l3_agent.py":[{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"unresolved":false,"context_lines":[{"line_number":239,"context_line":"                       \u0027nexthop\u0027: \u00271.2.3.4\u0027}"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":"        agent._update_routing_table(ri, \u0027replace\u0027, fake_route1)"},{"line_number":242,"context_line":"        expected \u003d [[\u0027135.207.0.0/16\u0027, \u00271.2.3.4\u0027]]"},{"line_number":243,"context_line":"        self._check_route_calls(\u0027replace\u0027, expected, ri.ns_name())"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        agent._update_routing_table(ri, \u0027delete\u0027, fake_route1)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAOH%2F%2Ftek%3D","line":242,"updated":"2013-04-02 01:14:31.000000000","message":"(Suggestion only) Apologies, since this comment has nothing to do with your perfectly good changes, but this test case is terrible.  The method under test is update_routing_table, and since that method does not hold state, complete coverage could be achieved by one call with namespaces and another without.  Consider cleaning this method up, in this change or a subsequent one for...er...the benefit of all mankind.  Or failing that, my tender sensibilities.","commit_id":"865fe18cc3d01158f4f1911954bd198449c3fe31"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":239,"context_line":"                       \u0027nexthop\u0027: \u00271.2.3.4\u0027}"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":"        agent._update_routing_table(ri, \u0027replace\u0027, fake_route1)"},{"line_number":242,"context_line":"        expected \u003d [[\u0027135.207.0.0/16\u0027, \u00271.2.3.4\u0027]]"},{"line_number":243,"context_line":"        self._check_route_calls(\u0027replace\u0027, expected, ri.ns_name())"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        agent._update_routing_table(ri, \u0027delete\u0027, fake_route1)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAOH%2F%2Fs8Q%3D","line":242,"in_reply_to":"AAAAOH%2F%2Ftek%3D","updated":"2013-04-02 13:41:39.000000000","message":"Maru, you mean that fake_route2 and operations on it are useless and fake_route1 would be enough, don\u0027t you?","commit_id":"865fe18cc3d01158f4f1911954bd198449c3fe31"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"unresolved":false,"context_lines":[{"line_number":234,"context_line":"                                 None)"},{"line_number":235,"context_line":"        agent \u003d l3_agent.L3NATAgent(HOSTNAME, self.conf)"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"        fake_route \u003d {\u0027destination\u0027: \u0027135.207.0.0/16\u0027,"},{"line_number":238,"context_line":"                      \u0027nexthop\u0027: \u00271.2.3.4\u0027}"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        agent._update_routing_table(ri, \u0027replace\u0027, fake_route)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOH%2F%2FsQA%3D","line":237,"updated":"2013-04-02 21:29:58.000000000","message":"(Suggestion only) This is definitely an improvement, but I still recommend a simpler solution: \n\n - Scrap the existing tests (_test_routing_table_update, testAgentRoutingTableUpdated, and testAgentRoutingTableUpdatedNoNamespace).  \n - Add a test for each of the execution paths of _update_routing_table (i.e. test_update_routing_table_replacement, etc).\n\nThe existing tests are unfocused and it isn\u0027t very clear from looking at them what the test target is.  The suggested approach will remedy that.","commit_id":"12f9cbd25b3a5ea212f370616dc61d827a17eeb9"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":234,"context_line":"                                 None)"},{"line_number":235,"context_line":"        agent \u003d l3_agent.L3NATAgent(HOSTNAME, self.conf)"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"        fake_route \u003d {\u0027destination\u0027: \u0027135.207.0.0/16\u0027,"},{"line_number":238,"context_line":"                      \u0027nexthop\u0027: \u00271.2.3.4\u0027}"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        agent._update_routing_table(ri, \u0027replace\u0027, fake_route)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOX%2F%2F%2FjU%3D","line":237,"in_reply_to":"AAAAOH%2F%2FsQA%3D","updated":"2013-04-03 09:41:07.000000000","message":"_update_routing_table() method is gone, so I think it\u0027s kind of fixed now :)","commit_id":"12f9cbd25b3a5ea212f370616dc61d827a17eeb9"}],"quantum/tests/unit/test_linux_ip_lib.py":[{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"unresolved":false,"context_lines":[{"line_number":648,"context_line":"        super(TestIpRouteDeviceCommand, self).setUp()"},{"line_number":649,"context_line":"        self.parent.name \u003d \u0027eth0\u0027"},{"line_number":650,"context_line":"        self.command \u003d \u0027route\u0027"},{"line_number":651,"context_line":"        self.route_cmd \u003d ip_lib.IpRouteDeviceCommand(self.parent)"},{"line_number":652,"context_line":""},{"line_number":653,"context_line":"    def test_add_gateway(self):"},{"line_number":654,"context_line":"        gw \u003d \u002710.0.0.1\u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAOH%2F%2FtfA%3D","line":651,"updated":"2013-04-02 01:14:31.000000000","message":"(Suggestion only) Given that the commands all execute against the _route attribute of IpRouteDeviceCommand, consider using the simpler option of mocking out self.route_cmd._route in setUp instead of mocking out the specific method in each test.","commit_id":"865fe18cc3d01158f4f1911954bd198449c3fe31"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":648,"context_line":"        super(TestIpRouteDeviceCommand, self).setUp()"},{"line_number":649,"context_line":"        self.parent.name \u003d \u0027eth0\u0027"},{"line_number":650,"context_line":"        self.command \u003d \u0027route\u0027"},{"line_number":651,"context_line":"        self.route_cmd \u003d ip_lib.IpRouteDeviceCommand(self.parent)"},{"line_number":652,"context_line":""},{"line_number":653,"context_line":"    def test_add_gateway(self):"},{"line_number":654,"context_line":"        gw \u003d \u002710.0.0.1\u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAOH%2F%2Fs78%3D","line":651,"in_reply_to":"AAAAOH%2F%2FtfA%3D","updated":"2013-04-02 13:41:39.000000000","message":"Done","commit_id":"865fe18cc3d01158f4f1911954bd198449c3fe31"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"unresolved":false,"context_lines":[{"line_number":589,"context_line":"                           \u0027metric\u0027, metric,"},{"line_number":590,"context_line":"                           \u0027dev\u0027, dev))"},{"line_number":591,"context_line":""},{"line_number":592,"context_line":"    def test_del_gateway_with_dev(self):"},{"line_number":593,"context_line":"        gateway \u003d \u0027192.168.45.100\u0027"},{"line_number":594,"context_line":"        dev \u003d \u0027eth0\u0027"},{"line_number":595,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOH%2F%2FseA%3D","line":592,"updated":"2013-04-02 21:29:58.000000000","message":"(Suggestion only) Minor nit, but this test name would ideally reflect the name of the method that is being tested (i.e. test_delete_gateway_with_dev).  I know this was an existing test name, but it and \u0027test_delete_gateway\u0027 should follow the same convention.","commit_id":"12f9cbd25b3a5ea212f370616dc61d827a17eeb9"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"2b829f09c569cc0d7acc5a30ef988dec7bf97934","unresolved":false,"context_lines":[{"line_number":589,"context_line":"                           \u0027metric\u0027, metric,"},{"line_number":590,"context_line":"                           \u0027dev\u0027, dev))"},{"line_number":591,"context_line":""},{"line_number":592,"context_line":"    def test_del_gateway_with_dev(self):"},{"line_number":593,"context_line":"        gateway \u003d \u0027192.168.45.100\u0027"},{"line_number":594,"context_line":"        dev \u003d \u0027eth0\u0027"},{"line_number":595,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOX%2F%2F%2FpM%3D","line":592,"in_reply_to":"AAAAOH%2F%2FseA%3D","updated":"2013-04-03 09:41:07.000000000","message":"Done","commit_id":"12f9cbd25b3a5ea212f370616dc61d827a17eeb9"}]}