)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"91b3200a863f8d64a86d781119b9f54d0d3a1ba0","unresolved":false,"context_lines":[{"line_number":27,"context_line":"This approach means that we can remove the initial"},{"line_number":28,"context_line":"optimization for a single server case."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"Fixes Bug: 1228384"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"Change-Id: I6f72533056e4f336f7578cc883fd1a125c2048a9"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"AAAATX%2F%2F%2BVc%3D","line":30,"updated":"2013-10-23 13:29:41.000000000","message":"Closes-Bug:","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"633e0b6b7800183e99e554f9a85621b81856325c","unresolved":false,"context_lines":[{"line_number":27,"context_line":"This approach means that we can remove the initial"},{"line_number":28,"context_line":"optimization for a single server case."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"Fixes Bug: 1228384"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"Change-Id: I6f72533056e4f336f7578cc883fd1a125c2048a9"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"AAAATn%2F%2FmCU%3D","line":30,"in_reply_to":"AAAATX%2F%2F%2BVc%3D","updated":"2013-10-31 17:45:07.000000000","message":"Done","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"}],"nova/api/openstack/compute/contrib/security_groups.py":[{"author":{"_account_id":7040,"name":"GuoHui LIu","email":"liuguohui@gmail.com","username":"guohliu"},"change_message_id":"0c869390a06abb3ce89fbc671c8e5b073518b6e8","unresolved":false,"context_lines":[{"line_number":507,"context_line":"        return (\u0027tenant_id\u0027)"},{"line_number":508,"context_line":""},{"line_number":509,"context_line":"    def _remove_invalid_search_options(self, search_options,"},{"line_number":510,"context_line":"                                       allowed_search_options):"},{"line_number":511,"context_line":"        \"\"\"Remove search options that are not valid for neutron.\"\"\""},{"line_number":512,"context_line":"        # Strip out all unknown options"},{"line_number":513,"context_line":"        unknown_options \u003d [opt for opt in search_options"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FofI%3D","line":510,"updated":"2013-09-23 17:22:58.000000000","message":"Is this we really want? that change the search options provided by user silently though those options doesn\u0027t take any effect rather than tells user those are invalid search options.","commit_id":"7e53acb0d46a8eaf1aae1d4818670e025e65dd50"},{"author":{"_account_id":7040,"name":"GuoHui LIu","email":"liuguohui@gmail.com","username":"guohliu"},"change_message_id":"0cdb6d5998eb79d28e59a2b83c236f88ab96c7a9","unresolved":false,"context_lines":[{"line_number":507,"context_line":"        return (\u0027tenant_id\u0027)"},{"line_number":508,"context_line":""},{"line_number":509,"context_line":"    def _remove_invalid_search_options(self, search_options,"},{"line_number":510,"context_line":"                                       allowed_search_options):"},{"line_number":511,"context_line":"        \"\"\"Remove search options that are not valid for neutron.\"\"\""},{"line_number":512,"context_line":"        # Strip out all unknown options"},{"line_number":513,"context_line":"        unknown_options \u003d [opt for opt in search_options"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FlZo%3D","line":510,"in_reply_to":"AAAAS3%2F%2FlaE%3D","updated":"2013-09-24 11:12:48.000000000","message":"you are right about it, I wasn\u0027t checking the codes carefully, thanks.","commit_id":"7e53acb0d46a8eaf1aae1d4818670e025e65dd50"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"ae09d005fa5b4afa1729f060c28539333c6d2c44","unresolved":false,"context_lines":[{"line_number":507,"context_line":"        return (\u0027tenant_id\u0027)"},{"line_number":508,"context_line":""},{"line_number":509,"context_line":"    def _remove_invalid_search_options(self, search_options,"},{"line_number":510,"context_line":"                                       allowed_search_options):"},{"line_number":511,"context_line":"        \"\"\"Remove search options that are not valid for neutron.\"\"\""},{"line_number":512,"context_line":"        # Strip out all unknown options"},{"line_number":513,"context_line":"        unknown_options \u003d [opt for opt in search_options"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FlaE%3D","line":510,"in_reply_to":"AAAAS3%2F%2FofI%3D","updated":"2013-09-24 11:07:52.000000000","message":"The search options the user provided are for Nova, and the Nova API will have already processed them before we get here resulting in the server list that we\u0027re now trying to get security group info for.  What we\u0027re doing here is trying to also make use of any that are relevant to Neutron (to also optimize the Neutron Queries) , and remove any that  Neutron doesn\u0027t support","commit_id":"7e53acb0d46a8eaf1aae1d4818670e025e65dd50"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"4409c8d006e09b7ae6b1cd5c8d9ea64b4b61b54a","unresolved":false,"context_lines":[{"line_number":538,"context_line":"                # Get any search options from the request and use these to"},{"line_number":539,"context_line":"                # limit the scope of ports we ask Neutron for - otherwist a"},{"line_number":540,"context_line":"                # user with admin role in Neutron will end up getting all ports"},{"line_number":541,"context_line":"                # and Sec Groups details. "},{"line_number":542,"context_line":"                search_opts \u003d {}"},{"line_number":543,"context_line":"                search_opts.update(req.GET)"},{"line_number":544,"context_line":"                valid_opts \u003d self._get_valid_neutron_search_opts()"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FovU%3D","line":541,"updated":"2013-09-23 16:04:14.000000000","message":"whitespace","commit_id":"7e53acb0d46a8eaf1aae1d4818670e025e65dd50"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"4409c8d006e09b7ae6b1cd5c8d9ea64b4b61b54a","unresolved":false,"context_lines":[{"line_number":586,"context_line":"                        if groups:"},{"line_number":587,"context_line":"                            server[key] \u003d groups"},{"line_number":588,"context_line":"                else:"},{"line_number":589,"context_line":"                    # Seems to be all servers in the scope of the tenant, so "},{"line_number":590,"context_line":"                    # allow Neutron to get all ports in this scope"},{"line_number":591,"context_line":"                    sg_instance_bindings \u003d ("},{"line_number":592,"context_line":"                        self.security_group_api"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FovE%3D","line":589,"updated":"2013-09-23 16:04:14.000000000","message":"ditto","commit_id":"7e53acb0d46a8eaf1aae1d4818670e025e65dd50"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"53fa51154e67e3a5bea134a6dd33bbb7942486f3","unresolved":false,"context_lines":[{"line_number":513,"context_line":"        unknown_options \u003d [opt for opt in search_options"},{"line_number":514,"context_line":"                            if opt not in allowed_search_options]"},{"line_number":515,"context_line":"        for opt in unknown_options:"},{"line_number":516,"context_line":"            search_options.pop(opt, None)"},{"line_number":517,"context_line":""},{"line_number":518,"context_line":"    def _extend_servers(self, req, servers):"},{"line_number":519,"context_line":"        # TODO(arosen) this function should be refactored to reduce duplicate"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FdCA%3D","line":516,"updated":"2013-09-26 13:31:29.000000000","message":"See comments on the other _remove_invalid_search_options method.  Actually, why not use a function rather than a method so you can use the same one for both?  You\u0027re not actually using self.","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"a17b30c5d432b0899d984cfb77c4b0a11bd36389","unresolved":false,"context_lines":[{"line_number":528,"context_line":"                for server in servers:"},{"line_number":529,"context_line":"                    groups \u003d sg_instance_bindings.get(server[\u0027id\u0027])"},{"line_number":530,"context_line":"                    if groups:"},{"line_number":531,"context_line":"                        server[key] \u003d groups"},{"line_number":532,"context_line":""},{"line_number":533,"context_line":"            # In this section of code len(servers) \u003d\u003d 1 as you can only POST"},{"line_number":534,"context_line":"            # one server in an API request."}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATH%2F%2Fg7o%3D","line":531,"updated":"2013-10-15 07:40:43.000000000","message":"Why v3 didn\u0027t doing same change as v2?","commit_id":"5b0cd311334dbb37f66785f20efb126748ba383f"}],"nova/api/openstack/compute/plugins/v3/security_groups.py":[{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"53fa51154e67e3a5bea134a6dd33bbb7942486f3","unresolved":false,"context_lines":[{"line_number":59,"context_line":"        unknown_options \u003d [opt for opt in search_options"},{"line_number":60,"context_line":"                            if opt not in allowed_search_options]"},{"line_number":61,"context_line":"        for opt in unknown_options:"},{"line_number":62,"context_line":"            search_options.pop(opt, None)"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    def _extend_servers(self, req, servers):"},{"line_number":65,"context_line":"        # TODO(arosen) this function should be refactored to reduce duplicate"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FdCk%3D","line":62,"updated":"2013-09-26 13:31:29.000000000","message":"You don\u0027t really need two passes here if you return the valid search_options rather than mutating the original search_options.  Why not something like:\n\nreturn [opt for opt in search_options if opt in allowed_search_options]","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"dbc0a9c8f5d224f1e7fb9a982a5d8cd9c4ffbef6","unresolved":false,"context_lines":[{"line_number":59,"context_line":"        unknown_options \u003d [opt for opt in search_options"},{"line_number":60,"context_line":"                            if opt not in allowed_search_options]"},{"line_number":61,"context_line":"        for opt in unknown_options:"},{"line_number":62,"context_line":"            search_options.pop(opt, None)"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    def _extend_servers(self, req, servers):"},{"line_number":65,"context_line":"        # TODO(arosen) this function should be refactored to reduce duplicate"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FaTg%3D","line":62,"in_reply_to":"AAAAS3%2F%2FdCk%3D","updated":"2013-09-27 08:53:28.000000000","message":"Except that I need to return a dict not a list, and dict comprehensions are a 2.7 feature only (and we have to keep 2.6 compatibility right ?) \n\nThe code here is pretty much copied from equivalent code in the main compute/servers.py","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"}],"nova/network/security_group/neutron_driver.py":[{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"53fa51154e67e3a5bea134a6dd33bbb7942486f3","unresolved":false,"context_lines":[{"line_number":293,"context_line":"        if id in lookup_list:"},{"line_number":294,"context_line":"            return lookup_list[id]"},{"line_number":295,"context_line":"        else:"},{"line_number":296,"context_line":"            return"},{"line_number":297,"context_line":""},{"line_number":298,"context_line":"    def get_instances_security_groups_bindings(self, context, search_opts\u003d{}):"},{"line_number":299,"context_line":"        \"\"\"Returns a dict(instance_id, [security_groups]) to allow obtaining"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FdE4%3D","line":296,"updated":"2013-09-26 13:31:29.000000000","message":"These 4 lines could be  \"return lookup_list.get(id)\"","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"dbc0a9c8f5d224f1e7fb9a982a5d8cd9c4ffbef6","unresolved":false,"context_lines":[{"line_number":293,"context_line":"        if id in lookup_list:"},{"line_number":294,"context_line":"            return lookup_list[id]"},{"line_number":295,"context_line":"        else:"},{"line_number":296,"context_line":"            return"},{"line_number":297,"context_line":""},{"line_number":298,"context_line":"    def get_instances_security_groups_bindings(self, context, search_opts\u003d{}):"},{"line_number":299,"context_line":"        \"\"\"Returns a dict(instance_id, [security_groups]) to allow obtaining"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FaSE%3D","line":296,"in_reply_to":"AAAAS3%2F%2FdE4%3D","updated":"2013-09-27 08:53:28.000000000","message":"\"lookup_list\" is a list not a dict, so it doesn\u0027t have a .get opertaion","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"32040f4192f2a1d2973c20312d957453d52cd674","unresolved":false,"context_lines":[{"line_number":305,"context_line":"        instances_security_group_bindings \u003d {}"},{"line_number":306,"context_line":"        if search_opts:"},{"line_number":307,"context_line":"            security_groups \u003d neutron.list_security_groups(**search_opts).\\"},{"line_number":308,"context_line":"                                     get(\u0027security_groups\u0027)"},{"line_number":309,"context_line":"            for security_group in security_groups:"},{"line_number":310,"context_line":"                security_group_lookup[security_group[\u0027id\u0027]] \u003d security_group"},{"line_number":311,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FWVM%3D","line":308,"updated":"2013-09-29 02:45:30.000000000","message":"We can list securty_groups like this:\n\nids \u003d []\n\nfor port in ports:\n\n    ids.append(port[\u0027id\u0027])\n\n\nsearch_opts[\u0027id\u0027] \u003d ids\n\nsecurity_groups \u003d neutron.list_security_groups(**search_opts).get(security_groups)\n\n\nThat looks like more effective.","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"53fa51154e67e3a5bea134a6dd33bbb7942486f3","unresolved":false,"context_lines":[{"line_number":352,"context_line":"                        # Since the name is optional for"},{"line_number":353,"context_line":"                        # neutron security groups"},{"line_number":354,"context_line":"                        if not name:"},{"line_number":355,"context_line":"                            name \u003d security_group_id"},{"line_number":356,"context_line":"                        ret.append({\u0027name\u0027: name})"},{"line_number":357,"context_line":"        return ret"},{"line_number":358,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FdEc%3D","line":355,"updated":"2013-09-26 13:31:29.000000000","message":"If you\u0027re only trying to replace None rather than \u0027\u0027, those 3 lines could be\nname \u003d security_group.get(\u0027name\u0027, security_group_id)","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"4d0aa9babac4c885eabea11ed472e36553aa0741","unresolved":false,"context_lines":[{"line_number":352,"context_line":"                        # Since the name is optional for"},{"line_number":353,"context_line":"                        # neutron security groups"},{"line_number":354,"context_line":"                        if not name:"},{"line_number":355,"context_line":"                            name \u003d security_group_id"},{"line_number":356,"context_line":"                        ret.append({\u0027name\u0027: name})"},{"line_number":357,"context_line":"        return ret"},{"line_number":358,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FaFI%3D","line":355,"in_reply_to":"AAAAS3%2F%2FdEc%3D","updated":"2013-09-27 09:55:48.000000000","message":"Actually I think what Neutron allows is an empty name, so the get may return an empty string, not the default value that you\u0027re suggesting.\n\nSince this code isn\u0027t part of what  my changing I suggest leaving it as is rather risk breaking something","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"dbc0a9c8f5d224f1e7fb9a982a5d8cd9c4ffbef6","unresolved":false,"context_lines":[{"line_number":352,"context_line":"                        # Since the name is optional for"},{"line_number":353,"context_line":"                        # neutron security groups"},{"line_number":354,"context_line":"                        if not name:"},{"line_number":355,"context_line":"                            name \u003d security_group_id"},{"line_number":356,"context_line":"                        ret.append({\u0027name\u0027: name})"},{"line_number":357,"context_line":"        return ret"},{"line_number":358,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FaQY%3D","line":355,"in_reply_to":"AAAAS3%2F%2FdEc%3D","updated":"2013-09-27 08:53:28.000000000","message":"Done","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"29d341043727318fb6dbbb364369ab6f0749b21f","unresolved":false,"context_lines":[{"line_number":314,"context_line":"            for port_security_group in port.get(\u0027security_groups\u0027, []):"},{"line_number":315,"context_line":"                sg \u003d self._get_and_cache_security_group("},{"line_number":316,"context_line":"                                             neutron, port_security_group,"},{"line_number":317,"context_line":"                                             security_group_lookup)"},{"line_number":318,"context_line":"                if sg:"},{"line_number":319,"context_line":"                    # name is optional in neutron so if not specified return id"},{"line_number":320,"context_line":"                    if sg.get(\u0027name\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAS3%2F%2FJs0%3D","line":317,"updated":"2013-10-03 13:10:31.000000000","message":"how about:\n\nsg_ids \u003d set()\n\nfor port in ports:\n\n   sg_ids.update(port.get(\u0027security_groups\u0027, []))\n\nsg_search_opts \u003d {\u0027id\u0027: list(sg_ids)}\n\nsecurity_groups \u003d neutron.list_security_groups(**sg_search_opts)\n\n....\n\nwe get the needed security_groups in one neutron api call.","commit_id":"1e92413fd9fe78f53a22d83ec5f131533f8fe9e7"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"a3e429e103694e7362334dd52de9f10d02428d34","unresolved":false,"context_lines":[{"line_number":296,"context_line":"            sg_ids.update(port.get(\u0027security_groups\u0027, []))"},{"line_number":297,"context_line":"        sg_search_opts \u003d {\u0027id\u0027: list(sg_ids)}"},{"line_number":298,"context_line":"        security_groups \u003d neutron.list_security_groups(**sg_search_opts).\\"},{"line_number":299,"context_line":"                                 get(\u0027security_groups\u0027)"},{"line_number":300,"context_line":""},{"line_number":301,"context_line":"        instances_security_group_bindings \u003d {}"},{"line_number":302,"context_line":"        for port in ports:"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATH%2F%2Fzn4%3D","line":299,"updated":"2013-10-09 15:24:55.000000000","message":"if the sg_search_opts was {\u0027id\u0027: []}, it will try to get all the security_groups. we should return at here directly. Apology for i miss this previously","commit_id":"d82451ae4ca5bf50559350a613a88d856e095163"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"a3e429e103694e7362334dd52de9f10d02428d34","unresolved":false,"context_lines":[{"line_number":303,"context_line":"            for port_sg_id in port.get(\u0027security_groups\u0027, []):"},{"line_number":304,"context_line":"                port_sg \u003d [sg for sg in security_groups"},{"line_number":305,"context_line":"                                  if sg.get(\u0027id\u0027) \u003d\u003d port_sg_id]"},{"line_number":306,"context_line":"                # Note:  have to check we found port_sg as its possible"},{"line_number":307,"context_line":"                # the port has an SG that this user doesn\u0027t have access to"},{"line_number":308,"context_line":"                if port_sg:"},{"line_number":309,"context_line":"                    # name is optional in neutron so if not specified return id"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATH%2F%2Fzlo%3D","line":306,"updated":"2013-10-09 15:24:55.000000000","message":"nit, double blank","commit_id":"d82451ae4ca5bf50559350a613a88d856e095163"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"a3e429e103694e7362334dd52de9f10d02428d34","unresolved":false,"context_lines":[{"line_number":330,"context_line":"            sg_ids.update(port.get(\u0027security_groups\u0027, []))"},{"line_number":331,"context_line":"        sg_search_opts \u003d {\u0027id\u0027: list(sg_ids)}"},{"line_number":332,"context_line":"        security_groups \u003d neutron.list_security_groups(**sg_search_opts).\\"},{"line_number":333,"context_line":"                                 get(\u0027security_groups\u0027)"},{"line_number":334,"context_line":""},{"line_number":335,"context_line":"        ret \u003d []"},{"line_number":336,"context_line":"        for port in ports:"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATH%2F%2Fzn0%3D","line":333,"updated":"2013-10-09 15:24:55.000000000","message":"ditto, and a lot of code looks like same between \u0027get_instance_security_groups\u0027 and \u0027get_instances_security_groups_bindings\u0027. Did we have any chance merge them as common code?","commit_id":"d82451ae4ca5bf50559350a613a88d856e095163"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"a3e429e103694e7362334dd52de9f10d02428d34","unresolved":false,"context_lines":[{"line_number":338,"context_line":"                port_sg \u003d [sg for sg in security_groups"},{"line_number":339,"context_line":"                                  if sg.get(\u0027id\u0027) \u003d\u003d port_sg_id]"},{"line_number":340,"context_line":"                if port_sg:"},{"line_number":341,"context_line":"                    # Note:  have to check we found port_sg as its possible"},{"line_number":342,"context_line":"                    # the port has an SG that this user doesn\u0027t have access to"},{"line_number":343,"context_line":"                    if detailed:"},{"line_number":344,"context_line":"                        ret.append(self._convert_to_nova_security_group_format("}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATH%2F%2Fzlk%3D","line":341,"updated":"2013-10-09 15:24:55.000000000","message":"ditto","commit_id":"d82451ae4ca5bf50559350a613a88d856e095163"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"91b3200a863f8d64a86d781119b9f54d0d3a1ba0","unresolved":false,"context_lines":[{"line_number":288,"context_line":"        # NOTE: Neutron client has a max URL length of 8192, so we have"},{"line_number":289,"context_line":"        # to limit the number of IDs we include in any single search.  Really"},{"line_number":290,"context_line":"        # doesn\u0027t seem to be any point in making this a config value."},{"line_number":291,"context_line":"        max_search_ids \u003d 150"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"        neutron \u003d neutronv2.get_client(context)"},{"line_number":294,"context_line":"        ids \u003d []"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATX%2F%2F%2BTY%3D","line":291,"updated":"2013-10-23 13:29:41.000000000","message":"maybe it is worthwhile to calculate this at runtime - \nmax_ids \u003d (8192-header size)//(len(uuid))\nbut this may just be an overkill","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":2271,"name":"Michael Still","email":"mikal@stillhq.com","username":"mikalstill"},"change_message_id":"d32407c4089567e98be9dc0145dd0315c1f26bd5","unresolved":false,"context_lines":[{"line_number":288,"context_line":"        # NOTE: Neutron client has a max URL length of 8192, so we have"},{"line_number":289,"context_line":"        # to limit the number of IDs we include in any single search.  Really"},{"line_number":290,"context_line":"        # doesn\u0027t seem to be any point in making this a config value."},{"line_number":291,"context_line":"        max_search_ids \u003d 150"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"        neutron \u003d neutronv2.get_client(context)"},{"line_number":294,"context_line":"        ids \u003d []"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2F%2FpA%3D","line":291,"in_reply_to":"AAAATX%2F%2F%2BTY%3D","updated":"2013-10-24 05:36:53.000000000","message":"uuid length is unlikely to change, so I think hard coding this is ok, especially with the associated note.","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"91b3200a863f8d64a86d781119b9f54d0d3a1ba0","unresolved":false,"context_lines":[{"line_number":292,"context_line":""},{"line_number":293,"context_line":"        neutron \u003d neutronv2.get_client(context)"},{"line_number":294,"context_line":"        ids \u003d []"},{"line_number":295,"context_line":"        ports \u003d []"},{"line_number":296,"context_line":"        for server in servers:"},{"line_number":297,"context_line":"            ids.append(server[\u0027id\u0027])"},{"line_number":298,"context_line":"            if len(ids) \u003e\u003d max_search_ids:"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATX%2F%2F%2BS8%3D","line":295,"updated":"2013-10-23 13:29:41.000000000","message":"the code would be more readable and easier to test if lines 296-\u003e 304 were in a method. then you could also write a specific tests for the overflow of the servers","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"633e0b6b7800183e99e554f9a85621b81856325c","unresolved":false,"context_lines":[{"line_number":292,"context_line":""},{"line_number":293,"context_line":"        neutron \u003d neutronv2.get_client(context)"},{"line_number":294,"context_line":"        ids \u003d []"},{"line_number":295,"context_line":"        ports \u003d []"},{"line_number":296,"context_line":"        for server in servers:"},{"line_number":297,"context_line":"            ids.append(server[\u0027id\u0027])"},{"line_number":298,"context_line":"            if len(ids) \u003e\u003d max_search_ids:"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2FmHs%3D","line":295,"in_reply_to":"AAAATX%2F%2F%2BS8%3D","updated":"2013-10-31 17:45:07.000000000","message":"Done","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"91b3200a863f8d64a86d781119b9f54d0d3a1ba0","unresolved":false,"context_lines":[{"line_number":313,"context_line":"        # client URL length limit"},{"line_number":314,"context_line":"        security_groups \u003d []"},{"line_number":315,"context_line":"        sg_id_list \u003d []"},{"line_number":316,"context_line":"        for sg_id in sg_ids:"},{"line_number":317,"context_line":"            sg_id_list.append(sg_id)"},{"line_number":318,"context_line":"            if len(sg_id_list) \u003e\u003d max_search_ids:"},{"line_number":319,"context_line":"                sg_search_opts \u003d {\u0027id\u0027: sg_id_list}"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATX%2F%2F%2BS0%3D","line":316,"updated":"2013-10-23 13:29:41.000000000","message":"same comment here as before","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"633e0b6b7800183e99e554f9a85621b81856325c","unresolved":false,"context_lines":[{"line_number":313,"context_line":"        # client URL length limit"},{"line_number":314,"context_line":"        security_groups \u003d []"},{"line_number":315,"context_line":"        sg_id_list \u003d []"},{"line_number":316,"context_line":"        for sg_id in sg_ids:"},{"line_number":317,"context_line":"            sg_id_list.append(sg_id)"},{"line_number":318,"context_line":"            if len(sg_id_list) \u003e\u003d max_search_ids:"},{"line_number":319,"context_line":"                sg_search_opts \u003d {\u0027id\u0027: sg_id_list}"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2FmHo%3D","line":316,"in_reply_to":"AAAATX%2F%2F%2BS0%3D","updated":"2013-10-31 17:45:07.000000000","message":"Done","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"91b3200a863f8d64a86d781119b9f54d0d3a1ba0","unresolved":false,"context_lines":[{"line_number":343,"context_line":"                    else:"},{"line_number":344,"context_line":"                        # name is optional in neutron so if not specified"},{"line_number":345,"context_line":"                        # return id"},{"line_number":346,"context_line":"                        name \u003d port_sg[0].get(\u0027name\u0027)"},{"line_number":347,"context_line":"                        if not name:"},{"line_number":348,"context_line":"                            name \u003d port_sg[0].get(\u0027id\u0027)"},{"line_number":349,"context_line":"                        sg_entry \u003d {\u0027name\u0027: name}"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATX%2F%2F%2BSY%3D","line":346,"updated":"2013-10-23 13:29:41.000000000","message":"why not just use the ids here? names do not need to be unique in neutron","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"633e0b6b7800183e99e554f9a85621b81856325c","unresolved":false,"context_lines":[{"line_number":343,"context_line":"                    else:"},{"line_number":344,"context_line":"                        # name is optional in neutron so if not specified"},{"line_number":345,"context_line":"                        # return id"},{"line_number":346,"context_line":"                        name \u003d port_sg[0].get(\u0027name\u0027)"},{"line_number":347,"context_line":"                        if not name:"},{"line_number":348,"context_line":"                            name \u003d port_sg[0].get(\u0027id\u0027)"},{"line_number":349,"context_line":"                        sg_entry \u003d {\u0027name\u0027: name}"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2FmHM%3D","line":346,"in_reply_to":"AAAATX%2F%2F%2BSY%3D","updated":"2013-10-31 17:45:07.000000000","message":"\u0027cos the existing code returns the name if defined - changing this to always be the id would be a change in API","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":2271,"name":"Michael Still","email":"mikal@stillhq.com","username":"mikalstill"},"change_message_id":"d32407c4089567e98be9dc0145dd0315c1f26bd5","unresolved":false,"context_lines":[{"line_number":350,"context_line":"                        instances_security_group_bindings.setdefault("},{"line_number":351,"context_line":"                            port[\u0027device_id\u0027], []).append(sg_entry)"},{"line_number":352,"context_line":""},{"line_number":353,"context_line":"        return instances_security_group_bindings"},{"line_number":354,"context_line":""},{"line_number":355,"context_line":"    def get_instance_security_groups(self, context, instance_uuid,"},{"line_number":356,"context_line":"                                     detailed\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2F%2Fo4%3D","line":353,"updated":"2013-10-24 05:36:53.000000000","message":"I agree that breaking this method up a bit would be nice.","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"633e0b6b7800183e99e554f9a85621b81856325c","unresolved":false,"context_lines":[{"line_number":350,"context_line":"                        instances_security_group_bindings.setdefault("},{"line_number":351,"context_line":"                            port[\u0027device_id\u0027], []).append(sg_entry)"},{"line_number":352,"context_line":""},{"line_number":353,"context_line":"        return instances_security_group_bindings"},{"line_number":354,"context_line":""},{"line_number":355,"context_line":"    def get_instance_security_groups(self, context, instance_uuid,"},{"line_number":356,"context_line":"                                     detailed\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2FmHI%3D","line":353,"in_reply_to":"AAAATn%2F%2F%2Fo4%3D","updated":"2013-10-31 17:45:07.000000000","message":"Done","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"378f4e2881d30d902b902da3e5078f0f11432cb9","unresolved":false,"context_lines":[{"line_number":282,"context_line":"    def _max_search_ids(self):"},{"line_number":283,"context_line":"        # NOTE: Neutron client has a max URL length of 8192, so we have"},{"line_number":284,"context_line":"        # to limit the number of IDs we include in any single search.  Really"},{"line_number":285,"context_line":"        # doesn\u0027t seem to be any point in making this a config value."},{"line_number":286,"context_line":"        return 150"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    def _get_ports_from_server_list(self, servers, neutron):"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2FcSg%3D","line":285,"updated":"2013-11-06 05:26:22.000000000","message":"150 looks magical.  Maybe you should define it as 8192 / (max_id_length) so that it\u0027s obvious that it\u0027s a calculated value rather than one picked out of the air.","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"456ee1c6fcaadec7c4e3fa5043cd9bbf24d4fd84","unresolved":false,"context_lines":[{"line_number":282,"context_line":"    def _max_search_ids(self):"},{"line_number":283,"context_line":"        # NOTE: Neutron client has a max URL length of 8192, so we have"},{"line_number":284,"context_line":"        # to limit the number of IDs we include in any single search.  Really"},{"line_number":285,"context_line":"        # doesn\u0027t seem to be any point in making this a config value."},{"line_number":286,"context_line":"        return 150"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    def _get_ports_from_server_list(self, servers, neutron):"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2FaP8%3D","line":285,"in_reply_to":"AAAATn%2F%2Fa0c%3D","updated":"2013-11-07 00:26:25.000000000","message":"You do have a comment explaining where it comes from, so I guess it\u0027s okay.  Just feels kind of magical.","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"5eb36da2a2278c92551dff17601cd4191681e1a7","unresolved":false,"context_lines":[{"line_number":282,"context_line":"    def _max_search_ids(self):"},{"line_number":283,"context_line":"        # NOTE: Neutron client has a max URL length of 8192, so we have"},{"line_number":284,"context_line":"        # to limit the number of IDs we include in any single search.  Really"},{"line_number":285,"context_line":"        # doesn\u0027t seem to be any point in making this a config value."},{"line_number":286,"context_line":"        return 150"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    def _get_ports_from_server_list(self, servers, neutron):"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2Fa0c%3D","line":285,"in_reply_to":"AAAATn%2F%2FcSg%3D","updated":"2013-11-06 17:18:46.000000000","message":"I appreciate your comment, but we\u0027ve already had one review cycle on this that conclude that with the supporting comment this was ok as a constant","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"378f4e2881d30d902b902da3e5078f0f11432cb9","unresolved":false,"context_lines":[{"line_number":297,"context_line":"                ids \u003d []"},{"line_number":298,"context_line":"        if ids:"},{"line_number":299,"context_line":"            search_opts \u003d {\u0027device_id\u0027: ids}"},{"line_number":300,"context_line":"            ports.extend(neutron.list_ports(**search_opts).get(\u0027ports\u0027))"},{"line_number":301,"context_line":""},{"line_number":302,"context_line":"        return ports"},{"line_number":303,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2FcSc%3D","line":300,"updated":"2013-11-06 05:26:22.000000000","message":"This looks correct but a bit awkward because of the repeated code for the last chunk.  I think it would be better to have a generator function that chunks the ids, and then you could iterate over the generator to consistently build ports.","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"456ee1c6fcaadec7c4e3fa5043cd9bbf24d4fd84","unresolved":false,"context_lines":[{"line_number":297,"context_line":"                ids \u003d []"},{"line_number":298,"context_line":"        if ids:"},{"line_number":299,"context_line":"            search_opts \u003d {\u0027device_id\u0027: ids}"},{"line_number":300,"context_line":"            ports.extend(neutron.list_ports(**search_opts).get(\u0027ports\u0027))"},{"line_number":301,"context_line":""},{"line_number":302,"context_line":"        return ports"},{"line_number":303,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2FaO0%3D","line":300,"in_reply_to":"AAAATn%2F%2Fa0I%3D","updated":"2013-11-07 00:26:25.000000000","message":"something like (untested):\n\ndef _get_ports_from_server_list(self, servers, neutron):\n\n    def chunk_by_ids(servers, limit):\n        ids \u003d []\n        for server in servers:\n            ids.append(server[\u0027id\u0027])\n            if len(ids) \u003e\u003d limit:\n                yield ids\n                ids \u003d []\n        yield ids\n\n    ports \u003d []\n    for ids in chunk_by_ids(servers, self._max_search_ids()):\n        search_opts \u003d {\u0027device_id\u0027: ids}\n        ports.extend(neutron.list_ports(**search_opts).get(\u0027ports\u0027))\n    return ports\n\n\nOf course the extra function takes away any length benefit here, so if you prefer the original code I won\u0027t argue too much.","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"5eb36da2a2278c92551dff17601cd4191681e1a7","unresolved":false,"context_lines":[{"line_number":297,"context_line":"                ids \u003d []"},{"line_number":298,"context_line":"        if ids:"},{"line_number":299,"context_line":"            search_opts \u003d {\u0027device_id\u0027: ids}"},{"line_number":300,"context_line":"            ports.extend(neutron.list_ports(**search_opts).get(\u0027ports\u0027))"},{"line_number":301,"context_line":""},{"line_number":302,"context_line":"        return ports"},{"line_number":303,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2Fa0I%3D","line":300,"in_reply_to":"AAAATn%2F%2FcSc%3D","updated":"2013-11-06 17:18:46.000000000","message":"Can you help me out here, as I don\u0027t really understand what it is you\u0027re looking for. The code as proposed may not be the most elegant, but it seems pretty easy to follow.","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"378f4e2881d30d902b902da3e5078f0f11432cb9","unresolved":false,"context_lines":[{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        # Find the set of unique SecGroup IDs to search for"},{"line_number":307,"context_line":"        sg_ids \u003d set()"},{"line_number":308,"context_line":"        security_groups \u003d {}"},{"line_number":309,"context_line":"        for port in ports:"},{"line_number":310,"context_line":"            sg_ids.update(port.get(\u0027security_groups\u0027, []))"},{"line_number":311,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2FcSY%3D","line":308,"updated":"2013-11-06 05:26:22.000000000","message":"This is misleading because you redefine security_groups as a list a few lines down.","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"456ee1c6fcaadec7c4e3fa5043cd9bbf24d4fd84","unresolved":false,"context_lines":[{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        # Find the set of unique SecGroup IDs to search for"},{"line_number":307,"context_line":"        sg_ids \u003d set()"},{"line_number":308,"context_line":"        security_groups \u003d {}"},{"line_number":309,"context_line":"        for port in ports:"},{"line_number":310,"context_line":"            sg_ids.update(port.get(\u0027security_groups\u0027, []))"},{"line_number":311,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2FaPA%3D","line":308,"in_reply_to":"AAAATn%2F%2FcSY%3D","updated":"2013-11-07 00:26:25.000000000","message":"Just making sure you didn\u0027t miss this.","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"378f4e2881d30d902b902da3e5078f0f11432cb9","unresolved":false,"context_lines":[{"line_number":323,"context_line":"            sg_search_opts \u003d {\u0027id\u0027: sg_id_list}"},{"line_number":324,"context_line":"            security_groups.extend(neutron.list_security_groups("},{"line_number":325,"context_line":"                                      **sg_search_opts)."},{"line_number":326,"context_line":"                                      get(\u0027security_groups\u0027))"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        return security_groups"},{"line_number":329,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2FcSU%3D","line":326,"updated":"2013-11-06 05:26:22.000000000","message":"Again, this repetition is awkward.  I\u0027d rather see a chunking generator to abstract away the looping.","commit_id":"da703115c3cb6f18e65d8d536f5f3224a78a0d21"},{"author":{"_account_id":2271,"name":"Michael Still","email":"mikal@stillhq.com","username":"mikalstill"},"change_message_id":"d0eb32b175f423599ebae5c333485149fa64dc41","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        # NOTE: Neutron client has a max URL length of 8192, so we have"},{"line_number":284,"context_line":"        # to limit the number of IDs we include in any single search.  Really"},{"line_number":285,"context_line":"        # doesn\u0027t seem to be any point in making this a config value."},{"line_number":286,"context_line":"        return 150"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    def _get_ports_from_server_list(self, servers, neutron):"},{"line_number":289,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2FHhA%3D","line":286,"updated":"2013-11-14 08:17:27.000000000","message":"Why not make this a constant at the top of the file then? A method seems a bit strange.","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"a1e1a435d664332d9332a59715924e602a0655fa","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        # NOTE: Neutron client has a max URL length of 8192, so we have"},{"line_number":284,"context_line":"        # to limit the number of IDs we include in any single search.  Really"},{"line_number":285,"context_line":"        # doesn\u0027t seem to be any point in making this a config value."},{"line_number":286,"context_line":"        return 150"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    def _get_ports_from_server_list(self, servers, neutron):"},{"line_number":289,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2Bmns%3D","line":286,"in_reply_to":"AAAATn%2F%2BrfI%3D","updated":"2013-11-22 15:52:31.000000000","message":"Done","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"},{"author":{"_account_id":642,"name":"Matt Dietz","email":"matthew.dietz@gmail.com","username":"cerberus"},"change_message_id":"730c606c5cb78b1d43c665415eebb26e59febdf7","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        # NOTE: Neutron client has a max URL length of 8192, so we have"},{"line_number":284,"context_line":"        # to limit the number of IDs we include in any single search.  Really"},{"line_number":285,"context_line":"        # doesn\u0027t seem to be any point in making this a config value."},{"line_number":286,"context_line":"        return 150"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    def _get_ports_from_server_list(self, servers, neutron):"},{"line_number":289,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2BrfI%3D","line":286,"in_reply_to":"AAAATn%2F%2FHhA%3D","updated":"2013-11-21 16:45:22.000000000","message":"I\u0027m not a fan of this either...","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"19731177916c24fb28628f6a6596602786be7f91","unresolved":false,"context_lines":[{"line_number":321,"context_line":"        for port in ports:"},{"line_number":322,"context_line":"            sg_ids.update(port.get(\u0027security_groups\u0027, []))"},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"        security_groups \u003d []"},{"line_number":325,"context_line":"        for sg_id_list in _chunk_by_ids(sg_ids, self._max_search_ids()):"},{"line_number":326,"context_line":"            sg_search_opts \u003d {\u0027id\u0027: sg_id_list}"},{"line_number":327,"context_line":"            security_groups.extend(neutron.list_security_groups("}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2BzXo%3D","line":324,"updated":"2013-11-19 23:43:14.000000000","message":"It seems we want to index these with sg_id.  Why not use a dict here?  A list seems unnatural.  See below.","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"a1e1a435d664332d9332a59715924e602a0655fa","unresolved":false,"context_lines":[{"line_number":321,"context_line":"        for port in ports:"},{"line_number":322,"context_line":"            sg_ids.update(port.get(\u0027security_groups\u0027, []))"},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"        security_groups \u003d []"},{"line_number":325,"context_line":"        for sg_id_list in _chunk_by_ids(sg_ids, self._max_search_ids()):"},{"line_number":326,"context_line":"            sg_search_opts \u003d {\u0027id\u0027: sg_id_list}"},{"line_number":327,"context_line":"            security_groups.extend(neutron.list_security_groups("}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2BmpQ%3D","line":324,"in_reply_to":"AAAATn%2F%2BzXo%3D","updated":"2013-11-22 15:52:31.000000000","message":"Done","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"19731177916c24fb28628f6a6596602786be7f91","unresolved":false,"context_lines":[{"line_number":345,"context_line":"        instances_security_group_bindings \u003d {}"},{"line_number":346,"context_line":"        for port in ports:"},{"line_number":347,"context_line":"            for port_sg_id in port.get(\u0027security_groups\u0027, []):"},{"line_number":348,"context_line":"                port_sg \u003d [sg for sg in security_groups"},{"line_number":349,"context_line":"                                  if sg.get(\u0027id\u0027) \u003d\u003d port_sg_id]"},{"line_number":350,"context_line":"                # Note:  have to check we found port_sg as its possible"},{"line_number":351,"context_line":"                # the port has an SG that this user doesn\u0027t have access to"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2BzXg%3D","line":348,"updated":"2013-11-19 23:43:14.000000000","message":"Why a list here?  Accessing as port_sg[0] is awkward.  Related to comment above.","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"a1e1a435d664332d9332a59715924e602a0655fa","unresolved":false,"context_lines":[{"line_number":345,"context_line":"        instances_security_group_bindings \u003d {}"},{"line_number":346,"context_line":"        for port in ports:"},{"line_number":347,"context_line":"            for port_sg_id in port.get(\u0027security_groups\u0027, []):"},{"line_number":348,"context_line":"                port_sg \u003d [sg for sg in security_groups"},{"line_number":349,"context_line":"                                  if sg.get(\u0027id\u0027) \u003d\u003d port_sg_id]"},{"line_number":350,"context_line":"                # Note:  have to check we found port_sg as its possible"},{"line_number":351,"context_line":"                # the port has an SG that this user doesn\u0027t have access to"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2BmpM%3D","line":348,"in_reply_to":"AAAATn%2F%2BzXg%3D","updated":"2013-11-22 15:52:31.000000000","message":"Done","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"dd8f74d2e4abdb048b0caa6b3741721eba93771a","unresolved":false,"context_lines":[{"line_number":347,"context_line":"            for port_sg_id in port.get(\u0027security_groups\u0027, []):"},{"line_number":348,"context_line":"                port_sg \u003d [sg for sg in security_groups"},{"line_number":349,"context_line":"                                  if sg.get(\u0027id\u0027) \u003d\u003d port_sg_id]"},{"line_number":350,"context_line":"                # Note:  have to check we found port_sg as its possible"},{"line_number":351,"context_line":"                # the port has an SG that this user doesn\u0027t have access to"},{"line_number":352,"context_line":"                if port_sg:"},{"line_number":353,"context_line":"                    if detailed:"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2Bsd8%3D","line":350,"updated":"2013-11-21 12:58:48.000000000","message":"should this comment be above line 348?\n\nI do see there is a corner case here that i looks like we need to filter out security_groups on ports that are not owned by the tenant as the tenant won\u0027t be able to see what is in them (thus this would have failed if detailed\u003dTrue). Good Catch.  (I think that we should probably not allow this type of thing in neutron as this makes things super complex and it doesn\u0027t seem that an admin user shouldn\u0027t do this. It seems like if this happens the admin probably doesn\u0027t understand the affect of doing this : i.e the user won\u0027t know what rules are allowed in his security profile).","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"a1e1a435d664332d9332a59715924e602a0655fa","unresolved":false,"context_lines":[{"line_number":347,"context_line":"            for port_sg_id in port.get(\u0027security_groups\u0027, []):"},{"line_number":348,"context_line":"                port_sg \u003d [sg for sg in security_groups"},{"line_number":349,"context_line":"                                  if sg.get(\u0027id\u0027) \u003d\u003d port_sg_id]"},{"line_number":350,"context_line":"                # Note:  have to check we found port_sg as its possible"},{"line_number":351,"context_line":"                # the port has an SG that this user doesn\u0027t have access to"},{"line_number":352,"context_line":"                if port_sg:"},{"line_number":353,"context_line":"                    if detailed:"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2BmoU%3D","line":350,"in_reply_to":"AAAATn%2F%2Bsd8%3D","updated":"2013-11-22 15:52:31.000000000","message":"I was surprised by this as well - but it came up in an earlier review that apparently some folks do have a set up where they add an \"admin\" SecGroup to a port that a user has no visibility or control over. \n\nAdded an explicit test for this as well now:\ntest_instances_security_group_bindings_with_hidden_sg()\n\nLike you it seems just a cause of confusion / support calls to me","commit_id":"48af28f8666a648d28d7e4c52a6a4dce535c22ef"}],"nova/tests/api/openstack/compute/contrib/test_security_groups.py":[{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"53fa51154e67e3a5bea134a6dd33bbb7942486f3","unresolved":false,"context_lines":[{"line_number":1507,"context_line":"    return ([fake_compute_get()], \u0027\u0027)"},{"line_number":1508,"context_line":""},{"line_number":1509,"context_line":""},{"line_number":1510,"context_line":"def fake_get_instances_security_groups_bindings(inst, context, search_opts\u003d{}):"},{"line_number":1511,"context_line":"    return {UUID1: [{\u0027name\u0027: \u0027fake-0-0\u0027}, {\u0027name\u0027: \u0027fake-0-1\u0027}],"},{"line_number":1512,"context_line":"            UUID2: [{\u0027name\u0027: \u0027fake-1-0\u0027}, {\u0027name\u0027: \u0027fake-1-1\u0027}]}"},{"line_number":1513,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FdD4%3D","line":1510,"updated":"2013-09-26 13:31:29.000000000","message":"Using mutable default args is a bad idea.  Since you don\u0027t actually use search_opts in the function, why not search_opts\u003dNone ?","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"dbc0a9c8f5d224f1e7fb9a982a5d8cd9c4ffbef6","unresolved":false,"context_lines":[{"line_number":1507,"context_line":"    return ([fake_compute_get()], \u0027\u0027)"},{"line_number":1508,"context_line":""},{"line_number":1509,"context_line":""},{"line_number":1510,"context_line":"def fake_get_instances_security_groups_bindings(inst, context, search_opts\u003d{}):"},{"line_number":1511,"context_line":"    return {UUID1: [{\u0027name\u0027: \u0027fake-0-0\u0027}, {\u0027name\u0027: \u0027fake-0-1\u0027}],"},{"line_number":1512,"context_line":"            UUID2: [{\u0027name\u0027: \u0027fake-1-0\u0027}, {\u0027name\u0027: \u0027fake-1-1\u0027}]}"},{"line_number":1513,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FaRY%3D","line":1510,"in_reply_to":"AAAAS3%2F%2FdD4%3D","updated":"2013-09-27 08:53:28.000000000","message":"Done","commit_id":"685c2737ab6d931bc48351fc5755426409a7f8c0"}],"nova/tests/network/security_group/test_neutron_driver.py":[{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"91b3200a863f8d64a86d781119b9f54d0d3a1ba0","unresolved":false,"context_lines":[{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    def _test_instances_security_group_bindings_scale(self, num_servers):"},{"line_number":110,"context_line":"        max_query \u003d 150"},{"line_number":111,"context_line":"        sg1 \u003d {\u0027id\u0027: \u00271\u0027, \u0027name\u0027: \u0027wol\u0027}"},{"line_number":112,"context_line":"        sg2 \u003d {\u0027id\u0027: \u00272\u0027, \u0027name\u0027: \u0027eor\u0027}"},{"line_number":113,"context_line":"        security_groups_list \u003d {\u0027security_groups\u0027: [sg1, sg2]}"},{"line_number":114,"context_line":"        servers \u003d []"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATX%2F%2F%2BSA%3D","line":111,"updated":"2013-10-23 13:29:41.000000000","message":"please use real uuids here","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":2271,"name":"Michael Still","email":"mikal@stillhq.com","username":"mikalstill"},"change_message_id":"d32407c4089567e98be9dc0145dd0315c1f26bd5","unresolved":false,"context_lines":[{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    def _test_instances_security_group_bindings_scale(self, num_servers):"},{"line_number":110,"context_line":"        max_query \u003d 150"},{"line_number":111,"context_line":"        sg1 \u003d {\u0027id\u0027: \u00271\u0027, \u0027name\u0027: \u0027wol\u0027}"},{"line_number":112,"context_line":"        sg2 \u003d {\u0027id\u0027: \u00272\u0027, \u0027name\u0027: \u0027eor\u0027}"},{"line_number":113,"context_line":"        security_groups_list \u003d {\u0027security_groups\u0027: [sg1, sg2]}"},{"line_number":114,"context_line":"        servers \u003d []"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2F%2Fow%3D","line":111,"in_reply_to":"AAAATX%2F%2F%2BSA%3D","updated":"2013-10-24 05:36:53.000000000","message":"Agreed.","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"},{"author":{"_account_id":1501,"name":"Phil Day","email":"philip.day@hp.com","username":"philip-day"},"change_message_id":"633e0b6b7800183e99e554f9a85621b81856325c","unresolved":false,"context_lines":[{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    def _test_instances_security_group_bindings_scale(self, num_servers):"},{"line_number":110,"context_line":"        max_query \u003d 150"},{"line_number":111,"context_line":"        sg1 \u003d {\u0027id\u0027: \u00271\u0027, \u0027name\u0027: \u0027wol\u0027}"},{"line_number":112,"context_line":"        sg2 \u003d {\u0027id\u0027: \u00272\u0027, \u0027name\u0027: \u0027eor\u0027}"},{"line_number":113,"context_line":"        security_groups_list \u003d {\u0027security_groups\u0027: [sg1, sg2]}"},{"line_number":114,"context_line":"        servers \u003d []"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F%2FmCc%3D","line":111,"in_reply_to":"AAAATn%2F%2F%2Fow%3D","updated":"2013-10-31 17:45:07.000000000","message":"Done","commit_id":"e763244d97638e916e48e2f184c8c93bc4c2daf1"}]}