feat: add feature to get inherited member for project/group #1187

Shkurupii · 2020-09-12T09:54:28Z

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:

project = gl.projects.get(project_id)
project.members_all.list()
project.members_all.get(member_id)

group = gl.groups.get(group_id)
group.members_all.list()
group.members_all.get(member_id)

I had thought about group.members.all(id=group_id) also. But then changed my mind.
If the approach is right?

nejch

Nice, I really like this. It helps remove the confusion around having all() 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) :)

nejch · 2020-09-13T16:03:45Z

tools/python_test_v4.py

@@ -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())


Suggested change

assert len(group1.members_all.list())

assert len(group1.members.all()) # Deprecated

assert len(group1.members_all.list())

As you can see in the test you've had to change in 50f4b9c, 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 a DeprecationWarning and remove it later with a major release.

Maybe that can be done (temporarily) with a ListAllMixin that contains the method, to avoid copy/pasting. Just need to make sure to register_custom_action for both Project and Group members.

I rolled back the test case group1.members.all() and added MemberAllMixin with register_custom_action and DeprecationWarning , ListAllMixin sounds very general, we won't use ListAllMixin somewhere I guess.

nejch · 2020-09-13T16:44:13Z

gitlab/v4/objects/__init__.py

@@ -4595,6 +4558,7 @@ class Project(SaveMixin, ObjectDeleteMixin, RESTObject):
        ("issues", "ProjectIssueManager"),
        ("labels", "ProjectLabelManager"),
        ("members", "ProjectMemberManager"),
+        ("members_all", "ProjectMemberAllManager"),


For me the dilemma here is: members_all/ProjectMemberAllManager or all_members/ProjectAllMemberManager 😁 WDYT @max-wittig? all_members (or allmembers) sounds more natural to me. On the other hand members_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.

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.

gitlab/v4/objects/__init__.py

…herited" This reverts commit 50f4b9c

Shkurupii · 2020-09-14T12:58:53Z

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?

max-wittig · 2020-09-14T13:50:29Z

But why is this needed? We already have a few places, where we use all()

nejch · 2020-09-14T14:16:17Z

But why is this needed? We already have a few places, where we use all()

For me the all() method is problematic, because it means you can't implement the GET /resource/all/<resource-id> endpoints cleanly (at least from what I've seen - e.g. this endpoint https://docs.gitlab.com/ee/api/members.html#get-a-member-of-a-group-or-project-including-inherited-members), as this can currently only return lists. Maybe by adding the ID param to it but members.all(id=123) also seems inconsistent with the list()/get() from other managers?

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 gitlab project-member all --all/project.members.all(all=True) to combine pagination and this list 😁 and then gitlab somtimes also accepts an all param 🤦

Shkurupii · 2020-09-16T16:18:14Z

Hi! I think we're going to have to agree to disagree.
@max-wittig, how you would see the implementation of /projects/:id/members/all/:user_id?

JacobHenner · 2021-03-11T17:07:40Z

Hello,

Was just about to implement this when I found the PR. Is this still under consideration?

nejch · 2021-03-14T18:22:37Z

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 get(). This PR just needs to be rebased first because we had a lot of refactoring recently, @Shkurupii let us know if you need help or if we should rebase

Oleksii Shkurupii added 2 commits September 12, 2020 12:30

feat: add feature to get inherited member for project/group

1b62c2e

test: update test which lists all members including inherited

50f4b9c

nejch requested changes Sep 13, 2020

View reviewed changes

Oleksii Shkurupii added 3 commits September 14, 2020 13:42

chore: revert "test: update test which lists all members including in…

565151a

…herited" This reverts commit 50f4b9c

feat: add feature to get inherited member for project/group

55cdad8

test: add one more test which lists all members including inherited

323c279

Shkurupii closed this Mar 15, 2021

Shkurupii deleted the feat-get-inherited-members branch March 15, 2021 16:30

Shkurupii mentioned this pull request Mar 15, 2021

feat: add feature to get inherited member for project/group #1376

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add feature to get inherited member for project/group #1187

feat: add feature to get inherited member for project/group #1187

Uh oh!

Shkurupii commented Sep 12, 2020

Uh oh!

nejch left a comment •

edited

Loading

Uh oh!

nejch Sep 13, 2020

Uh oh!

Shkurupii Sep 14, 2020

Uh oh!

nejch Sep 13, 2020

Uh oh!

Shkurupii Sep 14, 2020

Uh oh!

Uh oh!

Shkurupii commented Sep 14, 2020

Uh oh!

max-wittig commented Sep 14, 2020

Uh oh!

nejch commented Sep 14, 2020

Uh oh!

Shkurupii commented Sep 16, 2020

Uh oh!

JacobHenner commented Mar 11, 2021

Uh oh!

nejch commented Mar 14, 2021

Uh oh!

Uh oh!

	assert len(group1.members_all.list())
	assert len(group1.members.all()) # Deprecated
	assert len(group1.members_all.list())

feat: add feature to get inherited member for project/group #1187

feat: add feature to get inherited member for project/group #1187

Uh oh!

Conversation

Shkurupii commented Sep 12, 2020

Uh oh!

nejch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nejch Sep 13, 2020

Choose a reason for hiding this comment

Uh oh!

Shkurupii Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

nejch Sep 13, 2020

Choose a reason for hiding this comment

Uh oh!

Shkurupii Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Shkurupii commented Sep 14, 2020

Uh oh!

max-wittig commented Sep 14, 2020

Uh oh!

nejch commented Sep 14, 2020

Uh oh!

Shkurupii commented Sep 16, 2020

Uh oh!

JacobHenner commented Mar 11, 2021

Uh oh!

nejch commented Mar 14, 2021

Uh oh!

Uh oh!

nejch left a comment •

edited

Loading