PR#4: Fix pagure-dist-git and optimize it - pagure-dist-git

pagure-dist-git

#4 Fix pagure-dist-git and optimize it

Merged 8 years ago by pingou. Opened 8 years ago by pingou.

optimization into master

Rework the plugin to reduce the diff it produces

Pierre-Yves Chibon • 8 years ago

3d57e18

Do not reconfigure the logger when one is already configured

Pierre-Yves Chibon • 8 years ago

37636f9

dist_git_auth.py

file modified

+93 -109

		`@@ -26,7 +26,7 @@`
		`from pagure.lib import model # noqa: E402`
		`from pagure.lib.git_auth import Gitolite3Auth, _read_file # noqa: E402`

		`- logging.config.dictConfig(APP.config.get('LOGGING') or {'version': 1})`
		`+`
		`_log = logging.getLogger(__name__)`

		`cache = dogpile.cache.make_region().configure(`
		`@@ -47,6 +47,7 @@`
		`'container': 'container',`
		`}`

		`+`
		`@cache.cache_on_arguments()`
		`def get_supported_branches(namespace, package):`
		`default_url = 'https://pdc.fedoraproject.org/rest_api/v1/'`
		`@@ -65,111 +66,94 @@`
		`""" A dist-git's gitolite authentication module. """`

		`@classmethod`
		`- def write_gitolite_acls(`
		`- cls, session, configfile, preconf=None, postconf=None):`
		`- ''' Generate the configuration file for gitolite for all projects`
		`- on the forge.`
		`- '''`
		`- _log.info('Write down the dist-git\'s gitolite configuration file')`
		`-`
		`- preconfig = None`
		`- if preconf:`
		`- _log.info(`
		`- 'Loading the file to include at the top of the generated one')`
		`- preconfig = _read_file(preconf)`
		`-`
		`- postconfig = None`
		`- if postconf:`
		`- _log.info(`
		`- 'Loading the file to include at the end of the generated one')`
		`- postconfig = _read_file(postconf)`
		`-`
		`- global_pr_only = pagure.APP.config.get('PR_ONLY', False)`
		`- config = []`
		`- groups = {}`
		`- query = session.query(model.Project).order_by(model.Project.id)`
		`- for project in query.all():`
		`- _log.debug(' Processing project: %s', project.fullname)`
		`- for group in project.committer_groups:`
		`- if group.group_name not in groups:`
		`- 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)`
		`-`
		`- for repos in ['repos', 'requests/']:`
		`- if repos == 'repos':`
		`- # Do not grant access to project enforcing the PR model`
		`- if pr_only or (global_pr_only and not project.is_fork):`
		`- continue`
		`- repos = ''`
		`-`
		`- config.append('repo %s%s' % (repos, project.fullname))`
		`- if repos not in ['tickets/', 'requests/']:`
		`- config.append(' R = @all')`
		`-`
		`- access = 'RWC'`
		`- if project.is_fork:`
		`- access = 'RW+'`
		`-`
		`- if repos == '':`
		`- # First, whitelist the supported branches from PDC`
		`- for branch in get_supported_branches(project.namespace, project.name):`
		`- config.append(' %s %s = %s' % (access, branch, project.user.user))`
		`- for user in project.committers:`
		`- if user != project.user:`
		`- config.append(' %s = %s' % (access, branch, user.user))`
		`-`
		`- # Then, blacklist a pattern over that (after).`
		`- config.append(_blacklist)`
		`-`
		`- if project.committer_groups:`
		`- config.append(' %s+ = @%s' % (access, ' @'.join(`
		`- [`
		`- group.group_name`
		`- for group in project.committer_groups`
		`- ]`
		`- )))`
		`-`
		`- config.append(' %s = %s' % (access, project.user.user))`
		`- for user in project.committers:`
		`- if user != project.user:`
		`- config.append(' %s = %s' % (access, user.user))`
		`-`
		`- for deploykey in project.deploykeys:`
		`- access = 'R'`
		`- if deploykey.pushaccess:`
		`- access = 'RW'`
		`- if project.is_fork:`
		`- access = 'RW+'`
		`- # Note: the replace of / with _ is because gitolite`
		`- # users can't contain a /. At first, this might look`
		`- # like deploy keys in a project called`
		`- # $namespace_$project would give access to the repos of`
		`- # a project $namespace/$project or vica versa, however`
		`- # this is NOT the case because we add the deploykey.id`
		`- # to the end of the deploykey name, which means it is`
		`- # unique. The project name is solely there to make it`
		`- # easier to determine what project created the deploykey`
		`- # for admins.`
		`- config.append(' %s = deploykey_%s_%s' %`
		`- (access,`
		`- werkzeug.secure_filename(project.fullname),`
		`- deploykey.id))`
		`- config.append('')`
		`-`
		`- with open(configfile, 'w') as stream:`
		`- if preconfig:`
		`- stream.write(preconfig + '\n')`
		`-`
		`- for key, users in groups.iteritems():`
		`- stream.write('@%s = %s\n' % (key, ' '.join(users)))`
		`- stream.write('\n')`
		`-`
		`- for row in config:`
		`- stream.write(row + '\n')`
		`-`
		`- if postconfig:`
		`- stream.write(postconfig + '\n')`
		`+ def _process_project(cls, project, config, groups, global_pr_only):`
		`+ """ Generate the gitolite configuration for the specified project.`
		`+`
		`+ :arg project: the project to generate the configuration for`
		`+ :type project: pagure.lib.model.Project`
		`+ :arg config: a list containing the different lines of the`
		`+ configuration file`
		`+ :type config: list`
		`+ :arg groups: a dictionary containing the group name as key and the`
		`+ users member of the group as values`
		`+ :type groups: dict(str: list)`
		`+ :arg global_pr_only: boolean on whether the pagure instance enforces`
		`+ the PR workflow only or not`
		`+ :type global_pr_only: bool`
		`+ :return: a tuple containing the updated config and groups variables`
		`+ :return type: tuple(list, dict(str: list))`
		`+`
		`+ """`
		`+`
		`+ _log.debug(' Processing project: %s', project.fullname)`
		`+ for group in project.committer_groups:`
		`+ if group.group_name not in groups:`
		`+ 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)`
		`+`
		`+ for repos in ['repos', 'requests/']:`
		`+ if repos == 'repos':`
		`+ # Do not grant access to project enforcing the PR model`
		`+ if pr_only or (global_pr_only and not project.is_fork):`
		`+ continue`
		`+ repos = ''`
		`+`
		`+ config.append('repo %s%s' % (repos, project.fullname))`
		`+ if repos not in ['tickets/', 'requests/']:`
		`+ config.append(' R = @all')`
		`+`
		`+ access = 'RWC'`
		`+ if project.is_fork:`
		`+ access = 'RW+'`
		`+`
		`+ if repos == '':`
		`+ # First, whitelist the supported branches from PDC`
		`+ for branch in get_supported_branches(project.namespace, project.name):`
		`+ config.append(' %s %s = %s' % (access, branch, project.user.user))`
		`+ for user in project.committers:`
		`+ if user != project.user:`
		`+ config.append(' %s %s = %s' % (access, branch, user.user))`
		`+`
		`+ # Then, blacklist a pattern over that (after).`
		`+ config.append(_blacklist)`
		`+`
		`+ if project.committer_groups:`
		`+ config.append(' %s+ = @%s' % (access, ' @'.join(`
		`+ [`
		`+ group.group_name`
		`+ for group in project.committer_groups`
		`+ ]`
		`+ )))`
		`+`
		`+ config.append(' %s = %s' % (access, project.user.user))`
		`+ for user in project.committers:`
		`+ if user != project.user:`
		`+ config.append(' %s = %s' % (access, user.user))`
		`+`
		`+ for deploykey in project.deploykeys:`
		`+ access = 'R'`
		`+ if deploykey.pushaccess:`
		`+ access = 'RW'`
		`+ if project.is_fork:`
		`+ access = 'RW+'`
		`+ # Note: the replace of / with _ is because gitolite`
		`+ # users can't contain a /. At first, this might look`
		`+ # like deploy keys in a project called`
		`+ # $namespace_$project would give access to the repos of`
		`+ # a project $namespace/$project or vica versa, however`
		`+ # this is NOT the case because we add the deploykey.id`
		`+ # to the end of the deploykey name, which means it is`
		`+ # unique. The project name is solely there to make it`
		`+ # easier to determine what project created the deploykey`
		`+ # for admins.`
		`+ config.append(' %s = deploykey_%s_%s' %`
		`+ (access,`
		`+ werkzeug.secure_filename(project.fullname),`
		`+ deploykey.id))`
		`+ config.append('')`
		`+`
		`+ return (groups, config)`

dist_git_auth_tests.py

file modified

+122 -1

		`@@ -7,10 +7,12 @@`

		`# These are the tests from the pagure/ git repo.`
		`# Run with PYTHONPATH=.:/path/to/pagure/checkout nosetests dist_git_auth_tests.py`
		`+ import pagure`
		`import tests`

		`import dist_git_auth`

		`+`
		`expected = """`
		`repo test`
		`R = @all`
		`@@ -57,14 +59,17 @@`


		`class DistGitoliteAuthTestCase(tests.Modeltests):`
		`+ """ Test generating the gitolite configuration file for dist-git. """`

		`maxDiff = None`

		`def setUp(self):`
		`+ """ Set up the environment in which to run the tests. """`
		`super(DistGitoliteAuthTestCase, self).setUp()`
		`self.configfile = tempfile.mkstemp()[1]`

		`def tearDown(self):`
		`+ """ Tear down the environment in which the tests ran. """`
		`try:`
		`os.remove(self.configfile)`
		`except:`
		`@@ -74,18 +79,134 @@`

		`@mock.patch('dist_git_auth.get_supported_branches')`
		`def test_write_gitolite_acls(self, get_supported_branches):`
		`+ """ Test generating the entire gitolite configuration file`
		`+ (project == -1).`
		`+`
		`+ """`
		`get_supported_branches.return_value = ['master', 'f9000']`
		`print("Initializing DB.")`
		`tests.create_projects(self.session)`
		`+`
		`print("Generating %r" % self.configfile)`
		`dist_git_auth.DistGitoliteAuth.write_gitolite_acls(`
		`- self.session, self.configfile)`
		`+ self.session,`
		`+ configfile=self.configfile,`
		`+ project=-1)`
		`+`
		`print("Checking the contents of %r" % self.configfile)`
		`with open(self.configfile, 'r') as f:`
		`contents = f.read()`
		`self.assertMultiLineEqual(contents.strip(), expected.strip())`

		`+ @mock.patch('dist_git_auth.get_supported_branches')`
		`+ def test_write_gitolite_acls_none_project(self, get_supported_branches):`
		`+ """ Test not touching the gitolite configuration file`
		`+ (project == None).`
		`+`
		`+ """`
		`+ get_supported_branches.return_value = ['master', 'f9000']`
		`+ print("Initializing DB.")`
		`+ tests.create_projects(self.session)`
		`+`
		`+ print("Generating %r" % self.configfile)`
		`+ dist_git_auth.DistGitoliteAuth.write_gitolite_acls(`
		`+ self.session,`
		`+ configfile=self.configfile,`
		`+ project=None)`
		`+`
		`+ print("Checking the contents of %r" % self.configfile)`
		`+ with open(self.configfile, 'r') as f:`
		`+ contents = f.read()`
		`+ self.assertMultiLineEqual(contents.strip(), '')`
		`+`
		`+ @mock.patch('dist_git_auth.get_supported_branches')`
		`+ def test_write_gitolite_acls_test_project(self, get_supported_branches):`
		`+ """ Test updating the gitolite configuration file for just one`
		`+ project (project == a pagure.lib.model.Project).`
		`+`
		`+ """`
		`+`
		`+ get_supported_branches.return_value = ['master', 'f9000']`
		`+ self.test_write_gitolite_acls()`
		`+`
		`+ print("Modifying the test project so the output differs.")`
		`+ project = pagure.lib._get_project(self.session, 'test')`
		`+ project.user_id = 2`
		`+ self.session.add(project)`
		`+ self.session.commit()`
		`+`
		`+ project = pagure.lib._get_project(self.session, 'test')`
		`+ msg = pagure.lib.add_user_to_project(`
		`+ self.session,`
		`+ project=project,`
		`+ new_user='pingou',`
		`+ user='foo',`
		`+ access='commit'`
		`+ )`
		`+ self.assertEqual(msg, 'User added')`
		`+ self.session.commit()`
		`+`
		`+ print("Rewriting %r" % self.configfile)`
		`+ project = pagure.lib._get_project(self.session, 'test')`
		`+ dist_git_auth.DistGitoliteAuth.write_gitolite_acls(`
		`+ self.session,`
		`+ configfile=self.configfile,`
		`+ project=project`
		`+ )`
		`+`
		`+ print("Checking the contents of %r" % self.configfile)`
		`+ with open(self.configfile, 'r') as f:`
		`+ contents = f.read()`
		`+`
		`+ expected = '''repo test2`
		`+ R = @all`
		`+ RWC master = pingou`
		`+ RWC f9000 = pingou`
		`+ - f[0-9][0-9] = @all`
		`+ - epel[0-9] = @all`
		`+ - epel[0-9][0-9] = @all`
		`+ - el[0-9] = @all`
		`+ - olpc[0-9] = @all`
		`+ RWC = pingou`
		`+`
		`+ repo requests/test2`
		`+ RWC = pingou`
		`+`
		`+ repo somenamespace/test3`
		`+ R = @all`
		`+ RWC master = pingou`
		`+ RWC f9000 = pingou`
		`+ - f[0-9][0-9] = @all`
		`+ - epel[0-9] = @all`
		`+ - epel[0-9][0-9] = @all`
		`+ - el[0-9] = @all`
		`+ - olpc[0-9] = @all`
		`+ RWC = pingou`
		`+`
		`+ repo requests/somenamespace/test3`
		`+ RWC = pingou`
		`+`
		`+ repo test`
		`+ R = @all`
		`+ RWC master = foo`
		`+ RWC master = pingou`
		`+ RWC f9000 = foo`
		`+ RWC f9000 = pingou`
		`+ - f[0-9][0-9] = @all`
		`+ - epel[0-9] = @all`
		`+ - epel[0-9][0-9] = @all`
		`+ - el[0-9] = @all`
		`+ - olpc[0-9] = @all`
		`+ RWC = foo`
		`+ RWC = pingou`
		`+`
		`+ repo requests/test`
		`+ RWC = foo`
		`+ RWC = pingou'''`
		`+ self.assertMultiLineEqual(contents.strip(), expected)`
		`+`
		`def test_get_supported_branches(self):`
		`+ """ Test for real what is returned by PDC. """`
		`expected = ['master', 'f26', 'f25', 'f24', 'el6']`
		`actual = dist_git_auth.get_supported_branches('rpms', 'nethack')`
		`self.assertEquals(set(actual), set(expected))`

no initial comment

bowlofeggs commented on line 132 of dist_git_auth.py 8 years ago

I recommend documenting these params and the return value.

bowlofeggs commented on line 6 of dist_git_auth_tests.py 8 years ago

I recommend dropping this line.

bowlofeggs commented on line 47 of dist_git_auth_tests.py 8 years ago

I recommend moving this to the top of the file.

bowlofeggs commented on line 29 of dist_git_auth_tests.py 8 years ago

It would be good to put a docblock here describing the condition this test is checking.

bowlofeggs commented on line 46 of dist_git_auth_tests.py 8 years ago

It would be good to give this one a docblock too.

bowlofeggs commented on line 77 of dist_git_auth_tests.py 8 years ago

I notice that there are a lot of print statements here. I personally prefer to have the tests not print anything but it doesn't really matter. I'm just calling it out in case it wasn't intentional.

bowlofeggs commented on line 204 of dist_git_auth.py 8 years ago

This code seems kinda similar to code in Pagure - is there an opportunity to share code here?

bowlofeggs commented 8 years ago

All comments are optional - LGTM!

pingou commented on line 6 of dist_git_auth_tests.py 8 years ago

It's actually the otherway around, I need to un-comment it, I had commented it out to debug something and forgot to revert it.

pingou commented on line 77 of dist_git_auth_tests.py 8 years ago

@ralph used this patter at first so I kept it there, I'll leave to call to him :)

pingou commented on line 204 of dist_git_auth.py 8 years ago

It's overriding the code in pagure with some additional tweaks (calling to pdc, different permissions), so I'm not sure we can easily share more code

2 new commits added

Rework the plugin to reduce the diff it produces
Do not reconfigure the logger when one is already configured

8 years ago

pingou commented 8 years ago

Thanks for the review! :)

Pull-Request has been merged by pingou

8 years ago

Metadata

Assignee

None

Tags

No Tags

Changes Summary 2