- Notifications
You must be signed in to change notification settings - Fork673
feat: add feature to get inherited member for project/group#1187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice, I really like this. It helps remove the confusion around havingall() methods from/all endpoints,all=True for pagination, and"all": True in query params where it's an attribute GitLab itself accepts. With the CLI this has caused a lot of confusion.
This solution is also very similar to@chrisoldwood's proposal in#593.
I just have some comments on keeping backwards compatibility and naming, if you can take a look, because this can probably later be reused in some other places (maybe for runners as well) :)
| @@ -312,7 +312,7 @@ | |||
| group1.members.delete(user1.id) | |||
| assert len(group1.members.list()) == 2 | |||
| assert len(group1.members.all()) | |||
| assert len(group1.members_all.list()) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| assertlen(group1.members_all.list()) | |
| assertlen(group1.members.all())# Deprecated | |
| assertlen(group1.members_all.list()) |
As you can see in the test you've had to change in50f4b9c, this would break the existing behavior for anyone who has relied on.all() so far. I think it might be better to preserve the old behavior with aDeprecationWarning and remove it later with a major release.
Maybe that can be done (temporarily) with aListAllMixin that contains the method, to avoid copy/pasting. Just need to make sure toregister_custom_action for both Project and Group members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I rolled back the test casegroup1.members.all() and addedMemberAllMixin withregister_custom_action andDeprecationWarning ,ListAllMixin sounds very general, we won't useListAllMixin somewhere I guess.
| @@ -4595,6 +4558,7 @@ class Project(SaveMixin, ObjectDeleteMixin, RESTObject): | |||
| ("issues", "ProjectIssueManager"), | |||
| ("labels", "ProjectLabelManager"), | |||
| ("members", "ProjectMemberManager"), | |||
| ("members_all", "ProjectMemberAllManager"), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For me the dilemma here is:members_all/ProjectMemberAllManager orall_members/ProjectAllMemberManager 😁 WDYT@max-wittig?all_members (orallmembers) sounds more natural to me. On the other handmembers_all does follow the URL segments.
Just asking because this pattern can be a precedent for improving other/all endpoints (e.g.#593) so it might have further reach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I spent a few minutes scanning the GitLab API and looks GitLab doesn't tend to make its addresses natural. Anyway, I am not a python Pro, final conclusion is up to you.
Uh oh!
There was an error while loading.Please reload this page.
Agree, I thought about backward compatibility right after made a PR. But decided to wait for a review, because in any way, there were more questions than answers. In the git log, I found RELEASE_NOTES.rst, where such things like mine need to be reflected. Who will manage that, I? |
But why is this needed? We already have a few places, where we use |
For me the It's weird that GitLab added these endpoints separately, but I guess they might add them to other resources. The other place where I've found it used is with runners and there is also a similar issue open for that. Visually it's also a bit confusing, you need to do |
Hi! I think we're going to have to agree to disagree. |
Hello, Was just about to implement this when I found the PR. Is this still under consideration? |
Yes this is still relevant IMO so people can use the endpoint properly for individual users with |
Hi!
This PR about feature#1133. Here I propose to introduce new custom managers ProjectMemberAllManager (/projects/:id/members/all/:user_id) and GroupMemberAllManager (/groups/:id/members/all/:user_id). Subsequently, in order to fetch a list or instance of members including inherited ones, you will need to:
I had thought about
group.members.all(id=group_id)also. But then changed my mind.If the approach is right?