)]}'
{"rally/plugins/openstack/context/keystone/roles.py":[{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"400850f918353b4f928083d7ece0ee1f479845ef","unresolved":false,"context_lines":[{"line_number":48,"context_line":"        \"type\": \"object\","},{"line_number":49,"context_line":"        \"$schema\": consts.JSON_SCHEMA,"},{"line_number":50,"context_line":"        \"properties\": {"},{"line_number":51,"context_line":"            \"role-list\": {"},{"line_number":52,"context_line":"                \"type\": \"array\","},{"line_number":53,"context_line":"                \"items\": {"},{"line_number":54,"context_line":"                    \"type\": \"string\","}],"source_content_type":"text/x-python","patch_set":1,"id":"3afc51ec_10727fad","line":51,"updated":"2016-03-14 13:44:38.000000000","message":"Sorry I can\u0027t understand why we need one more nested level? Why not to leave schema the same? At least this change is not backward compatible.","commit_id":"685e716a7cad6e38121bc94b4d8cd701e949c516"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"fb428b47b2516948ab738237d9107d855ea659ac","unresolved":false,"context_lines":[{"line_number":48,"context_line":"        \"type\": \"object\","},{"line_number":49,"context_line":"        \"$schema\": consts.JSON_SCHEMA,"},{"line_number":50,"context_line":"        \"properties\": {"},{"line_number":51,"context_line":"            \"role-list\": {"},{"line_number":52,"context_line":"                \"type\": \"array\","},{"line_number":53,"context_line":"                \"items\": {"},{"line_number":54,"context_line":"                    \"type\": \"string\","}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_95ad348b","line":51,"in_reply_to":"3afc51ec_10727fad","updated":"2016-03-15 13:18:46.000000000","message":"+1 to Roman. I don\u0027t think making the number of threads configurable per-scenario is worth this backwards-incompatible change.","commit_id":"685e716a7cad6e38121bc94b4d8cd701e949c516"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"fb428b47b2516948ab738237d9107d855ea659ac","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        self.credential \u003d self.context[\"admin\"][\"credential\"]"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def check_role_exists(self, admin_credential, context_role):"},{"line_number":75,"context_line":"        \"\"\"Check if role exists."},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"        :param admin_credential: The admin credential."},{"line_number":78,"context_line":"        :param context_role: name of existing role."}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_b52f90f5","line":75,"updated":"2016-03-15 13:18:46.000000000","message":"This should probably be renamed to something like _get_role_object(); checking if the role exists is only part of what it does.","commit_id":"685e716a7cad6e38121bc94b4d8cd701e949c516"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"271733892a4f9ddc3a1e868449bc0f8645730dbc","unresolved":false,"context_lines":[{"line_number":133,"context_line":"        def consume(cache, args):"},{"line_number":134,"context_line":"            user_id, role_id, project_id \u003d args"},{"line_number":135,"context_line":"            if \"client\" not in cache:"},{"line_number":136,"context_line":"                clients \u003d osclients.Clients(self.credential)"},{"line_number":137,"context_line":"                cache[\"client\"] \u003d keystone.wrap(clients.keystone())"},{"line_number":138,"context_line":"            cache[\"client\"].remove_role(user_id, role_id, project_id)"},{"line_number":139,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3afc51ec_32acdb34","line":136,"updated":"2016-03-14 15:02:47.000000000","message":"Please use keystone wrapper here https://github.com/openstack/rally/blob/master/rally/plugins/openstack/context/keystone/roles.py#L50","commit_id":"685e716a7cad6e38121bc94b4d8cd701e949c516"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"4af77290669430680b90136319173ad98b243b1a","unresolved":false,"context_lines":[{"line_number":133,"context_line":"        def consume(cache, args):"},{"line_number":134,"context_line":"            user_id, role_id, project_id \u003d args"},{"line_number":135,"context_line":"            if \"client\" not in cache:"},{"line_number":136,"context_line":"                clients \u003d osclients.Clients(self.credential)"},{"line_number":137,"context_line":"                cache[\"client\"] \u003d keystone.wrap(clients.keystone())"},{"line_number":138,"context_line":"            cache[\"client\"].remove_role(user_id, role_id, project_id)"},{"line_number":139,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3afc51ec_584d496b","line":136,"in_reply_to":"3afc51ec_32acdb34","updated":"2016-03-14 15:06:05.000000000","message":"Sorry. This is my mistake. I haven\u0027t saw the string lower","commit_id":"685e716a7cad6e38121bc94b4d8cd701e949c516"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"fb428b47b2516948ab738237d9107d855ea659ac","unresolved":false,"context_lines":[{"line_number":135,"context_line":"            if \"client\" not in cache:"},{"line_number":136,"context_line":"                clients \u003d osclients.Clients(self.credential)"},{"line_number":137,"context_line":"                cache[\"client\"] \u003d keystone.wrap(clients.keystone())"},{"line_number":138,"context_line":"            cache[\"client\"].remove_role(user_id, role_id, project_id)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"        broker.run(publish, consume, threads)"},{"line_number":141,"context_line":"        self.context[\"roles\"] \u003d {}"}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_b0867e63","line":138,"updated":"2016-03-15 13:18:46.000000000","message":"publish() and consume() are very similar to the publish() and consume() above. I think I\u0027d rather you define _get_publisher(self, role) and _get_consumer(self, func) that return a publisher for a given role and a consumer for a given function (add_role()/remove_role()) to deduplicate this code.","commit_id":"685e716a7cad6e38121bc94b4d8cd701e949c516"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"fb428b47b2516948ab738237d9107d855ea659ac","unresolved":false,"context_lines":[{"line_number":138,"context_line":"            cache[\"client\"].remove_role(user_id, role_id, project_id)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"        broker.run(publish, consume, threads)"},{"line_number":141,"context_line":"        self.context[\"roles\"] \u003d {}"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    @logging.log_task_wrapper(LOG.info, _(\"Enter context: `roles`\"))"},{"line_number":144,"context_line":"    def setup(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_f57118e2","line":141,"updated":"2016-03-15 13:18:46.000000000","message":"This isn\u0027t necessary, and it\u0027s wrong, too. This function removes one role, not all of them.","commit_id":"685e716a7cad6e38121bc94b4d8cd701e949c516"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"fb428b47b2516948ab738237d9107d855ea659ac","unresolved":false,"context_lines":[{"line_number":144,"context_line":"    def setup(self):"},{"line_number":145,"context_line":"        \"\"\"Add roles to all users.\"\"\""},{"line_number":146,"context_line":"        self.context[\"roles\"] \u003d [self._add_role(self.credential, name)"},{"line_number":147,"context_line":"                                 for name in self.config[\"role-list\"]]"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"    @logging.log_task_wrapper(LOG.info, _(\"Exit context: `roles`\"))"},{"line_number":150,"context_line":"    def cleanup(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_70b6b63e","line":147,"updated":"2016-03-15 13:18:46.000000000","message":"Why aren\u0027t we parallelizing across roles as well as across users? This only gives a performance boost when you have lots of users and few roles. If I\u0027m trying to add 30 roles to a single user, that still happens sequentially. I\u0027d rather see the setup() and cleanup() functions refactored so that the threading applies across the product of users x roles.","commit_id":"685e716a7cad6e38121bc94b4d8cd701e949c516"},{"author":{"_account_id":6835,"name":"Kun Huang","email":"gareth@openstacker.org","username":"academicgareth"},"change_message_id":"55f83f6a945a7fe61fafa7526b0f96fc616cac5c","unresolved":false,"context_lines":[{"line_number":86,"context_line":"        def publish(queue):"},{"line_number":87,"context_line":"            for context_role in self.config:"},{"line_number":88,"context_line":"                role \u003d self._get_role_object(admin_credential, context_role)"},{"line_number":89,"context_line":"                roles_dict \u003d {\"id\": str(role.id), \"name\": str(role.name)}"},{"line_number":90,"context_line":"                roles_list.append(roles_dict)"},{"line_number":91,"context_line":"                LOG.debug(\"Adding role %(role)s to all users using %(threads)s\""},{"line_number":92,"context_line":"                          \"threads\" % {\"role\": role.id,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa0719c6_fb5fe43e","line":89,"range":{"start_line":89,"start_character":16,"end_line":89,"end_character":26},"updated":"2016-03-22 17:35:46.000000000","message":"actually it is role_dict here, not roleS_dict which is confusing with L110","commit_id":"c9a3136bbdafb365a65fe7a125645b0d742f9f83"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":60,"context_line":"        self.credential \u003d self.context[\"admin\"][\"credential\"]"},{"line_number":61,"context_line":"        self.workers \u003d cfg.CONF.roles_context.resource_management_workers"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def _get_role_object(self, admin_credential, context_role):"},{"line_number":64,"context_line":"        \"\"\"Check if role exists."},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        :param admin_credential: The admin credential."}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_c6b080f7","line":63,"updated":"2016-03-29 13:05:44.000000000","message":"Why do we pass admin_credential around to all of these functions instead of just using self.credential?","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        for def_role in default_roles:"},{"line_number":72,"context_line":"            if str(def_role.name) \u003d\u003d context_role:"},{"line_number":73,"context_line":"                role \u003d def_role"},{"line_number":74,"context_line":"                return role"},{"line_number":75,"context_line":"        else:"},{"line_number":76,"context_line":"            raise exceptions.NoSuchRole(role\u003dcontext_role)"},{"line_number":77,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_c6b9a0eb","line":74,"updated":"2016-03-29 13:05:44.000000000","message":"Why not just:\n\n    return def_role","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        else:"},{"line_number":76,"context_line":"            raise exceptions.NoSuchRole(role\u003dcontext_role)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"    def _add_role(self, admin_credential):"},{"line_number":79,"context_line":"        \"\"\"Add role to users."},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"        :param admin_credential: The admin credential."}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_8694c837","line":78,"updated":"2016-03-29 13:05:44.000000000","message":"Maybe _add_roles() instead, since this adds all roles to the users?","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":89,"context_line":"                role_dict \u003d {\"id\": str(role.id), \"name\": str(role.name)}"},{"line_number":90,"context_line":"                roles_list.append(role_dict)"},{"line_number":91,"context_line":"                LOG.debug(\"Adding role %(role)s to all users using %(threads)s\""},{"line_number":92,"context_line":"                          \"threads\" % {\"role\": role.id,"},{"line_number":93,"context_line":"                                       \"threads\": self.workers})"},{"line_number":94,"context_line":"                for user in self.context[\"users\"]:"},{"line_number":95,"context_line":"                    args \u003d (role, user[\"id\"], user[\"tenant_id\"])"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_a6dd8c16","line":92,"updated":"2016-03-29 13:05:44.000000000","message":"Include the role name here, too.","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":92,"context_line":"                          \"threads\" % {\"role\": role.id,"},{"line_number":93,"context_line":"                                       \"threads\": self.workers})"},{"line_number":94,"context_line":"                for user in self.context[\"users\"]:"},{"line_number":95,"context_line":"                    args \u003d (role, user[\"id\"], user[\"tenant_id\"])"},{"line_number":96,"context_line":"                    queue.append(args)"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        def consume(cache, args):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_c6036072","line":95,"updated":"2016-03-29 13:05:44.000000000","message":"Since consume() only uses the role ID, only pass the role ID here.","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":107,"context_line":"        broker.run(publish, consume, threads)"},{"line_number":108,"context_line":"        roles_dict \u003d {}"},{"line_number":109,"context_line":"        for r in roles_list:"},{"line_number":110,"context_line":"            roles_dict[r[\"id\"]] \u003d r[\"name\"]"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"        return roles_dict"},{"line_number":113,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_467f50aa","line":110,"updated":"2016-03-29 13:05:44.000000000","message":"This whole business with role_dict, roles_list, and roles_dict seems overcomplicated. If all we want is a dict that maps role IDs to names, then publish should create that instead of creating a deque of dicts and deduplicating it later.\n\nAlternatively, if there\u0027s some reason that we need to do it this way, then please add a detailed comment explaining why. You could also improve this approach by adding tuples of (id, name) to the deque, and then just doing:\n\n    return dict(roles_list)\n\nSince a list of tuples dict-ifies nicely.","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"        return roles_dict"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def _remove_role(self, admin_credential):"},{"line_number":115,"context_line":"        \"\"\"Remove given role from users."},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"        :param admin_credential: The admin credential."}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_09ad6be9","line":114,"updated":"2016-03-29 13:05:44.000000000","message":"_remove_roles()","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        return roles_dict"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def _remove_role(self, admin_credential):"},{"line_number":115,"context_line":"        \"\"\"Remove given role from users."},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"        :param admin_credential: The admin credential."},{"line_number":118,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_e97c7774","line":115,"updated":"2016-03-29 13:05:44.000000000","message":"Remove *all* roles....","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":120,"context_line":""},{"line_number":121,"context_line":"        def publish(queue):"},{"line_number":122,"context_line":"            for role_id in self.context[\"roles\"]:"},{"line_number":123,"context_line":"                LOG.debug(\"Removing role %s from all users\" % (role_id))"},{"line_number":124,"context_line":"                for user in self.context[\"users\"]:"},{"line_number":125,"context_line":"                    args \u003d (role_id, user[\"id\"], user[\"tenant_id\"])"},{"line_number":126,"context_line":"                    queue.append(args)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_e644a475","line":123,"updated":"2016-03-29 13:05:44.000000000","message":"Include the role name here too.","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            if \"client\" not in cache:"},{"line_number":131,"context_line":"                clients \u003d osclients.Clients(self.credential)"},{"line_number":132,"context_line":"                cache[\"client\"] \u003d keystone.wrap(clients.keystone())"},{"line_number":133,"context_line":"            cache[\"client\"].remove_role(user_id, role_id, project_id)"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"        broker.run(publish, consume, threads)"},{"line_number":136,"context_line":"        self.context[\"roles\"] \u003d {}"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_c97373b0","line":133,"updated":"2016-03-29 13:05:44.000000000","message":"Your two consume() functions are very similar, and do not use any variable closures. To deduplicate code, they can be abstracted out to a single function:\n\n    def _get_consumer(self, func_name):\n        def consume(cache, args):\n            role_id, user_id, project_id \u003d args\n            if \"client\" not in cache:\n                clients \u003d osclients.Clients(self.credential)\n                cache[\"client\"] \u003d keystone.wrap(clients.keystone())\n            getattr(cache[\"client\"], func_name)(user_id, role_id, project_id)\n        return consume\n\nAnd:\n\n    broker.run(publish, self._get_consumer(\"remove_role\"), threads)","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":133,"context_line":"            cache[\"client\"].remove_role(user_id, role_id, project_id)"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"        broker.run(publish, consume, threads)"},{"line_number":136,"context_line":"        self.context[\"roles\"] \u003d {}"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    @logging.log_task_wrapper(LOG.info, _(\"Enter context: `roles`\"))"},{"line_number":139,"context_line":"    def setup(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_e9a59704","line":136,"updated":"2016-03-29 13:05:44.000000000","message":"This isn\u0027t necessary; contexts aren\u0027t reused after teardown.","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":138,"context_line":"    @logging.log_task_wrapper(LOG.info, _(\"Enter context: `roles`\"))"},{"line_number":139,"context_line":"    def setup(self):"},{"line_number":140,"context_line":"        \"\"\"Add roles to all users.\"\"\""},{"line_number":141,"context_line":"        self.context[\"roles\"] \u003d self._add_role(self.credential)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    @logging.log_task_wrapper(LOG.info, _(\"Exit context: `roles`\"))"},{"line_number":144,"context_line":"    def cleanup(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_69e86711","line":141,"updated":"2016-03-29 13:05:44.000000000","message":"The extra layer of indirection here and in cleanup() seems unnecessary. You can just move the code from _add_role()/_remove_role() into setup() and cleanup().","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"236d40cdeaa03aabd4a16df4127887cb2a3a9e4e","unresolved":false,"context_lines":[{"line_number":32,"context_line":"ROLES_CONTEXT_OPTS \u003d ["},{"line_number":33,"context_line":"    cfg.IntOpt(\"resource_management_workers\","},{"line_number":34,"context_line":"               default\u003d30,"},{"line_number":35,"context_line":"               help\u003d\"How many concurrent threads use for serving roles \""},{"line_number":36,"context_line":"                    \"context\"),"},{"line_number":37,"context_line":"]"},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"7a1b2928_0fe57936","line":35,"updated":"2016-04-11 15:19:49.000000000","message":"...threads *to use...","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"},{"author":{"_account_id":20196,"name":"Piyush","email":"piyush2011257@gmail.com","username":"pirsriva"},"change_message_id":"614050b00ac313ac17593bf1c2d7f6a4b9e9611f","unresolved":false,"context_lines":[{"line_number":32,"context_line":"ROLES_CONTEXT_OPTS \u003d ["},{"line_number":33,"context_line":"    cfg.IntOpt(\"resource_management_workers\","},{"line_number":34,"context_line":"               default\u003d30,"},{"line_number":35,"context_line":"               help\u003d\"How many concurrent threads use for serving roles \""},{"line_number":36,"context_line":"                    \"context\"),"},{"line_number":37,"context_line":"]"},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_5880b446","line":35,"in_reply_to":"7a1b2928_0fe57936","updated":"2016-04-18 12:14:37.000000000","message":"Done","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"236d40cdeaa03aabd4a16df4127887cb2a3a9e4e","unresolved":false,"context_lines":[{"line_number":86,"context_line":"    def setup(self):"},{"line_number":87,"context_line":"        \"\"\"Add all roles to users.\"\"\""},{"line_number":88,"context_line":"        threads \u003d self.workers"},{"line_number":89,"context_line":"        roles_list \u003d collections.deque()"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        def publish(queue):"},{"line_number":92,"context_line":"            for context_role in self.config:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a1b2928_4f3131a2","line":89,"updated":"2016-04-11 15:19:49.000000000","message":"Why do we still have this deque object? Why not just populate the dict directly? If there is some reason we need to do this, please add a detailed comment explaining why.","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"},{"author":{"_account_id":20196,"name":"Piyush","email":"piyush2011257@gmail.com","username":"pirsriva"},"change_message_id":"614050b00ac313ac17593bf1c2d7f6a4b9e9611f","unresolved":false,"context_lines":[{"line_number":86,"context_line":"    def setup(self):"},{"line_number":87,"context_line":"        \"\"\"Add all roles to users.\"\"\""},{"line_number":88,"context_line":"        threads \u003d self.workers"},{"line_number":89,"context_line":"        roles_list \u003d collections.deque()"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        def publish(queue):"},{"line_number":92,"context_line":"            for context_role in self.config:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_18ed9c25","line":89,"in_reply_to":"7a1b2928_4f3131a2","updated":"2016-04-18 12:14:37.000000000","message":"Yes you are correct. Publish() is sequential in broker, hence no need of collections.deque(). Changed to dictionary. ( roles_list -\u003e roles_dict )","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"236d40cdeaa03aabd4a16df4127887cb2a3a9e4e","unresolved":false,"context_lines":[{"line_number":91,"context_line":"        def publish(queue):"},{"line_number":92,"context_line":"            for context_role in self.config:"},{"line_number":93,"context_line":"                role \u003d self._get_role_object(context_role)"},{"line_number":94,"context_line":"                roles_list.append((str(role.id), str(role.name)))"},{"line_number":95,"context_line":"                LOG.debug(\"Adding role %(role_name)s having ID %(role_id)s \""},{"line_number":96,"context_line":"                          \"to all users using %(threads)s threads\" %"},{"line_number":97,"context_line":"                          {\"role_name\": role.name,"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a1b2928_2ff2f5d2","line":94,"updated":"2016-04-11 15:19:49.000000000","message":"Aren\u0027t these both already strings (or string-like objects)?","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"},{"author":{"_account_id":20196,"name":"Piyush","email":"piyush2011257@gmail.com","username":"pirsriva"},"change_message_id":"614050b00ac313ac17593bf1c2d7f6a4b9e9611f","unresolved":false,"context_lines":[{"line_number":91,"context_line":"        def publish(queue):"},{"line_number":92,"context_line":"            for context_role in self.config:"},{"line_number":93,"context_line":"                role \u003d self._get_role_object(context_role)"},{"line_number":94,"context_line":"                roles_list.append((str(role.id), str(role.name)))"},{"line_number":95,"context_line":"                LOG.debug(\"Adding role %(role_name)s having ID %(role_id)s \""},{"line_number":96,"context_line":"                          \"to all users using %(threads)s threads\" %"},{"line_number":97,"context_line":"                          {\"role_name\": role.name,"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_f8e8a012","line":94,"in_reply_to":"7a1b2928_2ff2f5d2","updated":"2016-04-18 12:14:37.000000000","message":"Yeah both are unicode string. Fixed","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"},{"author":{"_account_id":20196,"name":"Piyush","email":"piyush2011257@gmail.com","username":"pirsriva"},"change_message_id":"539e5644997376c920ef6cd7660c65e33c9a5f1c","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                    queue.append(args)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        broker.run(publish, self._get_consumer(\"remove_role\"), threads)"},{"line_number":120,"context_line":"        self.context[\"roles\"] \u003d {}"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a061dce_e2480530","line":120,"range":{"start_line":120,"start_character":0,"end_line":120,"end_character":34},"updated":"2016-04-05 08:03:47.000000000","message":"Have kept this to keep it consistent with users context. E.g. https://github.com/openstack/rally/blob/master/rally/plugins/openstack/context/keystone/users.py#L266","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"},{"author":{"_account_id":20196,"name":"Piyush","email":"piyush2011257@gmail.com","username":"pirsriva"},"change_message_id":"614050b00ac313ac17593bf1c2d7f6a4b9e9611f","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                    queue.append(args)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        broker.run(publish, self._get_consumer(\"remove_role\"), threads)"},{"line_number":120,"context_line":"        self.context[\"roles\"] \u003d {}"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_98c08c7e","line":120,"range":{"start_line":120,"start_character":0,"end_line":120,"end_character":34},"in_reply_to":"9a061dce_d195bae4","updated":"2016-04-18 12:14:37.000000000","message":"Done","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"236d40cdeaa03aabd4a16df4127887cb2a3a9e4e","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                    queue.append(args)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        broker.run(publish, self._get_consumer(\"remove_role\"), threads)"},{"line_number":120,"context_line":"        self.context[\"roles\"] \u003d {}"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a061dce_d195bae4","line":120,"range":{"start_line":120,"start_character":0,"end_line":120,"end_character":34},"in_reply_to":"9a061dce_e2480530","updated":"2016-04-11 15:19:49.000000000","message":"Actually, please don\u0027t keep it consistent. That code isn\u0027t necessary either, and can be removed.","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"}],"tests/unit/plugins/openstack/context/keystone/test_roles.py":[{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":68,"context_line":"        ctx \u003d roles.RoleGenerator(self.context)"},{"line_number":69,"context_line":"        ctx.context[\"users\"] \u003d [{\"id\": \"u1\", \"tenant_id\": \"t1\"},"},{"line_number":70,"context_line":"                                {\"id\": \"u2\", \"tenant_id\": \"t2\"}]"},{"line_number":71,"context_line":"        ex \u003d self.assertRaises(exceptions.NoSuchRole, ctx._get_role_object,"},{"line_number":72,"context_line":"                               mock.MagicMock(), \"unknown_role\")"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        expected \u003d \"There is no role with name `unknown_role`.\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_c95d33eb","line":71,"updated":"2016-03-29 13:05:44.000000000","message":"It seems like this should be tested against setup(); testing it against _get_role_object() is a bit too narrow IMO.","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"62fc65dc5b882f5f70fa00312c329d7993cb9172","unresolved":false,"context_lines":[{"line_number":86,"context_line":"        ctx.context[\"users\"] \u003d [{\"id\": \"u1\", \"tenant_id\": \"t1\"},"},{"line_number":87,"context_line":"                                {\"id\": \"u2\", \"tenant_id\": \"t2\"}]"},{"line_number":88,"context_line":"        ctx._remove_role(mock.MagicMock())"},{"line_number":89,"context_line":"        self.assertEqual(len(ctx.context[\"roles\"]), 0)"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    @mock.patch(\"%s.osclients\" % CTX)"},{"line_number":92,"context_line":"    def test_setup_and_cleanup(self, mock_osclients):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_a92c3f4b","line":89,"updated":"2016-03-29 13:05:44.000000000","message":"This needs much better assertions, similar to the original test.","commit_id":"b64983a6946167375174eee2b331c694d0b81834"},{"author":{"_account_id":20196,"name":"Piyush","email":"piyush2011257@gmail.com","username":"pirsriva"},"change_message_id":"539e5644997376c920ef6cd7660c65e33c9a5f1c","unresolved":false,"context_lines":[{"line_number":71,"context_line":"                                {\"id\": \"u2\", \"tenant_id\": \"t2\"}]"},{"line_number":72,"context_line":"        ctx.config \u003d [\"unknown_role\"]"},{"line_number":73,"context_line":"        ctx.credential \u003d mock.MagicMock()"},{"line_number":74,"context_line":"        ex \u003d self.assertRaises(exceptions.NoSuchRole, ctx._get_role_object,"},{"line_number":75,"context_line":"                               \"unknown_role\")"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"        expected \u003d \"There is no role with name `unknown_role`.\""}],"source_content_type":"text/x-python","patch_set":4,"id":"9a061dce_22ef9de9","line":74,"updated":"2016-04-05 08:03:47.000000000","message":"Cannot check for exception in ctx.setup() because of the way broker will work. The exception raised by _get_role_object() will be caught at https://github.com/openstack/rally/blob/master/rally/common/broker.py#L58 and NOT reflected in setup()","commit_id":"487dd02b68048170a52d65dd8b5f5422f25cad9c"}]}