Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add User.groups and Group.members #341

Merged
merged 17 commits into from
Dec 5, 2024
Merged

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Nov 18, 2024

Fixes #340
Related https://github.com/rstudio/connect/issues/28176

Added

  • Group
    • Group.members.find() - Retrieve all users for a given group
    • Group.members.add(user) - Add user to a group
    • Group.members.delete(user) - Remove user form a group
  • User
    • Users.groups.find() - Retrieve all groups for a given user
    • User.groups.add(group) - Add user to a group
    • User.groups.delete(group) - Remove user from a group

Other changes

  • Context class added self.ctx = weakref.proxy(client)
    • A weakref was used as Client -> Context -> Client circular dependency is made quickly. By having the Context -> Client be weak, then the Context will only disappear once the parent Client wants to be gc'd.
  • Permissions
    • .destroy(*permissions) was changed to .destroy(permission) to follow pattern of how a single user can be added to a single group. Similarly, destroying multiple permissions in one method is no faster through the API than looping through and destorying single permissions individually.

@schloerke schloerke added enhancement New feature or request sdk Used for automation labels Nov 18, 2024
@schloerke schloerke self-assigned this Nov 18, 2024
@schloerke schloerke force-pushed the schloerke/340-user-groups branch from b59c689 to 6651301 Compare November 18, 2024 22:01
Copy link

github-actions bot commented Nov 18, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
1875 1735 93% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/posit/connect/client.py 99% 🟢
src/posit/connect/content.py 96% 🟢
src/posit/connect/context.py 92% 🟢
src/posit/connect/groups.py 77% 🟢
src/posit/connect/me.py 100% 🟢
src/posit/connect/permissions.py 96% 🟢
src/posit/connect/users.py 94% 🟢
TOTAL 93% 🟢

updated for commit: 8bcbdb7 by action🐍

@schloerke schloerke marked this pull request as ready for review November 18, 2024 22:06
@schloerke schloerke requested a review from tdstein as a code owner November 18, 2024 22:06
@schloerke schloerke requested a review from jonkeane November 20, 2024 16:03
Copy link
Collaborator

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this

I understand that circular reference/dependency is an issue, but could you say more about how/why this particular addition makes us bump into it?

src/posit/connect/groups.py Show resolved Hide resolved
src/posit/connect/groups.py Outdated Show resolved Hide resolved
src/posit/connect/users.py Outdated Show resolved Hide resolved
@schloerke
Copy link
Collaborator Author

I understand that circular reference/dependency is an issue, but could you say more about how/why this particular addition makes us bump into it?

Up until now, functions have been fairly isolated. However, gathering all of the groups a User needs requires access to the original client context ._ctx.client. The group information can not be found through a User's currently available knowledge.

To get around this, I added the client to the Context class. This Context class the the new form of params (of type ParameterResources) that Taylor has been transitioning to.

The problem comes in where a client (Client) has a ._ctx (Context) object who then points back to the same client (Client) class object. This circular reference delayed garbage collecting on the del client test being performed causing the test to fail. When debugging, I learned that the .__del__() methods are not run until garbage collection, not immediately when del obj is called. Once the reference counter goes to 0, then the .__del__() method is called. (Reference)

When the child Context.client was a weak reference, the value would not be garbage collected too early.

We could adjust the del client test and remove the weak reference. It should work, but I believe memory would be retained until the execution context is finished.

src/posit/connect/groups.py Show resolved Hide resolved
src/posit/connect/users.py Show resolved Hide resolved
src/posit/connect/content.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@toph-allen toph-allen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional changes look good

@schloerke schloerke marked this pull request as draft December 2, 2024 18:58
@schloerke schloerke marked this pull request as ready for review December 2, 2024 20:44
@schloerke schloerke requested a review from toph-allen December 2, 2024 20:44
@schloerke schloerke marked this pull request as draft December 4, 2024 19:54
@schloerke
Copy link
Collaborator Author

schloerke commented Dec 4, 2024

Following comment from other PR. Am going to only allow for add / delete to accept a single user/group object. This makes is very clear as to what is happening.

@schloerke schloerke marked this pull request as ready for review December 5, 2024 17:49
@schloerke schloerke merged commit 918bd5f into main Dec 5, 2024
35 checks passed
@schloerke schloerke deleted the schloerke/340-user-groups branch December 5, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sdk Used for automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Find groups from user" recipe
3 participants