From 499752c793b62db9740c2c8daaf5fa398c2a773d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 07 2017 13:00:03 +0000 Subject: [PATCH 1/4] Add the opportunity to enforce a PR-based workflow With this feature we are simply dropping all access to the main repo, thus forcing all contributors to use a PR-based workflow. In a world where we have CI on PR, this may be an interesting to turn on and in dist-git this may be even more important. --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index ae7f1b6..5038898 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -88,6 +88,7 @@ def write_gitolite_acls(session, configfile): ''' Generate the configuration file for gitolite for all projects on the forge. ''' + global_pr_only = pagure.APP.config.get('PR_ONLY', False) config = [] groups = {} for project in session.query(model.Project).all(): @@ -96,8 +97,16 @@ def write_gitolite_acls(session, configfile): groups[group.group_name] = [ user.username for user in group.users] + # Check if the project or the pagure instance enforce the PR only + # development model. + pr_only = project.settings.get( + 'pull_request_access_only', False) or global_pr_only + for repos in ['repos', 'docs/', 'tickets/', 'requests/']: if repos == 'repos': + # Do not grand access to project enforcing the PR model + if pr_only and not project.is_fork: + continue repos = '' config.append('repo %s%s' % (repos, project.fullname)) diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 5ac9c59..d6941b8 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -487,6 +487,7 @@ class Project(BASE): 'always_merge': False, 'issues_default_to_private': False, 'fedmsg_notifications': True, + 'pull_request_access_only': False, } if self._settings: diff --git a/tests/test_pagure_flask_api_user.py b/tests/test_pagure_flask_api_user.py index 157f358..a4f3254 100644 --- a/tests/test_pagure_flask_api_user.py +++ b/tests/test_pagure_flask_api_user.py @@ -121,6 +121,7 @@ class PagureFlaskApiUSertests(tests.Modeltests): "issue_tracker": True, "issues_default_to_private": False, "project_documentation": False, + "pull_request_access_only": False, "pull_requests": True }, "tags": [], @@ -156,6 +157,7 @@ class PagureFlaskApiUSertests(tests.Modeltests): "issue_tracker": True, "issues_default_to_private": False, "project_documentation": False, + "pull_request_access_only": False, "pull_requests": True }, "tags": [], @@ -191,6 +193,7 @@ class PagureFlaskApiUSertests(tests.Modeltests): "issue_tracker": True, "issues_default_to_private": False, "project_documentation": False, + "pull_request_access_only": False, "pull_requests": True }, "tags": [], diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 4fa467f..60dcec5 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -1390,6 +1390,7 @@ class PagureLibtests(tests.Modeltests): 'always_merge': False, 'issues_default_to_private': False, 'fedmsg_notifications': True, + 'pull_request_access_only': False, }, user='pingou', ) @@ -1408,6 +1409,7 @@ class PagureLibtests(tests.Modeltests): 'Enforce_signed-off_commits_in_pull-request': False, 'issues_default_to_private': False, 'fedmsg_notifications': True, + 'pull_request_access_only': False, }, user='pingou', ) diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index f7f59b4..788ce7e 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -947,6 +947,210 @@ repo requests/forks/pingou/test2 os.unlink(outputconf) self.assertFalse(os.path.exists(outputconf)) + def test_write_gitolite_project_pr_only(self): + """ Test the write_gitolite_acls function of pagure.lib.git. + when the project enforces the PR approach. + """ + tests.create_projects(self.session) + + repo = pagure.lib.get_project(self.session, 'test') + # Make the project enforce the PR workflow + settings = repo.settings + settings['pull_request_access_only'] = True + repo.settings = settings + self.session.add(repo) + self.session.commit() + + # Add an user to a project + # The user will be an admin of the project + msg = pagure.lib.add_user_to_project( + session=self.session, + project=repo, + new_user='foo', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'User added') + # Add a forked project + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + description='test project #2', + is_fork=True, + parent_id=1, + hook_token='aaabbbvvv', + ) + self.session.add(item) + self.session.commit() + + outputconf = os.path.join(self.path, 'test_gitolite.conf') + + pagure.lib.git.write_gitolite_acls(self.session, outputconf) + + self.assertTrue(os.path.exists(outputconf)) + + with open(outputconf) as stream: + data = stream.read() + + exp = """ +repo docs/test + R = @all + RW+ = pingou + RW+ = foo + +repo tickets/test + RW+ = pingou + RW+ = foo + +repo requests/test + RW+ = pingou + RW+ = foo + +repo test2 + R = @all + RW+ = pingou + +repo docs/test2 + R = @all + RW+ = pingou + +repo tickets/test2 + RW+ = pingou + +repo requests/test2 + RW+ = pingou + +repo somenamespace/test3 + R = @all + RW+ = pingou + +repo docs/somenamespace/test3 + R = @all + RW+ = pingou + +repo tickets/somenamespace/test3 + RW+ = pingou + +repo requests/somenamespace/test3 + RW+ = pingou + +repo forks/pingou/test3 + R = @all + RW+ = pingou + +repo docs/forks/pingou/test3 + R = @all + RW+ = pingou + +repo tickets/forks/pingou/test3 + RW+ = pingou + +repo requests/forks/pingou/test3 + RW+ = pingou + +""" + #print data + self.assertEqual(data, exp) + + os.unlink(outputconf) + self.assertFalse(os.path.exists(outputconf)) + + def test_write_gitolite_global_pr_only(self): + """ Test the write_gitolite_acls function of pagure.lib.git. + when the project enforces the PR approach. + """ + tests.create_projects(self.session) + + pagure.APP.config['PR_ONLY'] = True + + repo = pagure.lib.get_project(self.session, 'test') + # Add an user to a project + # The user will be an admin of the project + msg = pagure.lib.add_user_to_project( + session=self.session, + project=repo, + new_user='foo', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'User added') + # Add a forked project + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + description='test project #2', + is_fork=True, + parent_id=1, + hook_token='aaabbbvvv', + ) + self.session.add(item) + self.session.commit() + + outputconf = os.path.join(self.path, 'test_gitolite.conf') + + pagure.lib.git.write_gitolite_acls(self.session, outputconf) + + self.assertTrue(os.path.exists(outputconf)) + + with open(outputconf) as stream: + data = stream.read() + + exp = """ +repo docs/test + R = @all + RW+ = pingou + RW+ = foo + +repo tickets/test + RW+ = pingou + RW+ = foo + +repo requests/test + RW+ = pingou + RW+ = foo + +repo docs/test2 + R = @all + RW+ = pingou + +repo tickets/test2 + RW+ = pingou + +repo requests/test2 + RW+ = pingou + +repo docs/somenamespace/test3 + R = @all + RW+ = pingou + +repo tickets/somenamespace/test3 + RW+ = pingou + +repo requests/somenamespace/test3 + RW+ = pingou + +repo forks/pingou/test3 + R = @all + RW+ = pingou + +repo docs/forks/pingou/test3 + R = @all + RW+ = pingou + +repo tickets/forks/pingou/test3 + RW+ = pingou + +repo requests/forks/pingou/test3 + RW+ = pingou + +""" + #print data + self.assertEqual(data, exp) + + os.unlink(outputconf) + self.assertFalse(os.path.exists(outputconf)) + pagure.APP.config['PR_ONLY'] = False + def test_commit_to_patch(self): """ Test the commit_to_patch function of pagure.lib.git. """ # Create a git repo to play with @@ -1403,7 +1607,7 @@ new file mode 100644 index 0000000..60f7480 --- /dev/null +++ b/456 -@@ -0,0 +1,98 @@ +@@ -0,0 +1,100 @@ +{ + "assignee": null, + "branch": "master", @@ -1439,6 +1643,7 @@ index 0000000..60f7480 + "issue_tracker": true, + "issues_default_to_private": false, + "project_documentation": false, ++ "pull_request_access_only": false, + "pull_requests": true + }, + "tags": [], @@ -1475,6 +1680,7 @@ index 0000000..60f7480 + "issue_tracker": true, + "issues_default_to_private": false, + "project_documentation": false, ++ "pull_request_access_only": false, + "pull_requests": true + }, + "tags": [], From 73f4b5b05a2136412e2d44407d0695f9bb55614c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 07 2017 13:00:03 +0000 Subject: [PATCH 2/4] Fix wording in the documentation (doc-block and comment) --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 5038898..6694901 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -104,7 +104,7 @@ def write_gitolite_acls(session, configfile): for repos in ['repos', 'docs/', 'tickets/', 'requests/']: if repos == 'repos': - # Do not grand access to project enforcing the PR model + # Do not grant access to project enforcing the PR model if pr_only and not project.is_fork: continue repos = '' diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index 788ce7e..f1acb26 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -1057,7 +1057,7 @@ repo requests/forks/pingou/test3 def test_write_gitolite_global_pr_only(self): """ Test the write_gitolite_acls function of pagure.lib.git. - when the project enforces the PR approach. + when the pagure instance enforces the PR approach. """ tests.create_projects(self.session) From 8974096f4df03d4413a169c332ddd1a261291fa9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 07 2017 13:00:03 +0000 Subject: [PATCH 3/4] Use mock to change pagure's main configuration --- diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index f1acb26..c972124 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -1055,15 +1055,16 @@ repo requests/forks/pingou/test3 os.unlink(outputconf) self.assertFalse(os.path.exists(outputconf)) + @patch.dict('pagure.APP.config', {'PR_ONLY': True}) def test_write_gitolite_global_pr_only(self): """ Test the write_gitolite_acls function of pagure.lib.git. when the pagure instance enforces the PR approach. """ tests.create_projects(self.session) - pagure.APP.config['PR_ONLY'] = True - repo = pagure.lib.get_project(self.session, 'test') + self.assertFalse(repo.settings['pull_request_access_only']) + # Add an user to a project # The user will be an admin of the project msg = pagure.lib.add_user_to_project( @@ -1149,7 +1150,6 @@ repo requests/forks/pingou/test3 os.unlink(outputconf) self.assertFalse(os.path.exists(outputconf)) - pagure.APP.config['PR_ONLY'] = False def test_commit_to_patch(self): """ Test the commit_to_patch function of pagure.lib.git. """ From d2a269daedd5778e6e5c7996f6e58e05e51b97c0 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 07 2017 13:00:03 +0000 Subject: [PATCH 4/4] Rework the logic for when to drop access to a repo The idea being to allow fork to enforce the PR only workflow while still ensuring that forks are allowing commit access when the instance globally enforces the PR only workflow. --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 6694901..c663740 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -99,13 +99,12 @@ def write_gitolite_acls(session, configfile): # Check if the project or the pagure instance enforce the PR only # development model. - pr_only = project.settings.get( - 'pull_request_access_only', False) or global_pr_only + pr_only = project.settings.get('pull_request_access_only', False) for repos in ['repos', 'docs/', 'tickets/', 'requests/']: if repos == 'repos': # Do not grant access to project enforcing the PR model - if pr_only and not project.is_fork: + if pr_only or (global_pr_only and not project.is_fork): continue repos = ''