)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"109d88a48037f154d57a80af658a4931d73dccb1","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     zhhuabj \u003czhhuabj@cn.ibm.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2013-11-18 18:09:54 +0800"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Fwaas can\u0027t run in operation system without namespace feature"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Change-Id: If8edb5c0bb0fc0fd9aaf48a3441287f99bcdcf13"},{"line_number":10,"context_line":"Closes-Bug: #1252201"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"AAAATn%2F%2B7kY%3D","line":7,"updated":"2013-11-18 13:17:15.000000000","message":"Nit: operation -\u003e operating?\n\nCould use more of a description on what is being done.","commit_id":"1f545999ce132d164d754ef144ef4e584473fa4d"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"acb40f545e3205b01269664344b34907f49e7849","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     zhhuabj \u003czhhuabj@cn.ibm.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2013-11-18 18:09:54 +0800"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Fwaas can\u0027t run in operation system without namespace feature"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Change-Id: If8edb5c0bb0fc0fd9aaf48a3441287f99bcdcf13"},{"line_number":10,"context_line":"Closes-Bug: #1252201"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"AAAATn%2F%2B3V4%3D","line":7,"in_reply_to":"AAAATn%2F%2B7kY%3D","updated":"2013-11-19 07:46:23.000000000","message":"Done","commit_id":"1f545999ce132d164d754ef144ef4e584473fa4d"}],"neutron/services/firewall/agents/l3reference/firewall_l3_agent.py":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"109d88a48037f154d57a80af658a4931d73dccb1","unresolved":false,"context_lines":[{"line_number":89,"context_line":"            router[\u0027id\u0027]"},{"line_number":90,"context_line":"            for router in routers"},{"line_number":91,"context_line":"            if router[\u0027tenant_id\u0027] \u003d\u003d tenant_id]"},{"line_number":92,"context_line":"        if(self.conf.use_namespaces):"},{"line_number":93,"context_line":"            local_ns_list \u003d root_ip.get_namespaces(self.root_helper)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"        router_info_list \u003d []"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F%2B7ko%3D","line":92,"updated":"2013-11-18 13:17:15.000000000","message":"Couple of questions...\n\nIf not using namespaces, should this function return an empty list? If so, can you short circuit all processing by returning immediately, if not using namespaces? Also, should there be some logging that (requrired?) namespaces are missing?\n\nIf the function is supposed to produce a list, even when not using namespaces, then shouldn\u0027t local_ns_list be set to an empty list to prevent an exception on L 100?\n\nAre there UT cases for both the case of using and not using namespaces to verify that this fix works?","commit_id":"1f545999ce132d164d754ef144ef4e584473fa4d"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"acb40f545e3205b01269664344b34907f49e7849","unresolved":false,"context_lines":[{"line_number":89,"context_line":"            router[\u0027id\u0027]"},{"line_number":90,"context_line":"            for router in routers"},{"line_number":91,"context_line":"            if router[\u0027tenant_id\u0027] \u003d\u003d tenant_id]"},{"line_number":92,"context_line":"        if(self.conf.use_namespaces):"},{"line_number":93,"context_line":"            local_ns_list \u003d root_ip.get_namespaces(self.root_helper)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"        router_info_list \u003d []"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F%2B3Vc%3D","line":92,"in_reply_to":"AAAATn%2F%2B59E%3D","updated":"2013-11-19 07:46:23.000000000","message":"Done","commit_id":"1f545999ce132d164d754ef144ef4e584473fa4d"},{"author":{"_account_id":6995,"name":"Sridar Kandaswamy","email":"skandasw@yahoo.com","username":"skandasw"},"change_message_id":"181d79445f059ddb649c8c3295ceced88e30a0f3","unresolved":false,"context_lines":[{"line_number":89,"context_line":"            router[\u0027id\u0027]"},{"line_number":90,"context_line":"            for router in routers"},{"line_number":91,"context_line":"            if router[\u0027tenant_id\u0027] \u003d\u003d tenant_id]"},{"line_number":92,"context_line":"        if(self.conf.use_namespaces):"},{"line_number":93,"context_line":"            local_ns_list \u003d root_ip.get_namespaces(self.root_helper)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"        router_info_list \u003d []"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F%2B59E%3D","line":92,"in_reply_to":"AAAATn%2F%2B6LE%3D","updated":"2013-11-18 18:30:10.000000000","message":"In terms of functionality, adding the check before generating the local_ns_list as u have done should fix the issue. \n\nPerhaps refactoring this into 2 blocks with first handling the namespace case completely and the else block handling the case without namespaces and returning the routerid(s) will make it cleaner. \n\nMost of our testing was done with namespaces and that never exposed the issue - so pls do add a UT for this.","commit_id":"1f545999ce132d164d754ef144ef4e584473fa4d"},{"author":{"_account_id":490,"name":"Sumit Naiksatam","email":"sumitnaiksatam@gmail.com","username":"snaiksat"},"change_message_id":"a99ced2daeb4097949c60f5cc52f30351ef098e3","unresolved":false,"context_lines":[{"line_number":89,"context_line":"            router[\u0027id\u0027]"},{"line_number":90,"context_line":"            for router in routers"},{"line_number":91,"context_line":"            if router[\u0027tenant_id\u0027] \u003d\u003d tenant_id]"},{"line_number":92,"context_line":"        if(self.conf.use_namespaces):"},{"line_number":93,"context_line":"            local_ns_list \u003d root_ip.get_namespaces(self.root_helper)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"        router_info_list \u003d []"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F%2B6LE%3D","line":92,"in_reply_to":"AAAATn%2F%2B7ko%3D","updated":"2013-11-18 17:33:49.000000000","message":"I agree that UTs are required. I am not as much sure that a log statement is required, since operation without namespace is a legit mode of operation, and we do not log in every other place to indicate that a namespace is not being used. I would not be averse to a debug log here though.\n\nI don\u0027t think the function can be entirely skipped, but in the case when namespaces are not being used, there is still the possibility of using one tenant router, which should be returned by this function. Sridar will be able to chime in here.","commit_id":"1f545999ce132d164d754ef144ef4e584473fa4d"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"5b6a5888a8fc733bc9a1073fe551934faa689a7e","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        router_info_list \u003d []"},{"line_number":96,"context_line":"        # Pick up namespaces for Tenant Routers"},{"line_number":97,"context_line":"        for rid in router_ids:"},{"line_number":98,"context_line":"            if self.router_info[rid].use_namespaces:"},{"line_number":99,"context_line":"                router_ns \u003d self.router_info[rid].ns_name()"},{"line_number":100,"context_line":"                if router_ns in local_ns_list:"},{"line_number":101,"context_line":"                    router_info_list.append(self.router_info[rid])"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2B2Jg%3D","line":98,"updated":"2013-11-19 12:52:51.000000000","message":"Can routers be configured to use namespaces if self.conf_use_namespaces is False?","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"6d4f3cf442dee0faa698dc80c2ea3b83841e9f71","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        router_info_list \u003d []"},{"line_number":96,"context_line":"        # Pick up namespaces for Tenant Routers"},{"line_number":97,"context_line":"        for rid in router_ids:"},{"line_number":98,"context_line":"            if self.router_info[rid].use_namespaces:"},{"line_number":99,"context_line":"                router_ns \u003d self.router_info[rid].ns_name()"},{"line_number":100,"context_line":"                if router_ns in local_ns_list:"},{"line_number":101,"context_line":"                    router_info_list.append(self.router_info[rid])"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2BycU%3D","line":98,"in_reply_to":"AAAATn%2F%2B2Dc%3D","updated":"2013-11-20 07:11:19.000000000","message":"Done","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a7c23d3e0c483d6d84cf9fc08be1eead644fc325","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        router_info_list \u003d []"},{"line_number":96,"context_line":"        # Pick up namespaces for Tenant Routers"},{"line_number":97,"context_line":"        for rid in router_ids:"},{"line_number":98,"context_line":"            if self.router_info[rid].use_namespaces:"},{"line_number":99,"context_line":"                router_ns \u003d self.router_info[rid].ns_name()"},{"line_number":100,"context_line":"                if router_ns in local_ns_list:"},{"line_number":101,"context_line":"                    router_info_list.append(self.router_info[rid])"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2B2Dc%3D","line":98,"in_reply_to":"AAAATn%2F%2B2Jg%3D","updated":"2013-11-19 13:01:04.000000000","message":"Yes, I have the same question. If so, then should have additional unit tests checking those combinations.","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"d93b5870c5e7d3960400eafce87f539a585c5bc9","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        router_info_list \u003d []"},{"line_number":96,"context_line":"        # Pick up namespaces for Tenant Routers"},{"line_number":97,"context_line":"        for rid in router_ids:"},{"line_number":98,"context_line":"            if self.router_info[rid].use_namespaces:"},{"line_number":99,"context_line":"                router_ns \u003d self.router_info[rid].ns_name()"},{"line_number":100,"context_line":"                if router_ns in local_ns_list:"},{"line_number":101,"context_line":"                    router_info_list.append(self.router_info[rid])"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F%2ByBs%3D","line":98,"updated":"2013-11-20 09:28:42.000000000","message":"I\u0027m afraid my question remains unanswered.  Is the intention of this method to ignore routers that are configured to use namespaces when namespace usage is disabled via conf?  My apologies if this should be obvious.","commit_id":"64dddeef9fd107cae227f45aed8950652c3c588d"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"7e33e849961e3ea46c4320376a2b8ed0714bdbe5","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        router_info_list \u003d []"},{"line_number":96,"context_line":"        # Pick up namespaces for Tenant Routers"},{"line_number":97,"context_line":"        for rid in router_ids:"},{"line_number":98,"context_line":"            if self.router_info[rid].use_namespaces:"},{"line_number":99,"context_line":"                router_ns \u003d self.router_info[rid].ns_name()"},{"line_number":100,"context_line":"                if router_ns in local_ns_list:"},{"line_number":101,"context_line":"                    router_info_list.append(self.router_info[rid])"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F%2Bxrw%3D","line":98,"in_reply_to":"AAAATn%2F%2ByBs%3D","updated":"2013-11-20 10:51:48.000000000","message":"hi Maru, thanks for your review.\nNow because neutron doesn\u0027t support multi-host feature, it\u0027s impossible to appear the situation that routers be configured to use namespaces while self.conf_use_namespaces is False.\nBut I think this kind of situation will appear in the future when multi-host feature is ready, some routers on different nodes support namespace, while some routers on different nodes doesn\u0027t support namespace, the network-schedule should support scheduling the tenant network into the node with the same namespace type in the future, otherwize two VMs with the same tenant will not be able to communicate.\nHowever, adding the check before generating the local_ns_list as I have done can fix the issue under all those cases of both now and future.\nso I add a additional unit tests test_get_router_info_list_for_tenant_for_namespaces_combinations to cover it in the latest patch set. thanks.","commit_id":"64dddeef9fd107cae227f45aed8950652c3c588d"}],"neutron/tests/unit/services/firewall/agents/l3reference/test_firewall_l3_agent.py":[{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"5b6a5888a8fc733bc9a1073fe551934faa689a7e","unresolved":false,"context_lines":[{"line_number":269,"context_line":"                ctx,"},{"line_number":270,"context_line":"                fake_firewall_list[0][\u0027id\u0027])"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    def _prepare_router_data(self):"},{"line_number":273,"context_line":"        ex_gw_port \u003d {\u0027id\u0027: _uuid(),"},{"line_number":274,"context_line":"                      \u0027network_id\u0027: _uuid(),"},{"line_number":275,"context_line":"                      \u0027fixed_ips\u0027: [{\u0027ip_address\u0027: \u0027172.24.4.2\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2B2FA%3D","line":272,"updated":"2013-11-19 12:52:51.000000000","message":"This fixture method seems excessive given the limited  requirements of _get_router_info_list, which only needs records with an id and tenant_id.  Unit tests are whitebox, so it makes sense to use minimal fixtures according to what the code will actually use.","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"6d4f3cf442dee0faa698dc80c2ea3b83841e9f71","unresolved":false,"context_lines":[{"line_number":269,"context_line":"                ctx,"},{"line_number":270,"context_line":"                fake_firewall_list[0][\u0027id\u0027])"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    def _prepare_router_data(self):"},{"line_number":273,"context_line":"        ex_gw_port \u003d {\u0027id\u0027: _uuid(),"},{"line_number":274,"context_line":"                      \u0027network_id\u0027: _uuid(),"},{"line_number":275,"context_line":"                      \u0027fixed_ips\u0027: [{\u0027ip_address\u0027: \u0027172.24.4.2\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2BycQ%3D","line":272,"in_reply_to":"AAAATn%2F%2B2FA%3D","updated":"2013-11-20 07:11:19.000000000","message":"Done","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a7c23d3e0c483d6d84cf9fc08be1eead644fc325","unresolved":false,"context_lines":[{"line_number":287,"context_line":"            \u0027enable_snat\u0027: True,"},{"line_number":288,"context_line":"            \u0027gw_port\u0027: ex_gw_port}"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"        ri \u003d l3_agent.RouterInfo(router[\u0027id\u0027], self.conf.root_helper,"},{"line_number":291,"context_line":"                                 self.conf.use_namespaces, router\u003drouter)"},{"line_number":292,"context_line":"        return ri"},{"line_number":293,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2B2Do%3D","line":290,"updated":"2013-11-19 13:01:04.000000000","message":"Could just do...\n\nreturn l3_agent.RouterInfo(...)","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"6d4f3cf442dee0faa698dc80c2ea3b83841e9f71","unresolved":false,"context_lines":[{"line_number":287,"context_line":"            \u0027enable_snat\u0027: True,"},{"line_number":288,"context_line":"            \u0027gw_port\u0027: ex_gw_port}"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"        ri \u003d l3_agent.RouterInfo(router[\u0027id\u0027], self.conf.root_helper,"},{"line_number":291,"context_line":"                                 self.conf.use_namespaces, router\u003drouter)"},{"line_number":292,"context_line":"        return ri"},{"line_number":293,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2BycM%3D","line":290,"in_reply_to":"AAAATn%2F%2B2Do%3D","updated":"2013-11-20 07:11:19.000000000","message":"Done","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"5b6a5888a8fc733bc9a1073fe551934faa689a7e","unresolved":false,"context_lines":[{"line_number":291,"context_line":"                                 self.conf.use_namespaces, router\u003drouter)"},{"line_number":292,"context_line":"        return ri"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"    def test_get_router_info_list_for_tenant_without_namespace(self):"},{"line_number":295,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, False)"},{"line_number":296,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":297,"context_line":"        routers \u003d []"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2B2JM%3D","line":294,"updated":"2013-11-19 12:52:51.000000000","message":"I can\u0027t argue against test additions, but I would encourage you to generalize the test for get_router_info_list to test with namespaces enabled first and then add a second test that only checks that root_ip.get_namespaces() is not called (via mock.patch) if namespaces are disabled by conf.","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"6d4f3cf442dee0faa698dc80c2ea3b83841e9f71","unresolved":false,"context_lines":[{"line_number":291,"context_line":"                                 self.conf.use_namespaces, router\u003drouter)"},{"line_number":292,"context_line":"        return ri"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"    def test_get_router_info_list_for_tenant_without_namespace(self):"},{"line_number":295,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, False)"},{"line_number":296,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":297,"context_line":"        routers \u003d []"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2BycI%3D","line":294,"in_reply_to":"AAAATn%2F%2B2DM%3D","updated":"2013-11-20 07:11:19.000000000","message":"Done","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a7c23d3e0c483d6d84cf9fc08be1eead644fc325","unresolved":false,"context_lines":[{"line_number":291,"context_line":"                                 self.conf.use_namespaces, router\u003drouter)"},{"line_number":292,"context_line":"        return ri"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"    def test_get_router_info_list_for_tenant_without_namespace(self):"},{"line_number":295,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, False)"},{"line_number":296,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":297,"context_line":"        routers \u003d []"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2B2DM%3D","line":294,"in_reply_to":"AAAATn%2F%2B2JM%3D","updated":"2013-11-19 13:01:04.000000000","message":"For completeness, consider adding a test case with namespaces.","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a7c23d3e0c483d6d84cf9fc08be1eead644fc325","unresolved":false,"context_lines":[{"line_number":296,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":297,"context_line":"        routers \u003d []"},{"line_number":298,"context_line":"        routers.append(ri.router)"},{"line_number":299,"context_line":"        self.api.router_info \u003d {}"},{"line_number":300,"context_line":"        self.api.router_info[ri.router_id] \u003d ri"},{"line_number":301,"context_line":"        router_info_list \u003d self.api._get_router_info_list_for_tenant("},{"line_number":302,"context_line":"            routers,"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2B2EE%3D","line":299,"updated":"2013-11-19 13:01:04.000000000","message":"Nit: These two lines could be combined.\n\nself.api.router_info \u003d {ri.router_id: ri}","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"6d4f3cf442dee0faa698dc80c2ea3b83841e9f71","unresolved":false,"context_lines":[{"line_number":296,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":297,"context_line":"        routers \u003d []"},{"line_number":298,"context_line":"        routers.append(ri.router)"},{"line_number":299,"context_line":"        self.api.router_info \u003d {}"},{"line_number":300,"context_line":"        self.api.router_info[ri.router_id] \u003d ri"},{"line_number":301,"context_line":"        router_info_list \u003d self.api._get_router_info_list_for_tenant("},{"line_number":302,"context_line":"            routers,"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F%2BycE%3D","line":299,"in_reply_to":"AAAATn%2F%2B2EE%3D","updated":"2013-11-20 07:11:19.000000000","message":"Done","commit_id":"2376eb50f49b15babf1e78cf82c8bbfebb6a6eab"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"d93b5870c5e7d3960400eafce87f539a585c5bc9","unresolved":false,"context_lines":[{"line_number":270,"context_line":"                fake_firewall_list[0][\u0027id\u0027])"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    def _prepare_router_data(self):"},{"line_number":273,"context_line":"        router \u003d {"},{"line_number":274,"context_line":"            \u0027id\u0027: _uuid(),"},{"line_number":275,"context_line":"            \u0027tenant_id\u0027: _uuid()}"},{"line_number":276,"context_line":"        return l3_agent.RouterInfo(router[\u0027id\u0027], self.conf.root_helper,"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F%2ByKA%3D","line":273,"updated":"2013-11-20 09:28:42.000000000","message":"(Suggestion only) As per HACKING, only use multiline dict format if unable to fit the definition on one line (which isn\u0027t the case here).","commit_id":"64dddeef9fd107cae227f45aed8950652c3c588d"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"7e33e849961e3ea46c4320376a2b8ed0714bdbe5","unresolved":false,"context_lines":[{"line_number":270,"context_line":"                fake_firewall_list[0][\u0027id\u0027])"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    def _prepare_router_data(self):"},{"line_number":273,"context_line":"        router \u003d {"},{"line_number":274,"context_line":"            \u0027id\u0027: _uuid(),"},{"line_number":275,"context_line":"            \u0027tenant_id\u0027: _uuid()}"},{"line_number":276,"context_line":"        return l3_agent.RouterInfo(router[\u0027id\u0027], self.conf.root_helper,"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F%2Bxrk%3D","line":273,"in_reply_to":"AAAATn%2F%2ByKA%3D","updated":"2013-11-20 10:51:48.000000000","message":"Done","commit_id":"64dddeef9fd107cae227f45aed8950652c3c588d"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"d93b5870c5e7d3960400eafce87f539a585c5bc9","unresolved":false,"context_lines":[{"line_number":276,"context_line":"        return l3_agent.RouterInfo(router[\u0027id\u0027], self.conf.root_helper,"},{"line_number":277,"context_line":"                                   self.conf.use_namespaces, router\u003drouter)"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"    def test_get_router_info_list(self):"},{"line_number":280,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, True)"},{"line_number":281,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":282,"context_line":"        routers \u003d [ri.router]"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F%2ByJw%3D","line":279,"updated":"2013-11-20 09:28:42.000000000","message":"(Suggestion only) Consider following the convention of test_[function under test]_[what is being tested], e.g. test_get_router_info_list_for_tenant_for_namespaces_enabled","commit_id":"64dddeef9fd107cae227f45aed8950652c3c588d"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"7e33e849961e3ea46c4320376a2b8ed0714bdbe5","unresolved":false,"context_lines":[{"line_number":276,"context_line":"        return l3_agent.RouterInfo(router[\u0027id\u0027], self.conf.root_helper,"},{"line_number":277,"context_line":"                                   self.conf.use_namespaces, router\u003drouter)"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"    def test_get_router_info_list(self):"},{"line_number":280,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, True)"},{"line_number":281,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":282,"context_line":"        routers \u003d [ri.router]"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F%2Bxrg%3D","line":279,"in_reply_to":"AAAATn%2F%2ByJw%3D","updated":"2013-11-20 10:51:48.000000000","message":"Done","commit_id":"64dddeef9fd107cae227f45aed8950652c3c588d"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"d93b5870c5e7d3960400eafce87f539a585c5bc9","unresolved":false,"context_lines":[{"line_number":291,"context_line":"            mock_get_namespaces.assert_called_once_with("},{"line_number":292,"context_line":"                self.conf.root_helper)"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, False)"},{"line_number":295,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":296,"context_line":"        routers \u003d [ri.router]"},{"line_number":297,"context_line":"        self.api.router_info \u003d {ri.router_id: ri}"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F%2ByGA%3D","line":294,"updated":"2013-11-20 09:28:42.000000000","message":"Please separate out this second block into its own test (e.g. test_get_router_info_list_for_tenant_for_namespaces_disabled)","commit_id":"64dddeef9fd107cae227f45aed8950652c3c588d"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"7e33e849961e3ea46c4320376a2b8ed0714bdbe5","unresolved":false,"context_lines":[{"line_number":291,"context_line":"            mock_get_namespaces.assert_called_once_with("},{"line_number":292,"context_line":"                self.conf.root_helper)"},{"line_number":293,"context_line":""},{"line_number":294,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, False)"},{"line_number":295,"context_line":"        ri \u003d self._prepare_router_data()"},{"line_number":296,"context_line":"        routers \u003d [ri.router]"},{"line_number":297,"context_line":"        self.api.router_info \u003d {ri.router_id: ri}"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F%2Bxrc%3D","line":294,"in_reply_to":"AAAATn%2F%2ByGA%3D","updated":"2013-11-20 10:51:48.000000000","message":"Done","commit_id":"64dddeef9fd107cae227f45aed8950652c3c588d"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"918218348603ed28877064ec070c13269ed01e37","unresolved":false,"context_lines":[{"line_number":299,"context_line":"            ri.router[\u0027tenant_id\u0027])"},{"line_number":300,"context_line":"        self.assertEqual([ri], router_info_list)"},{"line_number":301,"context_line":""},{"line_number":302,"context_line":"    def test_get_router_info_list_for_tenant_for_namespaces_combinations(self):"},{"line_number":303,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, True)"},{"line_number":304,"context_line":"        ri \u003d self._prepare_router_data(False)"},{"line_number":305,"context_line":"        routers \u003d [ri.router]"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATn%2F%2Bxcg%3D","line":302,"updated":"2013-11-20 12:00:09.000000000","message":"Consider breaking into two test cases.\ntest_get_router_info_list_tenant_with_namespace_router_without()\ntest_get_router_info_list_tenant_without_namespace_router_with()\nor something like that?","commit_id":"d4be9ca9926f31108da82342a7b567f24305ec68"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"5ee7c508191c1213f809d11be8cab46da1e81aef","unresolved":false,"context_lines":[{"line_number":299,"context_line":"            ri.router[\u0027tenant_id\u0027])"},{"line_number":300,"context_line":"        self.assertEqual([ri], router_info_list)"},{"line_number":301,"context_line":""},{"line_number":302,"context_line":"    def test_get_router_info_list_for_tenant_for_namespaces_combinations(self):"},{"line_number":303,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, True)"},{"line_number":304,"context_line":"        ri \u003d self._prepare_router_data(False)"},{"line_number":305,"context_line":"        routers \u003d [ri.router]"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATn%2F%2BucE%3D","line":302,"in_reply_to":"AAAATn%2F%2Bxcg%3D","updated":"2013-11-21 02:16:50.000000000","message":"thank paul, done","commit_id":"d4be9ca9926f31108da82342a7b567f24305ec68"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"918218348603ed28877064ec070c13269ed01e37","unresolved":false,"context_lines":[{"line_number":301,"context_line":""},{"line_number":302,"context_line":"    def test_get_router_info_list_for_tenant_for_namespaces_combinations(self):"},{"line_number":303,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, True)"},{"line_number":304,"context_line":"        ri \u003d self._prepare_router_data(False)"},{"line_number":305,"context_line":"        routers \u003d [ri.router]"},{"line_number":306,"context_line":"        self.api.router_info \u003d {ri.router_id: ri}"},{"line_number":307,"context_line":"        with mock.patch.object(ip_lib.IPWrapper,"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATn%2F%2BxdU%3D","line":304,"updated":"2013-11-20 12:00:09.000000000","message":"Suggestion. Could specify this as:\nself._prepare_router_data(use_namespaces\u003dFalse)\n\nto aid in readability of the test case here and on L318.","commit_id":"d4be9ca9926f31108da82342a7b567f24305ec68"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"5ee7c508191c1213f809d11be8cab46da1e81aef","unresolved":false,"context_lines":[{"line_number":301,"context_line":""},{"line_number":302,"context_line":"    def test_get_router_info_list_for_tenant_for_namespaces_combinations(self):"},{"line_number":303,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, True)"},{"line_number":304,"context_line":"        ri \u003d self._prepare_router_data(False)"},{"line_number":305,"context_line":"        routers \u003d [ri.router]"},{"line_number":306,"context_line":"        self.api.router_info \u003d {ri.router_id: ri}"},{"line_number":307,"context_line":"        with mock.patch.object(ip_lib.IPWrapper,"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATn%2F%2BucA%3D","line":304,"in_reply_to":"AAAATn%2F%2BxdU%3D","updated":"2013-11-21 02:16:50.000000000","message":"thank paul, done","commit_id":"d4be9ca9926f31108da82342a7b567f24305ec68"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"978ddf390ad38b4cd9a929f0a2437e887abcee74","unresolved":false,"context_lines":[{"line_number":274,"context_line":"        return l3_agent.RouterInfo(router[\u0027id\u0027], self.conf.root_helper,"},{"line_number":275,"context_line":"                                   use_namespaces, router\u003drouter)"},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"    def test_get_router_info_list_for_tenant_for_namespaces_enabled(self):"},{"line_number":278,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, True)"},{"line_number":279,"context_line":"        ri \u003d self._prepare_router_data("},{"line_number":280,"context_line":"            use_namespaces\u003dself.conf.use_namespaces)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BqiM%3D","line":277,"updated":"2013-11-21 20:36:55.000000000","message":"The 2 pairs of tests are largely duplicative, can you see a way to factor out the duplication to make the tests easier to maintain?","commit_id":"4e0b86d1b0143667c039e0c897bd54a145adc211"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"51c46d11148cdb08e6a59f2a2b87af9105914a3f","unresolved":false,"context_lines":[{"line_number":274,"context_line":"        return l3_agent.RouterInfo(router[\u0027id\u0027], self.conf.root_helper,"},{"line_number":275,"context_line":"                                   use_namespaces, router\u003drouter)"},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"    def test_get_router_info_list_for_tenant_for_namespaces_enabled(self):"},{"line_number":278,"context_line":"        self.conf.set_override(\u0027use_namespaces\u0027, True)"},{"line_number":279,"context_line":"        ri \u003d self._prepare_router_data("},{"line_number":280,"context_line":"            use_namespaces\u003dself.conf.use_namespaces)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BnNc%3D","line":277,"in_reply_to":"AAAATn%2F%2BqiM%3D","updated":"2013-11-22 13:55:40.000000000","message":"Done, thanks","commit_id":"4e0b86d1b0143667c039e0c897bd54a145adc211"},{"author":{"_account_id":6722,"name":"ChangBo Guo(gcb)","email":"guochbo@cn.ibm.com","username":"guochbo"},"change_message_id":"5a28af5db4dc9e182e5cce6835ba397b98fb91f9","unresolved":false,"context_lines":[{"line_number":32,"context_line":"from neutron.services.firewall.agents.l3reference import firewall_l3_agent"},{"line_number":33,"context_line":"from neutron.tests import base"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"_uuid \u003d uuidutils.generate_uuid"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"class FWaasHelper(object):"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2BhpQ%3D","line":35,"updated":"2013-11-25 04:42:33.000000000","message":"Please don\u0027t use  uuidutils, because it will be deprecated from Olso.\nSee https://bugs.launchpad.net/nova/+bug/1253497","commit_id":"83f766f0e8f482d30e7b24df331a1b37e2fe464c"},{"author":{"_account_id":2711,"name":"Zhang Hua","display_name":"Zhang Hua","email":"joshua.zhang@canonical.com","username":"zhhuabj"},"change_message_id":"e1f6ae4a18ad6ab5509d48e7a055ee07a23d625e","unresolved":false,"context_lines":[{"line_number":32,"context_line":"from neutron.services.firewall.agents.l3reference import firewall_l3_agent"},{"line_number":33,"context_line":"from neutron.tests import base"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"_uuid \u003d uuidutils.generate_uuid"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"class FWaasHelper(object):"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2BhFg%3D","line":35,"in_reply_to":"AAAATn%2F%2BhpQ%3D","updated":"2013-11-25 08:51:18.000000000","message":"Done","commit_id":"83f766f0e8f482d30e7b24df331a1b37e2fe464c"}]}