-
Notifications
You must be signed in to change notification settings - Fork 4
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 Permissions.delete(*permissions)
method
#339
base: main
Are you sure you want to change the base?
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
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.
Thanks for this, a few comments for you.
Would you mind adding more detail (can be only in the comments of the PR) about why we need to deprecate delete
and transition to destroy
for this? I am not necessarily opposed, but from the code itself it doesn't seem to be required and it would be good to have a papertrail of why we did that.
# aron, bill, cole, and me (and existing user) | ||
assert self.client.users.count() == 3 + self.existing_users |
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.
Could you say more about why 3 + self.existing_users
is preferred to the hard code?
It feels a little off that this blends something from setup with a test here: if count()
were to not work at all, that would make the setup always fail. And if we wanted to test that client.users.count(); client.users.create(); client.users.count()
increments like we expect, that should be in a test block, right?
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.
In the test file above (integration/tests/posit/connect/test_content_item_permissions.py), I want to use a couple users to test the permissions. So I added them there to have it be self contained. (We could move the users to an earlier location, but enforcing that the users exist for any subset of testing seemed like a big hassle.)
In slack, Aron stated that users can not be deleted from within Connect. So if another user is added in a different test, then the total number of users in this test will be larger than 4 (aron, bill, cole, and me).
It feels a little off that this blends something from setup with a test here
Agreed. But each of the other tests require the three user objects to already exist.
We could move the .create()
test to another test class and retrieve the users in this class to avoid crossing setup / testing. Using a subset of tests won't work well if trying to just test this situation but that's ok.
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.
In slack, Aron stated that users can not be deleted from within Connect. So if another user is added in a different test, then the total number of users in this test will be larger than 4 (aron, bill, cole, and me).
nods right right, that's true, but also true if we have 4
there or if we do 3 + self.existing_users
, right? We would need to do 4 + self.existing_users
or 5 + self.existing_users
if we add more users. Or am I missing something?
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.
In this test we only add 3x users: aron
, bill
, and cole
. So only if we add more users within this test will we need to change 3
to a new value.
Co-authored-by: Jonathan Keane <[email protected]>
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.
Thanks for cleaning this up, I like where this is. A few minor comments, mostly style / docs.
if isinstance(arg, str): | ||
principal_guid = arg |
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.
Do we have other facilities for checking if a string is the right shape to be a GUID? That might be nice to check here since someone might try passing client.content.get(content_guid).permissions.destroy("my group name")
or the like?
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.
No. We do not have the functionality.
I believe the thought process is to have the server return an error and we display the error.
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.
nods that makes sense. What does the server return as the error in that case? Is it something we expect SDK authors to know how to recover from?
Doing something more than ^^^ is probably scope creep, but I am curious what the ergonomics would be
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.
Should we instead change the approach of this method to always submit the delete API calls to the server? (rather than only the permissions we know exist.)
Then the server will always display an error. And we wouldn't need to do any validation on our end.
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.
AAAH right on line 223 it will silently be ignore if it's anything other than a matching GUID. HMMM I don't have a strong intuition which direction would be better here, pre-checking or always sending and failing. There certainly are complications with either.
Co-authored-by: Jonathan Keane <[email protected]>
Fixes: #338
Related: https://github.com/rstudio/connect/issues/28826
Other changes
Permission.delete()
deprecated forPermission.destroy()