)]}'
{"keystone/assignment/controllers.py":[{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"66aaff23e967ca93a23238ee113c11d61ac5ea0a","unresolved":false,"context_lines":[{"line_number":335,"context_line":"        else:"},{"line_number":336,"context_line":"            return self.list_roles(request)"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":"    @controller.filterprotected(\u0027name\u0027, \u0027domain_id\u0027)"},{"line_number":339,"context_line":"    def list_roles(self, request, filters):"},{"line_number":340,"context_line":"        return self._list_roles(request, filters)"},{"line_number":341,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1aa78d24_59a2379a","line":338,"range":{"start_line":338,"start_character":40,"end_line":338,"end_character":51},"updated":"2016-07-06 14:30:05.000000000","message":"As an aside, I think this can be removed (since by definition domain_id is None for global roles) - but this change is outside the scope of this patch","commit_id":"1f02564b210a83221492e90cca3bb626a9eb60ea"},{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"5325d9aec86b51a2fc497a9072f2c7d42507a06f","unresolved":false,"context_lines":[{"line_number":429,"context_line":"    def build_driver_hints(cls, request, supported_filters):"},{"line_number":430,"context_line":"        # NOTE(jamielennox): If there is no domain_id filter defined, then we"},{"line_number":431,"context_line":"        # only want to return global roles, so we set the domain_id filter to"},{"line_number":432,"context_line":"        # None. For some reason this doesn\u0027t happen by default so we have to"},{"line_number":433,"context_line":"        # override it."},{"line_number":434,"context_line":"        hints \u003d super(RoleV3, cls).build_driver_hints(request,"},{"line_number":435,"context_line":"                                                      supported_filters)"},{"line_number":436,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1aa78d24_aad34d37","line":433,"range":{"start_line":432,"start_character":16,"end_line":433,"end_character":22},"updated":"2016-07-05 09:26:57.000000000","message":"This is because there is a semantic difference between specifying domain_id\u003dNone (which means I want all global roles), vs not specifying a filter at all (which would mean I want all roles). For now, we do not provide an ability for the caller to semantically ask for all roles, so as to avoid confusion after upgrading to a version of keystone that started supporting domain specific roles. I would either update the comment to say this, and/or remove the \"for some reason\" line","commit_id":"1f02564b210a83221492e90cca3bb626a9eb60ea"},{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"47e7e55bc22b23748c014ce4a9f24b62dbee896e","unresolved":false,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":"    @classmethod"},{"line_number":429,"context_line":"    def build_driver_hints(cls, request, supported_filters):"},{"line_number":430,"context_line":"        # NOTE(jamielennox): To handle the default case of no domain_id defined"},{"line_number":431,"context_line":"        # the role_assignment backend does some hackery to distinguish between"},{"line_number":432,"context_line":"        # global and domain scoped roles. This backend behaviour relies upon a"},{"line_number":433,"context_line":"        # value of domain_id being set (not just defaulting to None). Manually"},{"line_number":434,"context_line":"        # set the empty filter if its not provided."},{"line_number":435,"context_line":""},{"line_number":436,"context_line":"        hints \u003d super(RoleV3, cls).build_driver_hints(request,"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_28d6cd4e","line":433,"range":{"start_line":430,"start_character":10,"end_line":433,"end_character":69},"updated":"2016-07-07 07:46:43.000000000","message":"so I\u0027m OK with this...and maybe I\u0027m being dumb at missing why you think the backend is doing something unusual here...I don\u0027t believe it is:\n\nThe domain_id attribute in the role entity is defined (at the manager level) as:\n\nNone - means this is a global role\n\u003cvalid domain_id\u003e - means this is a domain specific role for the named domain\n\nLike any other attribute filter, if you don\u0027t define it then you expect all entities to be returned regardless of their value of that attribute. If the attribute is defined in the filter, you expect a collection of entities that match that attribute value. \"None\" is just a supported attribute value, like any other.\n\nThe subtlety is that in the API today (for backward compatibility reasons) we don\u0027t want to allow you to fetch all roles, regardless of whether they are global or not - hence this special handling of the domain_id setting is in the controller (since that\u0027s where an API restriction like this should be handled).","commit_id":"3dd17505d01897e8fca503595a1301e47959b858"},{"author":{"_account_id":7191,"name":"Jamie Lennox","email":"jamielennox@gmail.com","username":"jamielennox"},"change_message_id":"f2271875a5fd4b3607de69f823bf6f9b836ee6d1","unresolved":false,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":"    @classmethod"},{"line_number":429,"context_line":"    def build_driver_hints(cls, request, supported_filters):"},{"line_number":430,"context_line":"        # NOTE(jamielennox): To handle the default case of no domain_id defined"},{"line_number":431,"context_line":"        # the role_assignment backend does some hackery to distinguish between"},{"line_number":432,"context_line":"        # global and domain scoped roles. This backend behaviour relies upon a"},{"line_number":433,"context_line":"        # value of domain_id being set (not just defaulting to None). Manually"},{"line_number":434,"context_line":"        # set the empty filter if its not provided."},{"line_number":435,"context_line":""},{"line_number":436,"context_line":"        hints \u003d super(RoleV3, cls).build_driver_hints(request,"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_540e508c","line":433,"range":{"start_line":430,"start_character":10,"end_line":433,"end_character":69},"in_reply_to":"1aa78d24_28d6cd4e","updated":"2016-07-07 09:13:56.000000000","message":"The thing i see is that if we always set this, then there\u0027s no option for the user to do anything else. The backend won\u0027t know the difference here so it would be just a default.\n\nAnyway, fixing that isn\u0027t part of this review i just wanted to make sure i understood what was going on for the refactor.","commit_id":"3dd17505d01897e8fca503595a1301e47959b858"}],"keystone/common/controller.py":[{"author":{"_account_id":5707,"name":"Henry Nash","email":"henryn@linux.vnet.ibm.com","username":"henry-nash"},"change_message_id":"5325d9aec86b51a2fc497a9072f2c7d42507a06f","unresolved":false,"context_lines":[{"line_number":633,"context_line":"    def build_driver_hints(cls, request, supported_filters):"},{"line_number":634,"context_line":"        \"\"\"Build list hints based on the context query string."},{"line_number":635,"context_line":""},{"line_number":636,"context_line":"        :param request: contains the current request that has the params that"},{"line_number":637,"context_line":"                        list hints can be extracted"},{"line_number":638,"context_line":"        :param supported_filters: list of filters supported, so ignore any"},{"line_number":639,"context_line":"                                  keys in query_dict that are not in this list."}],"source_content_type":"text/x-python","patch_set":2,"id":"1aa78d24_03c28233","line":636,"range":{"start_line":636,"start_character":73,"end_line":636,"end_character":77},"updated":"2016-07-05 09:26:57.000000000","message":"nit: that vs from which? My english language was never that good, but it sounds better (to me) the way it was.....","commit_id":"1f02564b210a83221492e90cca3bb626a9eb60ea"}]}