get project's owners and permissions of various users #101

marusinm · 2019-07-08T13:03:30Z

Implementation of getting project owners related to #88.
And getting users with permission for closing Issues and merging PR based on Github permissions and Pagure permissions related to issue #100.

packit-as-a-service · 2019-07-08T19:56:59Z

Congratulations! The build has finished successfully. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/packit-service-ogr-101
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

lachmanfrantisek

Nice work!

Just a few notes.

lachmanfrantisek · 2019-07-09T06:49:08Z

ogr/services/github.py

+        owners = []
+        owner = self.github_repo.owner
+        owners.append(owner.login)
+        return owners


This can be done in one line, but leave it as is if you think it is more clear now.

return [self.github_repo.owner]

lachmanfrantisek · 2019-07-09T06:55:17Z

ogr/services/github.py

+        for username, persmission in collaborators.items():
+            if persmission == "admin" or persmission == "write":
+                usernames.append(username)
+        return usernames


Isn't it possible to close an issue created by myself even if I cannot do that on other issues?

(In that case, we need this method also in the Issue class as it is issue-related.)

Do it in the way it is useful for you in the release-bot..;-)

lachmanfrantisek · 2019-07-09T06:56:53Z

ogr/services/github.py

+        usernames = []
+        collaborators = self._get_collaborators_with_permission()
+        for username, persmission in collaborators.items():
+            if persmission == "admin" or persmission == "write":


Suggested change

if persmission == "admin" or persmission == "write":

if persmission in ["admin", "write"]:

lachmanfrantisek · 2019-07-09T06:57:41Z

ogr/services/github.py

+    def who_can_close_issue(self) -> List[str]:
+        usernames = []
+        collaborators = self._get_collaborators_with_permission()
+        for username, persmission in collaborators.items():


just typo: s/persmission/permission

lachmanfrantisek · 2019-07-09T06:58:45Z

ogr/services/github.py

+        for user in users:
+            permission = self.github_repo.get_collaborator_permission(user)
+            collaborators[user.login] = permission
+            # collaborators.append(user.login)


Suggested change

# collaborators.append(user.login)

lachmanfrantisek · 2019-07-09T07:00:04Z

ogr/services/pagure.py

+        return project["access_users"]["owner"]
+
+    def who_can_close_issue(self) -> List[str]:
+        users: Set[str] = set()


What about returning set for all methods (github/pagure/abstract)?

packit-as-a-service · 2019-07-10T10:00:48Z

Congratulations! The build has finished successfully. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/packit-service-ogr-101
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

marusinm · 2019-07-10T10:01:47Z

Isn't it possible to close an issue created by myself even if I cannot do that on other issues?

@lachmanfrantisek I completely changed the approach from the previous commit how to get user permissions. For example, I changed fnc who_can_close_issue() -> List[users] to can_close_issue(user, issue) -> bool. Using these new functions should be more straightforward and a user can close his issue even if he cannot do that on other issues.

I also change tests from previous commit and their yaml files.

packit-as-a-service · 2019-07-10T10:12:10Z

Congratulations! The build has finished successfully. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/packit-service-ogr-101
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

lachmanfrantisek · 2019-07-11T07:15:44Z

ogr/services/github.py

+            )
+            return False
+
+        for login, permission in collaborators.items():


Personally, I would like to see also the list/set of allowed users. (Since you are going through all of the collaborators and we are not saving any time with that.)

We have following possibilities:

who_can_close_issues and can_close_issue (which will use who_can_close_issues and checks the author of the issue) (We can move the can_close_issue to the Issue object later.)

who_can_close_issue with the optional parameter for specifying the issue id

leave it in the current state

I vote for the second one but make your own decision...;-)

(Same for the pull-requests.)

…n_close_issue and who_can_merge_pr

rpitonak

Nice work! I am going to merge, but feel free to continue the discussion if you find something.

lachmanfrantisek

Rado was quicker than me but I have only two notes.

I've not tested that but we have tests for it and you will realise possible problems in the ReleaseBot.

lachmanfrantisek · 2019-07-12T12:39:46Z

ogr/services/github.py

+
+        for allowed_user in allowed_users:
+            if username == allowed_user:
+                return True


What about this:

if username in allowed_users: return True

lachmanfrantisek · 2019-07-12T12:39:56Z

ogr/services/github.py

+
+        for allowed_user in allowed_users:
+            if username == allowed_user:
+                return True


Same as above.

This comment has been minimized.

Sign in to view

marusinm mentioned this pull request Jul 8, 2019

Add get_owner/get_owners method to project classes #88

Closed

lachmanfrantisek reviewed Jul 9, 2019

View reviewed changes

lachmanfrantisek added kind/feature New feature or a request for enhancement. area/github Related to GitHub implementation. pagure Related to Pagure implementation. labels Jul 9, 2019

lachmanfrantisek reviewed Jul 11, 2019

View reviewed changes

marusinm added 5 commits July 11, 2019 13:06

get project's owners and permissions of various users

fb84c27

fix mypy errors

7b46e35

fix review notes, add can_close_issue and can_merge_pr, remove who_ca…

612b293

…n_close_issue and who_can_merge_pr

delete obsolete comments

311478a

add who_can_marge_pr and who_can_close_issue

171f1fc

marusinm force-pushed the read-permissions branch from b5f070a to 171f1fc Compare July 11, 2019 11:10

rpitonak approved these changes Jul 12, 2019

View reviewed changes

rpitonak merged commit badeddd into packit:master Jul 12, 2019

lachmanfrantisek reviewed Jul 12, 2019

View reviewed changes

marusinm mentioned this pull request Jul 15, 2019

add get_release/get_releases into abstract.py #117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get project's owners and permissions of various users #101

get project's owners and permissions of various users #101

marusinm commented Jul 8, 2019

This comment has been minimized.

packit-as-a-service bot commented Jul 8, 2019

lachmanfrantisek left a comment

lachmanfrantisek Jul 9, 2019

lachmanfrantisek Jul 9, 2019

lachmanfrantisek Jul 9, 2019

lachmanfrantisek Jul 9, 2019

lachmanfrantisek Jul 9, 2019

lachmanfrantisek Jul 9, 2019

packit-as-a-service bot commented Jul 10, 2019

marusinm commented Jul 10, 2019 •

edited

Loading

packit-as-a-service bot commented Jul 10, 2019

lachmanfrantisek Jul 11, 2019

rpitonak left a comment

lachmanfrantisek left a comment

lachmanfrantisek Jul 12, 2019

lachmanfrantisek Jul 12, 2019

	if persmission == "admin" or persmission == "write":
	if persmission in ["admin", "write"]:

get project's owners and permissions of various users #101

get project's owners and permissions of various users #101

Conversation

marusinm commented Jul 8, 2019

This comment has been minimized.

packit-as-a-service bot commented Jul 8, 2019

lachmanfrantisek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Jul 10, 2019

marusinm commented Jul 10, 2019 • edited Loading

packit-as-a-service bot commented Jul 10, 2019

Choose a reason for hiding this comment

rpitonak left a comment

Choose a reason for hiding this comment

lachmanfrantisek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marusinm commented Jul 10, 2019 •

edited

Loading