feat: Extend identity route to list local users #458
Conversation
gtema
left a comment
gtema
left a comment
There was a problem hiding this comment.
no, this is not the way how the provider should be implemented. You can extend the existing UserListParameters with the dedicated user type and pass it down to the regular listing function. Otherwise the provider interface is simply going to explode.
Whether on the API level it make sense to expose it as a separate endpoint is a different question (which I am not able to answer as of now). For sure such new API will not go under the /v3
konac-hamza
force-pushed
the
feature/extend-identity-api
branch
from
332d931 to
d75b9c7
Compare
Could you review implemented |
gtema
left a comment
gtema
left a comment
There was a problem hiding this comment.
Problem with adding this in v3 is that for the user/client there is absolutely no way to figure out whether it is supported or not. When request goes to python keystone it will silently ignore it.
There was a problem hiding this comment.
this is fine in the concept, but we are not going to expose this in the api v3 (so far). I suggest first you focus only on the backend/provider part. In the separate PR you can then start relying on that, but only in the v4 api (or at least we can have a separate discussion in the dedicated PR)
There was a problem hiding this comment.
please remove this part of the change - no api changes for now
konac-hamza
force-pushed
the
feature/extend-identity-api
branch
from
d75b9c7 to
8477676
Compare
There was a problem hiding this comment.
please remove this part of the change - no api changes for now
There was a problem hiding this comment.
let's not duplicate the logic. This is something what we can integrate into the main listing method
| use crate::identity::types::*; | ||
|
|
||
| /// Prepare the paginated query for listing local users. | ||
| fn get_list_query( |
There was a problem hiding this comment.
I strongly discourage from adding this in this change - you make the change bigger and harder to review
src/identity/types/user.rs
Outdated
| #[serde(default, rename = "type")] | ||
| pub user_type: Option<UserType>, | ||
|
|
||
| /// Page marker. |
There was a problem hiding this comment.
let's not add anything about the pagination in the scope of this change
konac-hamza
force-pushed
the
feature/extend-identity-api
branch
from
34bd711 to
4a434aa
Compare
2 participants
No description provided.