)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8978,"name":"Marek Denis","email":"marek.denis+openstack@gmail.com","username":"marek-denis"},"change_message_id":"5fc431fb91051ef48495ba4fd415652d204c2ce3","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Marcos Fermin Lobo \u003cmarcos.fermin.lobo@cern.ch\u003e"},{"line_number":5,"context_line":"CommitDate: 2014-06-25 11:57:00 +0200"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"NotImplemented _for_groups functions on LDAP"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"These functions are not implemented on assignment LDAP backend:"},{"line_number":10,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"baada198_66291437","line":7,"updated":"2014-07-22 22:24:26.000000000","message":"how about hanging the main commit message to something like:\n\nGroup related methods for LDAP backend\n?","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1568fd934a56bdcf42d35159b7d9f0746604c74d","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Marcos Fermin Lobo \u003cmarcos.fermin.lobo@cern.ch\u003e"},{"line_number":5,"context_line":"CommitDate: 2014-06-25 11:57:00 +0200"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"NotImplemented _for_groups functions on LDAP"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"These functions are not implemented on assignment LDAP backend:"},{"line_number":10,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"baada198_752a8230","line":7,"in_reply_to":"baada198_66291437","updated":"2014-07-25 08:01:57.000000000","message":"Done","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":11333,"name":"David J Hu","email":"david.hu@hpe.com","username":"dhu"},"change_message_id":"a42e3010582823f820c7c220e79cc5bb667d5c32","unresolved":false,"context_lines":[{"line_number":11,"context_line":"- get_roles_for_groups"},{"line_number":12,"context_line":"- list_projects_for_groups"},{"line_number":13,"context_line":"- list_domains_for_groups"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: I0020a1a78004cfca1bcec40ecc938d3e2362dba8"},{"line_number":16,"context_line":"Closes-Bug: #1333712"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1ae5cdf2_1d5cdd7c","line":14,"updated":"2014-06-25 16:53:04.000000000","message":"Please describe what you are trying to implement in the commit message.  It looks like you are trying to implement couple of unimplemented *_for_groups() functions.","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"ec2004703e2697905aaa8b21eb4a3a8f1b127a44","unresolved":false,"context_lines":[{"line_number":11,"context_line":"- get_roles_for_groups"},{"line_number":12,"context_line":"- list_projects_for_groups"},{"line_number":13,"context_line":"- list_domains_for_groups"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: I0020a1a78004cfca1bcec40ecc938d3e2362dba8"},{"line_number":16,"context_line":"Closes-Bug: #1333712"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1ae5cdf2_63ca8d6f","line":14,"in_reply_to":"1ae5cdf2_1d5cdd7c","updated":"2014-06-26 06:43:26.000000000","message":"Yes, I\u0027m just trying to implement 3 unimplemented functions on LDAP backend. These functions are already implemented in the others backends (SQL and KVS).","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"ff32f9dee46aecdc35a4dd984e5ed69a4fb5a0dd","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Marcos Fermin Lobo \u003cmarcos.fermin.lobo@cern.ch\u003e"},{"line_number":5,"context_line":"CommitDate: 2014-08-07 14:09:59 +0200"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Group related methods for LDAP backend"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This patch implements three functions, empty in the LDAP backend,"},{"line_number":10,"context_line":"which are necessary to obtain information about groups,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"baada198_9deb75a4","line":7,"updated":"2014-08-08 16:10:23.000000000","message":"I think this would read better as: \u0027Implement group related methods for LDAP backend\u0027. When I read this I thought you were grouping related methods together.","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"71f3c0b92cdd4978c2e2bf976591633a6fc0e841","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Marcos Fermin Lobo \u003cmarcos.fermin.lobo@cern.ch\u003e"},{"line_number":5,"context_line":"CommitDate: 2014-08-11 11:08:02 +0200"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Group related methods for LDAP backend"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This patch implements three functions, empty in the LDAP backend,"},{"line_number":10,"context_line":"which are necessary to obtain information about groups,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"baada198_0c9079f2","line":7,"updated":"2014-08-13 15:13:50.000000000","message":"I still think my comment from patchset 5 applies here.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1874f7dc2872c18a1123384eb149bb305f74dc4c","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Marcos Fermin Lobo \u003cmarcos.fermin.lobo@cern.ch\u003e"},{"line_number":5,"context_line":"CommitDate: 2014-08-11 11:08:02 +0200"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Group related methods for LDAP backend"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This patch implements three functions, empty in the LDAP backend,"},{"line_number":10,"context_line":"which are necessary to obtain information about groups,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"7ac7a964_2476503f","line":7,"in_reply_to":"baada198_0c9079f2","updated":"2014-08-14 08:27:35.000000000","message":"I\u0027m sorry David, I\u0027m agree with your comment on patch 5... my fault.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"}],"keystone/assignment/backends/ldap.py":[{"author":{"_account_id":11333,"name":"David J Hu","email":"david.hu@hpe.com","username":"dhu"},"change_message_id":"a42e3010582823f820c7c220e79cc5bb667d5c32","unresolved":false,"context_lines":[{"line_number":169,"context_line":"        return [self._set_default_domain(self.get_project(project_id))"},{"line_number":170,"context_line":"                for project_id in project_ids]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_domains_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        return [assignment.calc_default_domain()]"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def list_user_ids_for_project(self, tenant_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_dd7a75ef","line":172,"updated":"2014-06-25 16:53:04.000000000","message":"I thought you want a list of domains associated with the group_ids?  The code looks no different from the generic list_domains() function.","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"6116f1d88061d8c8bda6ad4f3407f3972c2cd78f","unresolved":false,"context_lines":[{"line_number":169,"context_line":"        return [self._set_default_domain(self.get_project(project_id))"},{"line_number":170,"context_line":"                for project_id in project_ids]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_domains_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        return [assignment.calc_default_domain()]"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def list_user_ids_for_project(self, tenant_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_7a3fe325","line":172,"in_reply_to":"1ae5cdf2_631c4d14","updated":"2014-07-01 16:55:40.000000000","message":"It LDAP doesn\u0027t actually support this functionality why \"fake\" it here?","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"ec2004703e2697905aaa8b21eb4a3a8f1b127a44","unresolved":false,"context_lines":[{"line_number":169,"context_line":"        return [self._set_default_domain(self.get_project(project_id))"},{"line_number":170,"context_line":"                for project_id in project_ids]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_domains_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        return [assignment.calc_default_domain()]"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def list_user_ids_for_project(self, tenant_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ae5cdf2_631c4d14","line":172,"in_reply_to":"1ae5cdf2_dd7a75ef","updated":"2014-06-26 06:43:26.000000000","message":"That\u0027s because LDAP does not support domains, so always is the default domain. This function is more relevant in other backends like SQL or KVS.","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"eb43caaf1ceb595650296f05c5009eccb77d89a4","unresolved":false,"context_lines":[{"line_number":169,"context_line":"        return [self._set_default_domain(self.get_project(project_id))"},{"line_number":170,"context_line":"                for project_id in project_ids]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_domains_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        return [assignment.calc_default_domain()]"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def list_user_ids_for_project(self, tenant_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_ec6c019f","line":172,"in_reply_to":"baada198_7a3fe325","updated":"2014-07-02 12:44:10.000000000","message":"David Stanek, I think it is a good idea because in other LDAP backend functions, related with \"domains\", we return the same value https://github.com/openstack/keystone/blob/master/keystone/assignment/backends/ldap.py#L261","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"51bea4ec62dc1a9d39c11d21ab43f323b9311a79","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        associations \u003d []"},{"line_number":161,"context_line":"        project_subtree_dn \u003d self.project.tree_dn"},{"line_number":162,"context_line":"        if project_id is not None:"},{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for g in self.role.list_project_roles_for_group("}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_836a6bb3","line":163,"updated":"2014-08-07 13:23:04.000000000","message":"What about changing the above to this?\n\nif project_id is not None:\n    project_subtree_dn \u003d self.project._id_to_dn(project_id)\nelse:\n    project_subtree_dn \u003d self.project.tree_dn","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"ff32f9dee46aecdc35a4dd984e5ed69a4fb5a0dd","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        associations \u003d []"},{"line_number":161,"context_line":"        project_subtree_dn \u003d self.project.tree_dn"},{"line_number":162,"context_line":"        if project_id is not None:"},{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for g in self.role.list_project_roles_for_group("}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_bd3f5921","line":163,"in_reply_to":"baada198_836a6bb3","updated":"2014-08-08 16:10:23.000000000","message":"++","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"df111f8a6dcccb60cdc95339b06010f626cfeb65","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        associations \u003d []"},{"line_number":161,"context_line":"        project_subtree_dn \u003d self.project.tree_dn"},{"line_number":162,"context_line":"        if project_id is not None:"},{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for g in self.role.list_project_roles_for_group("}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_93e17d99","line":163,"in_reply_to":"baada198_836a6bb3","updated":"2014-08-11 09:08:21.000000000","message":"I think, if we put an \"IF-ELSE\" statement, it will increase the complexity of the code (http://en.wikipedia.org/wiki/Cyclomatic_complexity#Definition). Otherwise, in my opinion, the code is enough plain.\n\nWhat do you think Juan Antonio?, please, let me know if you are agree with my answer.","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"5fa116f28aa6e11243d0f175f03231fcef620d46","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        associations \u003d []"},{"line_number":161,"context_line":"        project_subtree_dn \u003d self.project.tree_dn"},{"line_number":162,"context_line":"        if project_id is not None:"},{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for g in self.role.list_project_roles_for_group("}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_ecdc25e4","line":163,"in_reply_to":"baada198_93e17d99","updated":"2014-08-13 15:07:15.000000000","message":"Adding the else will not increase complexity because there are still two code paths. project_id may be None or it may not. The \u0027else\u0027 doesn\u0027t make that any different, just explicit.","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"d2b4460b8003314e52750e7dd899ac1ae304310c","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        associations \u003d []"},{"line_number":161,"context_line":"        project_subtree_dn \u003d self.project.tree_dn"},{"line_number":162,"context_line":"        if project_id is not None:"},{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for g in self.role.list_project_roles_for_group("}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_626091a4","line":163,"in_reply_to":"baada198_93e17d99","updated":"2014-08-11 17:40:51.000000000","message":"Even though it increases the \"different paths\" through the source code. What we want to achieve is to have readable and hopefully efficient code. Though my emphasis is usually on readability, since you want your code to be maintainable. Anyway, to someone that just takes a glimpse at your source code (which I tested with the colleagues seating beside me), assigning a variable, and then replacing it on the line below seemed somewhat... out of place maybe? or slightly confusing. I really think that your intent becomes more apparent if you put the code in the way that I proposed initially.\n\nI do like the fact that you\u0027re trying to reduce code complexity, and it\u0027s a very good thing to do! But I think that would apply to a bit bigger pieces, other than this simple IF ... ELSE ... statement.\n\nOn the other hand, if you want to be really explicit about your intent, you could even separate the whole chunk to a separate function, there you can explain in the doc string why you are overwriting project_subtree_dn instead of using what comes from self.project.tree_dn .","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"51bea4ec62dc1a9d39c11d21ab43f323b9311a79","unresolved":false,"context_lines":[{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for g in self.role.list_project_roles_for_group("},{"line_number":167,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":168,"context_line":"                associations.append(self.role._dn_to_id(g.role_dn))"},{"line_number":169,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_23143f40","line":166,"updated":"2014-08-07 13:23:04.000000000","message":"Could you please avoid using a single letter notation for variables? \u0027g\u0027 is not very descriptive.","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"51bea4ec62dc1a9d39c11d21ab43f323b9311a79","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":168,"context_line":"                associations.append(self.role._dn_to_id(g.role_dn))"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"        return [self.get_role(r) for r in associations]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_projects_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        project_ids \u003d []"}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_e33087ec","line":170,"updated":"2014-08-07 13:23:04.000000000","message":"same here, \u0027r\u0027 is not very descriptive.","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"d9fc9c43a608d8abff7f104bc6387fe86f0caea2","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":168,"context_line":"                associations.append(self.role._dn_to_id(g.role_dn))"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"        return [self.get_role(r) for r in associations]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_projects_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        project_ids \u003d []"}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_a50a101c","line":170,"in_reply_to":"baada198_e33087ec","updated":"2014-08-07 14:46:07.000000000","message":"You are right Juan, I\u0027ll change it. Thanks for you review.","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"796d47b142f0e1e153a429435abd716dc0d5f977","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        associations \u003d []"},{"line_number":161,"context_line":"        project_subtree_dn \u003d self.project.tree_dn"},{"line_number":162,"context_line":"        if project_id is not None:"},{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for group_role in self.role.list_project_roles_for_group("}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_02fc35b5","line":163,"updated":"2014-08-11 10:50:34.000000000","message":"I left my comments regarding this on patch set 5. But tldr; I would still prefer to see this changed.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"71f3c0b92cdd4978c2e2bf976591633a6fc0e841","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        associations \u003d []"},{"line_number":161,"context_line":"        project_subtree_dn \u003d self.project.tree_dn"},{"line_number":162,"context_line":"        if project_id is not None:"},{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for group_role in self.role.list_project_roles_for_group("}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_4c0d1140","line":163,"in_reply_to":"baada198_02fc35b5","updated":"2014-08-13 15:13:50.000000000","message":"++","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1874f7dc2872c18a1123384eb149bb305f74dc4c","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        associations \u003d []"},{"line_number":161,"context_line":"        project_subtree_dn \u003d self.project.tree_dn"},{"line_number":162,"context_line":"        if project_id is not None:"},{"line_number":163,"context_line":"            project_subtree_dn \u003d self.project._id_to_dn(project_id)"},{"line_number":164,"context_line":"        for group_id in group_ids:"},{"line_number":165,"context_line":"            group_dn \u003d self.group._id_to_dn(group_id)"},{"line_number":166,"context_line":"            for group_role in self.role.list_project_roles_for_group("}],"source_content_type":"text/x-python","patch_set":6,"id":"7ac7a964_c4e2fc02","line":163,"in_reply_to":"baada198_4c0d1140","updated":"2014-08-14 08:27:35.000000000","message":"Ok, I\u0027ll change it.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"796d47b142f0e1e153a429435abd716dc0d5f977","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":168,"context_line":"                associations.append(self.role._dn_to_id(group_role.role_dn))"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"        return [self.get_role(r) for r in associations]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_projects_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        project_ids \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_e238213d","line":170,"updated":"2014-08-11 10:50:34.000000000","message":"haha, would still prefer something else than \u0027r\u0027","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"3196b303b27014de19d15c58de3736fbdd515a56","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":168,"context_line":"                associations.append(self.role._dn_to_id(group_role.role_dn))"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"        return [self.get_role(r) for r in associations]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_projects_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        project_ids \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"7ac7a964_5326b0bb","line":170,"in_reply_to":"7ac7a964_c43afc35","updated":"2014-08-14 09:05:14.000000000","message":"well, instead of trying to remember and translate what stuff should be, it would be better not to have one char variable names and just write self-explanatory code ;)","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1874f7dc2872c18a1123384eb149bb305f74dc4c","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":168,"context_line":"                associations.append(self.role._dn_to_id(group_role.role_dn))"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"        return [self.get_role(r) for r in associations]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_projects_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        project_ids \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"7ac7a964_c43afc35","line":170,"in_reply_to":"baada198_af0a1f1c","updated":"2014-08-14 08:27:35.000000000","message":"David, you are right, it is a list of role_ids. I\u0027ll change \"associations\" for other name like \"role_ids\" and \"r\" for \"role\"","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"71f3c0b92cdd4978c2e2bf976591633a6fc0e841","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":168,"context_line":"                associations.append(self.role._dn_to_id(group_role.role_dn))"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"        return [self.get_role(r) for r in associations]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_projects_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        project_ids \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_af0a1f1c","line":170,"in_reply_to":"baada198_b6290885","updated":"2014-08-13 15:13:50.000000000","message":"I\u0027m OK with single variable names in cases like this, but it got me thinking about the name associations. Is this just a list of role_ids?","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"81221948ae291f37f477e77edf5a95f74310f616","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":168,"context_line":"                associations.append(self.role._dn_to_id(group_role.role_dn))"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"        return [self.get_role(r) for r in associations]"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def list_projects_for_groups(self, group_ids):"},{"line_number":173,"context_line":"        project_ids \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_b6290885","line":170,"in_reply_to":"baada198_e238213d","updated":"2014-08-11 11:54:49.000000000","message":"oopss!","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"126019b40c8746743f04a73d3584dd6ba178ecf2","unresolved":false,"context_lines":[{"line_number":168,"context_line":"                    group_dn, project_subtree_dn):"},{"line_number":169,"context_line":"                role_ids.append(self.role._dn_to_id(group_role.role_dn))"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"        return [self.get_role(id) for id in role_ids]"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"    def list_projects_for_groups(self, group_ids):"},{"line_number":174,"context_line":"        project_ids \u003d []"}],"source_content_type":"text/x-python","patch_set":7,"id":"7ac7a964_24a00590","line":171,"updated":"2014-08-14 11:41:13.000000000","message":"This is a lot less ambiguous now :D","commit_id":"ff90e0942c5a84cd85adf824d7c4c899746248cc"}],"keystone/tests/test_backend.py":[{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"b3eaee4aa366f2b1213d4bf74d5463a33f4e7b03","unresolved":false,"context_lines":[{"line_number":797,"context_line":"                                         role_id\u003dself.role_member[\u0027id\u0027])"},{"line_number":798,"context_line":""},{"line_number":799,"context_line":"        # The result is group1 has only one project granted"},{"line_number":800,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":803,"context_line":"        self.assertIn(self.role_member, roles)"}],"source_content_type":"text/x-python","patch_set":4,"id":"baada198_384c2576","line":800,"updated":"2014-07-25 10:14:48.000000000","message":"I\u0027ve tried to achieve the comments from Dolph and Henry but here, the SQL implementation [1], returns empty list.\n\nThe error occurs when you send to assignment_api.get_roles_for_groups() function only the group list and the domain_id. In this case, the SQL implementation returns empty list. But, if you send the group list and tenant_id, the SQL implementation works fine.\n\nI\u0027m still investigating this issue... maybe Henry has some clue for me.\n\n[1] - https://github.com/openstack/keystone/blob/master/keystone/assignment/backends/sql.py#L292","commit_id":"b87bbc2c48c2c7cd3cd8b2ffb55379cc25b1d418"},{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"df8f1d82173b95d59c695185cb8097960c85e74a","unresolved":false,"context_lines":[{"line_number":797,"context_line":"                                         role_id\u003dself.role_member[\u0027id\u0027])"},{"line_number":798,"context_line":""},{"line_number":799,"context_line":"        # The result is group1 has only one project granted"},{"line_number":800,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":803,"context_line":"        self.assertIn(self.role_member, roles)"}],"source_content_type":"text/x-python","patch_set":4,"id":"baada198_537d3ae6","line":800,"in_reply_to":"baada198_384c2576","updated":"2014-07-25 11:09:06.000000000","message":"Isn\u0027t an empty list the right answer?  You have not assigned any role for this group to the domain....","commit_id":"b87bbc2c48c2c7cd3cd8b2ffb55379cc25b1d418"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"4289089369c1ef8708839ccea0b9bc8eb35d7cd6","unresolved":false,"context_lines":[{"line_number":797,"context_line":"                                         role_id\u003dself.role_member[\u0027id\u0027])"},{"line_number":798,"context_line":""},{"line_number":799,"context_line":"        # The result is group1 has only one project granted"},{"line_number":800,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":803,"context_line":"        self.assertIn(self.role_member, roles)"}],"source_content_type":"text/x-python","patch_set":4,"id":"baada198_ce528035","line":800,"in_reply_to":"baada198_537d3ae6","updated":"2014-07-25 12:56:57.000000000","message":"Henry, what do you think if I copy this test to test_backend_sql and make the appropriate changes to make it works in test_backend and test_backend_sql?","commit_id":"b87bbc2c48c2c7cd3cd8b2ffb55379cc25b1d418"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"2c37b71e89418794dcfa6f78c55a03265eac333e","unresolved":false,"context_lines":[{"line_number":797,"context_line":"                                         role_id\u003dself.role_member[\u0027id\u0027])"},{"line_number":798,"context_line":""},{"line_number":799,"context_line":"        # The result is group1 has only one project granted"},{"line_number":800,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":803,"context_line":"        self.assertIn(self.role_member, roles)"}],"source_content_type":"text/x-python","patch_set":4,"id":"baada198_995a9380","line":800,"in_reply_to":"baada198_537d3ae6","updated":"2014-07-25 12:01:17.000000000","message":"You are right Henry but, the self.assertThat() line works in different way between LDAP and SQL, because this assert works fine in LDAP and it does not work in SQL, so, the behavior of get_roles_for_group function is different when you use only group_ids and domain_id parameters.\n\nYou can see in the log file [1] that the problem (I think) is on SQL backend.\n\n[1] - http://logs.openstack.org/44/102244/4/check/gate-keystone-python26/930e0a6/console.html","commit_id":"b87bbc2c48c2c7cd3cd8b2ffb55379cc25b1d418"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"71f3c0b92cdd4978c2e2bf976591633a6fc0e841","unresolved":false,"context_lines":[{"line_number":796,"context_line":"                                         project_id\u003dself.tenant_bar[\u0027id\u0027],"},{"line_number":797,"context_line":"                                         role_id\u003dself.role_member[\u0027id\u0027])"},{"line_number":798,"context_line":""},{"line_number":799,"context_line":"        # The result is group1 has only one project granted"},{"line_number":800,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(0))"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_8f3b9bc8","line":799,"updated":"2014-08-13 15:13:50.000000000","message":"I don\u0027t think this is correct.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"796d47b142f0e1e153a429435abd716dc0d5f977","unresolved":false,"context_lines":[{"line_number":799,"context_line":"        # The result is group1 has only one project granted"},{"line_number":800,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(0))"},{"line_number":803,"context_line":""},{"line_number":804,"context_line":"        # Same result sending project_id parameter"},{"line_number":805,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_62333113","line":802,"updated":"2014-08-11 10:50:34.000000000","message":"The comment for this section of code is misleading. It says that there should be only one project granted... yet, the assertion is that there are no roles.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1874f7dc2872c18a1123384eb149bb305f74dc4c","unresolved":false,"context_lines":[{"line_number":799,"context_line":"        # The result is group1 has only one project granted"},{"line_number":800,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(0))"},{"line_number":803,"context_line":""},{"line_number":804,"context_line":"        # Same result sending project_id parameter"},{"line_number":805,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("}],"source_content_type":"text/x-python","patch_set":6,"id":"7ac7a964_847e44e3","line":802,"in_reply_to":"baada198_62333113","updated":"2014-08-14 08:27:35.000000000","message":"+1","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"71f3c0b92cdd4978c2e2bf976591633a6fc0e841","unresolved":false,"context_lines":[{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(0))"},{"line_number":803,"context_line":""},{"line_number":804,"context_line":"        # Same result sending project_id parameter"},{"line_number":805,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":806,"context_line":"            [group1[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":807,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_af5f7f0f","line":804,"updated":"2014-08-13 15:13:50.000000000","message":"I don\u0027t think this is correct. Is this the same result?","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1874f7dc2872c18a1123384eb149bb305f74dc4c","unresolved":false,"context_lines":[{"line_number":801,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(0))"},{"line_number":803,"context_line":""},{"line_number":804,"context_line":"        # Same result sending project_id parameter"},{"line_number":805,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":806,"context_line":"            [group1[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":807,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"}],"source_content_type":"text/x-python","patch_set":6,"id":"7ac7a964_cab5a5ce","line":804,"in_reply_to":"baada198_af5f7f0f","updated":"2014-08-14 08:27:35.000000000","message":"Not, it is not the same result than line 800 when you use not LDAP backend, because if get_roles_for_groups function receives domain_id parameter, the query on SQL is set to AssignmentType.GROUP_DOMAIN, but the previous create_grant action is not associating domain. So, the result expected on line 800 is 0 and in line 807 is 1.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"796d47b142f0e1e153a429435abd716dc0d5f977","unresolved":false,"context_lines":[{"line_number":810,"context_line":"        # No results sending a different project_id"},{"line_number":811,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":812,"context_line":"            [group1[\u0027id\u0027]], self.tenant_baz[\u0027id\u0027])"},{"line_number":813,"context_line":"        self.assertThat(roles, matchers.HasLength(0))"},{"line_number":814,"context_line":""},{"line_number":815,"context_line":"    def test_remove_role_grant_from_user_and_project(self):"},{"line_number":816,"context_line":"        self.assignment_api.create_grant(user_id\u003dself.user_foo[\u0027id\u0027],"}],"source_content_type":"text/x-python","patch_set":6,"id":"baada198_e2d4a1bc","line":813,"updated":"2014-08-11 10:50:34.000000000","message":"I don\u0027t particularly dig the usage of the name \"group1\", since I would expect there to be a group2 and maybe even a group3. Would prefer just group or new_group. But anyway, it\u0027s no big deal for me if you don\u0027t change it.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"126019b40c8746743f04a73d3584dd6ba178ecf2","unresolved":false,"context_lines":[{"line_number":784,"context_line":"        self.assertIn(self.role_admin[\u0027id\u0027], roles_ref_ids)"},{"line_number":785,"context_line":"        self.assertIn(\u0027member\u0027, roles_ref_ids)"},{"line_number":786,"context_line":""},{"line_number":787,"context_line":"    def test_get_roles_for_groups(self):"},{"line_number":788,"context_line":"        domain_id \u003d CONF.identity.default_domain_id"},{"line_number":789,"context_line":""},{"line_number":790,"context_line":"        # Create new group"}],"source_content_type":"text/x-python","patch_set":7,"id":"7ac7a964_7fffea01","line":787,"updated":"2014-08-14 11:41:13.000000000","message":"I\u0027m just wandering if this test should actually be divided into several tests to be more self-documented. Since it seems like this might actually be two or three tests, only divided by comments. On the other hand, would it make sense to test this operation with more groups in the same domain as parameters and maybe even without groups?","commit_id":"ff90e0942c5a84cd85adf824d7c4c899746248cc"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"a61bfd77bc170ae9b8b72609d458b1ed4b1bbe71","unresolved":false,"context_lines":[{"line_number":784,"context_line":"        self.assertIn(self.role_admin[\u0027id\u0027], roles_ref_ids)"},{"line_number":785,"context_line":"        self.assertIn(\u0027member\u0027, roles_ref_ids)"},{"line_number":786,"context_line":""},{"line_number":787,"context_line":"    def test_get_roles_for_groups(self):"},{"line_number":788,"context_line":"        domain_id \u003d CONF.identity.default_domain_id"},{"line_number":789,"context_line":""},{"line_number":790,"context_line":"        # Create new group"}],"source_content_type":"text/x-python","patch_set":7,"id":"7ac7a964_24ab7e1e","line":787,"in_reply_to":"7ac7a964_7fffea01","updated":"2014-08-15 06:58:11.000000000","message":"I think this test is not very complicated, so I think it is not necessary to split it in more tests. There are several tests here in which they make more than one thing... I\u0027ve based on this \"style\" of tests to write test_get_roles_for_groups() function.\n\nAbout test with more groups. In this test, I\u0027m sending a group list (with only one group) so, the loop inside assignment_api.get_roles_for_groups works fine... I think it is not necessary","commit_id":"ff90e0942c5a84cd85adf824d7c4c899746248cc"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"126019b40c8746743f04a73d3584dd6ba178ecf2","unresolved":false,"context_lines":[{"line_number":801,"context_line":"            [new_group[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(0))"},{"line_number":803,"context_line":""},{"line_number":804,"context_line":"        # Same result sending project_id parameter"},{"line_number":805,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":806,"context_line":"            [new_group[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":807,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"}],"source_content_type":"text/x-python","patch_set":7,"id":"7ac7a964_84ee398a","line":804,"updated":"2014-08-14 11:41:13.000000000","message":"Same result as above? it still seems like now the function actually got roles using the tenant_id as a parameter, is this the intended behaviour? Anyway, it seems like it\u0027s not actually the same result and the comment should be changed.","commit_id":"ff90e0942c5a84cd85adf824d7c4c899746248cc"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"a024e9af68381a9a94fb3f3e7d4986b027c10f23","unresolved":false,"context_lines":[{"line_number":801,"context_line":"            [new_group[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":802,"context_line":"        self.assertThat(roles, matchers.HasLength(0))"},{"line_number":803,"context_line":""},{"line_number":804,"context_line":"        # Same result sending project_id parameter"},{"line_number":805,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":806,"context_line":"            [new_group[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":807,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"}],"source_content_type":"text/x-python","patch_set":7,"id":"7ac7a964_1a37e4e4","line":804,"in_reply_to":"7ac7a964_84ee398a","updated":"2014-08-14 12:00:47.000000000","message":"Yes, it is old comment. I\u0027ll change it.","commit_id":"ff90e0942c5a84cd85adf824d7c4c899746248cc"},{"author":{"_account_id":10873,"name":"Juan Antonio Osorio Robles","email":"jaosorior@redhat.com","username":"ejuaoso"},"change_message_id":"126019b40c8746743f04a73d3584dd6ba178ecf2","unresolved":false,"context_lines":[{"line_number":803,"context_line":""},{"line_number":804,"context_line":"        # Same result sending project_id parameter"},{"line_number":805,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":806,"context_line":"            [new_group[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":807,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":808,"context_line":"        self.assertIn(self.role_member, roles)"},{"line_number":809,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"7ac7a964_047789d5","line":806,"updated":"2014-08-14 11:41:13.000000000","message":"maybe it would be better if you follow the same pattern as above, as to specifying the name of the parameter that is being passed. Something like:\n\nroles \u003d self.assignment_api.get_roles_for_groups(\n    [new_group[\u0027id\u0027]], project_id\u003dself.tenant_bar[\u0027id\u0027])","commit_id":"ff90e0942c5a84cd85adf824d7c4c899746248cc"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"a024e9af68381a9a94fb3f3e7d4986b027c10f23","unresolved":false,"context_lines":[{"line_number":803,"context_line":""},{"line_number":804,"context_line":"        # Same result sending project_id parameter"},{"line_number":805,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":806,"context_line":"            [new_group[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":807,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":808,"context_line":"        self.assertIn(self.role_member, roles)"},{"line_number":809,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"7ac7a964_1a3e04b9","line":806,"in_reply_to":"7ac7a964_047789d5","updated":"2014-08-14 12:00:47.000000000","message":"+1","commit_id":"ff90e0942c5a84cd85adf824d7c4c899746248cc"}],"keystone/tests/test_backend_ldap.py":[{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"1198db2fbf44c7eea2658bf5b90701e3d1e1114b","unresolved":false,"context_lines":[{"line_number":1453,"context_line":"        domain_ref \u003d self.assignment_api.get_domain_by_name(domain[\u0027name\u0027])"},{"line_number":1454,"context_line":"        self.assertEqual(domain_ref, domain)"},{"line_number":1455,"context_line":""},{"line_number":1456,"context_line":"    def test_get_roles_for_groups(self):"},{"line_number":1457,"context_line":"        domain \u003d self._get_domain_fixture()"},{"line_number":1458,"context_line":""},{"line_number":1459,"context_line":"        # Create new group"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_ca5f6571","line":1456,"updated":"2014-06-30 22:17:43.000000000","message":"aren\u0027t there tests for these in test_backend already?","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"eb43caaf1ceb595650296f05c5009eccb77d89a4","unresolved":false,"context_lines":[{"line_number":1453,"context_line":"        domain_ref \u003d self.assignment_api.get_domain_by_name(domain[\u0027name\u0027])"},{"line_number":1454,"context_line":"        self.assertEqual(domain_ref, domain)"},{"line_number":1455,"context_line":""},{"line_number":1456,"context_line":"    def test_get_roles_for_groups(self):"},{"line_number":1457,"context_line":"        domain \u003d self._get_domain_fixture()"},{"line_number":1458,"context_line":""},{"line_number":1459,"context_line":"        # Create new group"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_6c8471d4","line":1456,"in_reply_to":"baada198_1a0a678a","updated":"2014-07-02 12:44:10.000000000","message":"David, I think it is, but get_roles_for_groups() function is not implemented yet on KVS https://github.com/openstack/keystone/blob/master/keystone/assignment/backends/kvs.py#L161\n\nSo, if we \"skip\" this test (until KVS implementation is done) on test_backend_kvs.py, I think there are no problems to move these tests to test_backend.py.\n\nAre you agree?","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"6116f1d88061d8c8bda6ad4f3407f3972c2cd78f","unresolved":false,"context_lines":[{"line_number":1453,"context_line":"        domain_ref \u003d self.assignment_api.get_domain_by_name(domain[\u0027name\u0027])"},{"line_number":1454,"context_line":"        self.assertEqual(domain_ref, domain)"},{"line_number":1455,"context_line":""},{"line_number":1456,"context_line":"    def test_get_roles_for_groups(self):"},{"line_number":1457,"context_line":"        domain \u003d self._get_domain_fixture()"},{"line_number":1458,"context_line":""},{"line_number":1459,"context_line":"        # Create new group"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_1a0a678a","line":1456,"in_reply_to":"baada198_677e87b0","updated":"2014-07-01 16:55:40.000000000","message":"That\u0027s unfortunate.  Is it possible to repurpose these tests into test_backend so that all backends get tested?","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1142f21625ac5ff2d08ae6d3159032e804bc00d6","unresolved":false,"context_lines":[{"line_number":1453,"context_line":"        domain_ref \u003d self.assignment_api.get_domain_by_name(domain[\u0027name\u0027])"},{"line_number":1454,"context_line":"        self.assertEqual(domain_ref, domain)"},{"line_number":1455,"context_line":""},{"line_number":1456,"context_line":"    def test_get_roles_for_groups(self):"},{"line_number":1457,"context_line":"        domain \u003d self._get_domain_fixture()"},{"line_number":1458,"context_line":""},{"line_number":1459,"context_line":"        # Create new group"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_677e87b0","line":1456,"in_reply_to":"baada198_ca5f6571","updated":"2014-07-01 06:40:49.000000000","message":"I don\u0027t think so Brant, I\u0027ve searched https://github.com/openstack/keystone/search?q\u003dget_roles_for_groups\u0026ref\u003dcmdform and I didn\u0027t find previous tests for get_roles_for_groups function.","commit_id":"ea60521372882e1ff148a15b4f0075ef70e41765"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"9cefbee4eece677e6f27dba282f738450121505a","unresolved":false,"context_lines":[{"line_number":1527,"context_line":""},{"line_number":1528,"context_line":"        # Same result sending project_id parameter"},{"line_number":1529,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":1530,"context_line":"            [group1[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":1531,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":1532,"context_line":"        self.assertIn(self.role_member, roles)"},{"line_number":1533,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_5d0b9d2a","line":1530,"updated":"2014-07-24 21:01:42.000000000","message":"i\u0027d like henrynash to review this - i don\u0027t think the new sql assignment driver could satisfy this call.\n\non a related note, this test should be moved to the parent class of this test class (IdentityTests, IIRC) so that it applies equally to SQL, LDAP, and KVS... and then a skip method may be added to the KVS test suite. that would ensure the test works against both SQL and LDAP (which I think it should).","commit_id":"488197a89b850edd6bfa665e39811825923aca0e"},{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"599794d888323d2265ccea4c87ca15faa4b2fe4f","unresolved":false,"context_lines":[{"line_number":1527,"context_line":""},{"line_number":1528,"context_line":"        # Same result sending project_id parameter"},{"line_number":1529,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":1530,"context_line":"            [group1[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":1531,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":1532,"context_line":"        self.assertIn(self.role_member, roles)"},{"line_number":1533,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_eebaf9be","line":1530,"in_reply_to":"baada198_5d0b9d2a","updated":"2014-07-24 22:16:39.000000000","message":"So the new sql assignment driver does already provide this call, so I think we are good there.\n\nAgree that this test should go into test_backend.py....and with a skip in the kvs backend tests, since kvs will now be the only one that does not supper it.","commit_id":"488197a89b850edd6bfa665e39811825923aca0e"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"2239a1e5f8a43eda288e73dc08c22189717c5dd5","unresolved":false,"context_lines":[{"line_number":1527,"context_line":""},{"line_number":1528,"context_line":"        # Same result sending project_id parameter"},{"line_number":1529,"context_line":"        roles \u003d self.assignment_api.get_roles_for_groups("},{"line_number":1530,"context_line":"            [group1[\u0027id\u0027]], self.tenant_bar[\u0027id\u0027])"},{"line_number":1531,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":1532,"context_line":"        self.assertIn(self.role_member, roles)"},{"line_number":1533,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_447d7ae6","line":1530,"in_reply_to":"baada198_eebaf9be","updated":"2014-07-25 08:22:53.000000000","message":"Thank you for your review Dolph and Henry, I\u0027ll do it.","commit_id":"488197a89b850edd6bfa665e39811825923aca0e"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"ff32f9dee46aecdc35a4dd984e5ed69a4fb5a0dd","unresolved":false,"context_lines":[{"line_number":1573,"context_line":"        list_group_ids \u003d [group1[\u0027id\u0027], group2[\u0027id\u0027]]"},{"line_number":1574,"context_line":"        domains \u003d self.assignment_api.list_domains_for_groups(list_group_ids)"},{"line_number":1575,"context_line":""},{"line_number":1576,"context_line":"        self.assertThat(domains, matchers.HasLength(1))"},{"line_number":1577,"context_line":""},{"line_number":1578,"context_line":"    def test_list_groups_for_user_no_dn(self):"},{"line_number":1579,"context_line":"        # Create a test user."}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_fd60f1a5","line":1576,"updated":"2014-08-08 16:10:23.000000000","message":"Instead of just checking for the lengths of the returned lists it would be nice to see if the value was what you expected. This goes for all of the tests below.","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"df111f8a6dcccb60cdc95339b06010f626cfeb65","unresolved":false,"context_lines":[{"line_number":1573,"context_line":"        list_group_ids \u003d [group1[\u0027id\u0027], group2[\u0027id\u0027]]"},{"line_number":1574,"context_line":"        domains \u003d self.assignment_api.list_domains_for_groups(list_group_ids)"},{"line_number":1575,"context_line":""},{"line_number":1576,"context_line":"        self.assertThat(domains, matchers.HasLength(1))"},{"line_number":1577,"context_line":""},{"line_number":1578,"context_line":"    def test_list_groups_for_user_no_dn(self):"},{"line_number":1579,"context_line":"        # Create a test user."}],"source_content_type":"text/x-python","patch_set":5,"id":"baada198_16fc7bb5","line":1576,"in_reply_to":"baada198_fd60f1a5","updated":"2014-08-11 09:08:21.000000000","message":"Done","commit_id":"39048655c6a206f000e50d24ddd39f6116367dd1"},{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"10a72990f05eb8a9714186b66ba51109df6834b8","unresolved":false,"context_lines":[{"line_number":1561,"context_line":"            self.assertNotIn(\u0027dn\u0027, group_ref)"},{"line_number":1562,"context_line":"        self.assertEqual(set(expected_group_ids), group_ids)"},{"line_number":1563,"context_line":""},{"line_number":1564,"context_line":"    def test_list_domains_for_groups(self):"},{"line_number":1565,"context_line":"        domain \u003d self._get_domain_fixture()"},{"line_number":1566,"context_line":""},{"line_number":1567,"context_line":"        group1 \u003d {\u0027name\u0027: uuid.uuid4().hex, \u0027domain_id\u0027: domain[\u0027id\u0027]}"}],"source_content_type":"text/x-python","patch_set":6,"id":"9ab29df4_401d0473","line":1564,"updated":"2014-08-13 20:11:42.000000000","message":"So this should really be tested at the test_backend level and over-ridden here (due to LDAPs mono-domain implementation).  However,I would agree that implmenting this in test_backend is out of scope of this patch - but perhaps you could raise a defect to cover that this should be done in the future.","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"10a72990f05eb8a9714186b66ba51109df6834b8","unresolved":false,"context_lines":[{"line_number":1600,"context_line":"            self.assertNotIn(\u0027dn\u0027, group_ref)"},{"line_number":1601,"context_line":"        self.assertEqual(set(expected_group_ids), group_ids)"},{"line_number":1602,"context_line":""},{"line_number":1603,"context_line":"    def test_list_projects_for_groups(self):"},{"line_number":1604,"context_line":"        domain \u003d self._get_domain_fixture()"},{"line_number":1605,"context_line":""},{"line_number":1606,"context_line":"        # Create new group"}],"source_content_type":"text/x-python","patch_set":6,"id":"9ab29df4_60120864","line":1603,"updated":"2014-08-13 20:11:42.000000000","message":"as above","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1874f7dc2872c18a1123384eb149bb305f74dc4c","unresolved":false,"context_lines":[{"line_number":1600,"context_line":"            self.assertNotIn(\u0027dn\u0027, group_ref)"},{"line_number":1601,"context_line":"        self.assertEqual(set(expected_group_ids), group_ids)"},{"line_number":1602,"context_line":""},{"line_number":1603,"context_line":"    def test_list_projects_for_groups(self):"},{"line_number":1604,"context_line":"        domain \u003d self._get_domain_fixture()"},{"line_number":1605,"context_line":""},{"line_number":1606,"context_line":"        # Create new group"}],"source_content_type":"text/x-python","patch_set":6,"id":"7ac7a964_cd3a1f36","line":1603,"in_reply_to":"9ab29df4_60120864","updated":"2014-08-14 08:27:35.000000000","message":"Ok Henry, I\u0027ll try to move test_list_domains_for_groups and test_list_projects_for_groups to test_backend.py","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"649e379739198655a9fd8256002d9198f1cd3b09","unresolved":false,"context_lines":[{"line_number":1635,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":1636,"context_line":"        # NOTE(marcos-fermin-lobo): LDAPIdentity has different"},{"line_number":1637,"context_line":"        #                           behavior here than IdentityTests on"},{"line_number":1638,"context_line":"        #                           test_backend.py"},{"line_number":1639,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":1640,"context_line":"        self.assertEqual(self.role_member[\u0027id\u0027], roles[0][\u0027id\u0027])"},{"line_number":1641,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"9ab29df4_eb3359f4","line":1638,"updated":"2014-08-13 20:12:39.000000000","message":"i must admit, I still don\u0027t understand why the result should be different...","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"},{"author":{"_account_id":8533,"name":"Marcos Fermín Lobo","email":"lobo@lukos.org","username":"mlobo"},"change_message_id":"1874f7dc2872c18a1123384eb149bb305f74dc4c","unresolved":false,"context_lines":[{"line_number":1635,"context_line":"            [group1[\u0027id\u0027]], domain_id\u003ddomain_id)"},{"line_number":1636,"context_line":"        # NOTE(marcos-fermin-lobo): LDAPIdentity has different"},{"line_number":1637,"context_line":"        #                           behavior here than IdentityTests on"},{"line_number":1638,"context_line":"        #                           test_backend.py"},{"line_number":1639,"context_line":"        self.assertThat(roles, matchers.HasLength(1))"},{"line_number":1640,"context_line":"        self.assertEqual(self.role_member[\u0027id\u0027], roles[0][\u0027id\u0027])"},{"line_number":1641,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"7ac7a964_8d7e67e3","line":1638,"in_reply_to":"9ab29df4_eb3359f4","updated":"2014-08-14 08:27:35.000000000","message":"I think the reason is how SQL makes the query https://github.com/openstack/keystone/blob/master/keystone/assignment/backends/sql.py#L292","commit_id":"8ddbdd07b2e193462a54b00f5b7fa218259e8ba7"}]}