PR#421: Extend allowed_scms format to allow explicit blocks - koji

		`@@ -1104,29 +1104,46 @@`
		`Kojid will not attempt kerberos authentication to the koji-hub unless the`
		`username field is commented out`

		`- Optional Configuration SourceCodeManagement`
		`- -------------------------------------------`
		`+ Source Control Configuration`
		`+ ----------------------------`

		`/etc/kojid/kojid.conf`
		`^^^^^^^^^^^^^^^^^^^^^`

		`- The pattern is as follows:`
		`- hostname:/path/match:checkout /common?, hostname2:/path/match:checkout /common?`
		`-`
		- ``checkout /common? is 'true' unless set to false``
		`+ The allowed_scms setting controls which source control systems the builder`
		`+ will accept. It is a space-separated list of entries in one of the following`
		`+ forms:`

		`::`

		`- allowed_scms=scm-server.example.com:/repo/base/repos*/:false`
		`+ hostname:path[:use_common[:source_cmd]]`
		`+ !hostname:path`
		`+`
		`+ where`
		`+`
		`+ hostname is a glob pattern matched against SCM hosts.`
		`+`
		`+ path is a glob pattern matched against the SCM path.`
		`+`
		`+ use_common is boolean setting (yes/no, on/off, true/false) that indicates`
		`+ whether koji should also check out /common from the SCM host. The default`
		`+ is on.`
		`+`
		`+ source_cmd is a shell command to be run before building the`
		+ srpm, with commas instead of spaces. It defaults to ``make,sources``.
		`+`
		+ The second form (``!hostname:path``) is used to explicitly block a host:path
		`+ pattern. In particular, it provides the option to block specific subtrees of`
		`+ a host, but allow from it otherwise`

		`- Once the code is checked out kojid will run the following:`

		`::`

		`- make sources`
		`- mv *.spec <rpmbuild>/SPECS`
		`- mv * <rpmbuild>/SOURCES`
		`- rpmbuild -bs <rpmbuild>/SPECS/*.spec`
		`+ allowed_scms=`
		`+ !scm-server.example.com:/blocked/path/*`
		`+ scm-server.example.com:/repo/base/repos*/:no`
		`+ alt-server.example.com:/repo/dist/repos*/:no:fedpkg,sources`
		`+`

		`Add the host to the createrepo channel`
		`--------------------------------------`
		`@@ -1174,7 +1191,7 @@`
		`clicking on the hosts tab. This will show you a list of builders in the`
		`database and the status of each builder.`

		`- Kojira - Dnd\|Yum repository creation and maintenance`
		`+ Kojira - Dnf\|Yum repository creation and maintenance`
		`====================================================`

		`Configuration Files`

koji/daemon.py

file modified

+58 -29

		`@@ -225,7 +225,7 @@`
		`if self.scheme in schemes:`
		`self.scmtype = scmtype`
		`break`
		`- else:`
		`+ else: # pragma: no cover`
		`# should never happen`
		`raise koji.GenericError('Invalid SCM URL: %s' % url)`

		`@@ -267,7 +267,8 @@`
		`# check for validity: params should be empty, query may be empty, everything else should be populated`
		`if params:`
		`raise koji.GenericError('Unable to parse SCM URL: %s . Params element %s should be empty.' % (self.url, params))`
		`- if not scheme:`
		`+ if not scheme: #pragma: no cover`
		`+ # should not happen because of is_scm_url check earlier`
		`raise koji.GenericError('Unable to parse SCM URL: %s . Could not find the scheme element.' % self.url)`
		`if not netloc:`
		`raise koji.GenericError('Unable to parse SCM URL: %s . Could not find the netloc element.' % self.url)`
		`@@ -281,40 +282,68 @@`

		`def assert_allowed(self, allowed):`
		`"""`
		`- Verify that the host and repository of this SCM is in the provided list of`
		`- allowed repositories.`
		`+ Check this scm against allowed list and apply options`
		`+`
		`+ allowed is a space-separated list of entries in one of the following`
		`+ forms:`
		`+`
		`+ host:repository[:use_common[:source_cmd]]`
		`+ !host:repository`
		`+`
		`+ Incorrectly-formatted entries will be skipped with a warning.`
		`+`
		`+ The first form allows a host:repository pattern and optionally sets a`
		`+ few options for it.`
		`+`
		`+ The second form explicitly blocks a host:repository pattern`
		`+`
		`+ Both host and repository are treated as glob patterns`
		`+`
		`+ If there is a matching entry, then the optional fields, if given, will`
		`+ be applied to the instance.`

		`- allowed is a space-separated list of host:repository[:use_common[:source_cmd]] tuples. Incorrectly-formatted`
		`- tuples will be ignored.`
		`+ If there is no matching entry, or if the host:repository is blocked`
		`+ then BuildError is raised.`

		`- If use_common is not present, kojid will attempt to checkout a common/ directory from the`
		`- repository. If use_common is set to no, off, false, or 0, it will not attempt to checkout a common/`
		`- directory.`
		`+ The use_common option defaults to on. If it is set to no, off, false`
		`+ or 0, it will be disabled. If the option is on, then kojid will`
		`+ attempt to checkout a common/ directory from the repository.`

		`- source_cmd is a shell command (args separated with commas instead of spaces) to run before building the srpm.`
		`- It is generally used to retrieve source files from a remote location. If no source_cmd is specified,`
		`- "make sources" is run by default.`
		`+ The source_command is a shell command to be run before building the`
		`+ srpm. It defaults to "make sources". This can be overridden by the`
		`+ matching allowed entry. The command must be encoded with commas`
		`+ instead of spaces (e.g. "make,sources").`
		`"""`
		`+ is_allowed = False`
		`for allowed_scm in allowed.split():`
		`scm_tuple = allowed_scm.split(':')`
		`- if len(scm_tuple) >= 2:`
		`- if fnmatch(self.host, scm_tuple[0]) and fnmatch(self.repository, scm_tuple[1]):`
		`- # SCM host:repository is in the allowed list`
		`- # check if we specify a value for use_common`
		`- if len(scm_tuple) >= 3:`
		`- if scm_tuple[2].lower() in ('no', 'off', 'false', '0'):`
		`- self.use_common = False`
		`- # check if we specify a custom source_cmd`
		`- if len(scm_tuple) >= 4:`
		`- if scm_tuple[3]:`
		`- self.source_cmd = scm_tuple[3].split(',')`
		`- else:`
		`- # there was nothing after the trailing :, so they don't want to run a source_cmd at all`
		`- self.source_cmd = None`
		`- break`
		`- else:`
		`+ if len(scm_tuple) < 2:`
		`self.logger.warn('Ignoring incorrectly formatted SCM host:repository: %s' % allowed_scm)`
		`- else:`
		`+ continue`
		`+ host_pat = scm_tuple[0]`
		`+ repo_pat = scm_tuple[1]`
		`+ invert = False`
		`+ if host_pat.startswith('!'):`
		`+ invert = True`
		`+ host_pat = host_pat[1:]`
		`+ if fnmatch(self.host, host_pat) and fnmatch(self.repository, repo_pat):`
		`+ # match`
		`+ if invert:`
		`+ break`
		`+ is_allowed = True`
		`+ # check if we specify a value for use_common`
		`+ if len(scm_tuple) >= 3:`
		`+ if scm_tuple[2].lower() in ('no', 'off', 'false', '0'):`
		`+ self.use_common = False`
		`+ # check if we specify a custom source_cmd`
		`+ if len(scm_tuple) >= 4:`
		`+ if scm_tuple[3]:`
		`+ self.source_cmd = scm_tuple[3].split(',')`
		`+ else:`
		`+ # there was nothing after the trailing :, so they don't want to run a source_cmd at all`
		`+ self.source_cmd = None`
		`+ break`
		`+ if not is_allowed:`
		`raise koji.BuildError('%s:%s is not in the list of allowed SCMs' % (self.host, self.repository))`

		`def checkout(self, scmdir, session=None, uploadpath=None, logfile=None):`

tests/test_scm.py

file added

+371

		`@@ -0,0 +1,371 @@`
		`+ import mock`
		`+ import unittest`
		`+`
		`+ import logging`
		`+ import shutil`
		`+ import tempfile`
		`+`
		`+ from pprint import pprint`
		`+`
		`+ import koji`
		`+ import koji.daemon`
		`+`
		`+ from koji.daemon import SCM`
		`+`
		`+`
		`+ class TestSCM(unittest.TestCase):`
		`+`
		`+ def test_urlcheck(self):`
		`+ good = [`
		`+ "git://server/foo.git#bab0c73900241ef5c465d7e873e9d8b34c948e67",`
		`+ "git+ssh://server2/other/path#bab0c73900241ef5c465d7e873e9d8b34c948e67",`
		`+ "svn://server/path/to/code#bab0c73900241ef5c465d7e873e9d8b34c948e67",`
		`+ "svn+ssh://server/some/path#bab0c73900241ef5c465d7e873e9d8b34c948e67",`
		`+ "cvs://server/some/path#bab0c73900241ef5c465d7e873e9d8b34c948e67",`
		`+ "cvs+ssh://server/some/path#bab0c73900241ef5c465d7e873e9d8b34c948e67",`
		`+ ]`
		`+ bad = [`
		`+ "http://localhost/foo.html",`
		`+ "foo-1.1-1.src.rpm",`
		`+ "https://server/foo-1.1-1.src.rpm",`
		`+ ]`
		`+ for url in good:`
		`+ self.assertTrue(SCM.is_scm_url(url))`
		`+ for url in bad:`
		`+ self.assertFalse(SCM.is_scm_url(url))`
		`+`
		`+ @mock.patch('logging.getLogger')`
		`+ def test_init(self, getLogger):`
		`+ bad = [`
		`+ "git://user@@server/foo.git#bab0c73900241ef5c465d7e873e9d8b34c948e67",`
		`+ "git://user:pass@server/foo.git#bab0c73900241ef5c465d7e873e9d8b34c948e67",`
		`+ "git://server/foo.git;params=not_allowed",`
		`+ "git://server#asdasd", # no path`
		`+ "git://server/foo.git", # no fragment`
		`+ "http://localhost/foo.html",`
		`+ "git://@localhost/foo/?a=bar/",`
		`+ "http://localhost/foo.html?a=foo/",`
		`+ "foo-1.1-1.src.rpm",`
		`+ "git://",`
		`+ "https://server/foo-1.1-1.src.rpm",`
		`+ ]`
		`+ for url in bad:`
		`+ with self.assertRaises(koji.GenericError):`
		`+ scm = SCM(url)`
		`+`
		`+ url = "git://user@server/foo.git#bab0c73900241ef5c465d7e873e9d8b34c948e67"`
		`+ scm = SCM(url)`
		`+ self.assertEqual(scm.scheme, 'git://')`
		`+ self.assertEqual(scm.user, 'user')`
		`+ self.assertEqual(scm.host, 'server')`
		`+ self.assertEqual(scm.repository, '/foo.git')`
		`+ self.assertEqual(scm.module, '')`
		`+ self.assertEqual(scm.revision, 'bab0c73900241ef5c465d7e873e9d8b34c948e67')`
		`+ self.assertEqual(scm.use_common, True)`
		`+ self.assertEqual(scm.source_cmd, ['make', 'sources'])`
		`+ self.assertEqual(scm.scmtype, 'GIT')`
		`+`
		`+ @mock.patch('logging.getLogger')`
		`+ def test_allowed(self, getLogger):`
		`+ config = '''`
		`+ goodserver:*:no`
		`+ !badserver:*`
		`+ !maybeserver:/badpath/*`
		`+ maybeserver:*:no`
		`+ '''`
		`+ good = [`
		`+ "git://goodserver/path1#1234",`
		`+ "git+ssh://maybeserver/path1#1234",`
		`+ ]`
		`+ bad = [`
		`+ "cvs://badserver/projects/42#ref",`
		`+ "svn://badserver/projects/42#ref",`
		`+ "git://maybeserver/badpath/project#1234",`
		`+ ]`
		`+ for url in good:`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ for url in bad:`
		`+ scm = SCM(url)`
		`+ try:`
		`+ scm.assert_allowed(config)`
		`+ except koji.BuildError:`
		`+ pass`
		`+ else:`
		`+ raise AssertionError("allowed bad url: %s" % url)`
		`+`
		`+ @mock.patch('logging.getLogger')`
		`+ def test_badrule(self, getLogger):`
		`+ config = '''`
		`+ bogus-entry-should-be-ignored`
		`+ goodserver:*:no`
		`+ !badserver:*`
		`+ '''`
		`+ url = "git://goodserver/path1#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+`
		`+ @mock.patch('logging.getLogger')`
		`+ def test_opts(self, getLogger):`
		`+ config = '''`
		`+ default:*`
		`+ nocommon:*:no`
		`+ srccmd:*:no:fedpkg,sources`
		`+ nosrc:*:no:`
		`+ mixed:/foo/*:no`
		`+ mixed:/bar/*:yes`
		`+ mixed:/baz/*:no:fedpkg,sources`
		`+ '''`
		`+`
		`+ url = "git://default/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, True)`
		`+ self.assertEqual(scm.source_cmd, ['make', 'sources'])`
		`+`
		`+ url = "git://nocommon/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.assertEqual(scm.source_cmd, ['make', 'sources'])`
		`+`
		`+ url = "git://srccmd/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])`
		`+`
		`+ url = "git://nosrc/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.assertEqual(scm.source_cmd, None)`
		`+`
		`+ url = "git://mixed/foo/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.assertEqual(scm.source_cmd, ['make', 'sources'])`
		`+`
		`+ url = "git://mixed/bar/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, True)`
		`+ self.assertEqual(scm.source_cmd, ['make', 'sources'])`
		`+`
		`+ url = "git://mixed/baz/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])`
		`+`
		`+ url = "git://mixed/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ with self.assertRaises(koji.BuildError):`
		`+ scm.assert_allowed(config)`
		`+`
		`+ url = "git://mixed/foo/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.assertEqual(scm.source_cmd, ['make', 'sources'])`
		`+`
		`+ url = "git://mixed/bar/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, True)`
		`+ self.assertEqual(scm.source_cmd, ['make', 'sources'])`
		`+`
		`+ url = "git://mixed/baz/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(config)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])`
		`+`
		`+ url = "git://mixed/koji.git#1234"`
		`+ scm = SCM(url)`
		`+ with self.assertRaises(koji.BuildError):`
		`+ scm.assert_allowed(config)`
		`+`
		`+`
		`+ class TestSCMCheckouts(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.symlink = mock.patch('os.symlink').start()`
		`+ self.getLogger = mock.patch('logging.getLogger').start()`
		`+ self.log_output = mock.patch('koji.daemon.log_output').start()`
		`+ self.log_output.return_value = None`
		`+ self.tempdir = tempfile.mkdtemp()`
		`+ self.session = mock.MagicMock()`
		`+ self.uploadpath = mock.MagicMock()`
		`+ self.logfile = mock.MagicMock()`
		`+ self.config = '''`
		`+ default:*`
		`+ nocommon:*:no`
		`+ srccmd:*:no:fedpkg,sources`
		`+ nosrc:*:no:`
		`+ '''`
		`+`
		`+ def tearDown(self):`
		`+ mock.patch.stopall()`
		`+ shutil.rmtree(self.tempdir)`
		`+`
		`+ def test_checkout_git_nocommon(self):`
		`+`
		`+ url = "git://nocommon/koji.git#asdasd"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(self.config)`
		`+ scm.checkout(self.tempdir, session=self.session,`
		`+ uploadpath=self.uploadpath, logfile=self.logfile)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.symlink.assert_not_called()`
		`+ # expected commands`
		`+ cmd = ['git', 'clone', '-n', 'git://nocommon/koji.git',`
		`+ self.tempdir + '/koji']`
		`+ call1 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=False, env=None)`
		`+ cmd = ['git', 'reset', '--hard', 'asdasd']`
		`+ call2 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir + '/koji',`
		`+ logerror=1, append=True, env=None)`
		`+ self.log_output.assert_has_calls([call1, call2])`
		`+`
		`+ def test_checkout_gitssh_nocommon(self):`
		`+`
		`+ url = "git+ssh://user@nocommon/koji.git#asdasd"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(self.config)`
		`+ scm.checkout(self.tempdir, session=self.session,`
		`+ uploadpath=self.uploadpath, logfile=self.logfile)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.symlink.assert_not_called()`
		`+ # expected commands`
		`+ cmd = ['git', 'clone', '-n', 'git+ssh://user@nocommon/koji.git',`
		`+ self.tempdir + '/koji']`
		`+ call1 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=False, env=None)`
		`+ cmd = ['git', 'reset', '--hard', 'asdasd']`
		`+ call2 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir + '/koji',`
		`+ logerror=1, append=True, env=None)`
		`+ self.log_output.assert_has_calls([call1, call2])`
		`+`
		`+ def test_checkout_git_common(self):`
		`+`
		`+ url = "git://default/koji.git#asdasd"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(self.config)`
		`+ scm.checkout(self.tempdir, session=self.session,`
		`+ uploadpath=self.uploadpath, logfile=self.logfile)`
		`+ self.assertEqual(scm.use_common, True)`
		`+ self.symlink.assert_called_once()`
		`+ # expected commands`
		`+ cmd = ['git', 'clone', '-n', 'git://default/koji.git',`
		`+ self.tempdir + '/koji']`
		`+ call1 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=False, env=None)`
		`+ cmd = ['git', 'reset', '--hard', 'asdasd']`
		`+ call2 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir + '/koji',`
		`+ logerror=1, append=True, env=None)`
		`+ cmd = ['git', 'clone', 'git://default/common.git', 'common']`
		`+ call3 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir,`
		`+ logerror=1, append=True, env=None)`
		`+ self.log_output.assert_has_calls([call1, call2, call3])`
		`+`
		`+ def test_checkout_error_in_command(self):`
		`+`
		`+ url = "git://nocommon/koji.git#asdasd"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(self.config)`
		`+ self.log_output.return_value = 1`
		`+ with self.assertRaises(koji.BuildError):`
		`+ scm.checkout(self.tempdir, session=self.session,`
		`+ uploadpath=self.uploadpath, logfile=self.logfile)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.symlink.assert_not_called()`
		`+ # expected commands`
		`+ cmd = ['git', 'clone', '-n', 'git://nocommon/koji.git',`
		`+ self.tempdir + '/koji']`
		`+ call1 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=False, env=None)`
		`+ # should have errored after first command`
		`+ self.log_output.assert_has_calls([call1])`
		`+`
		`+ def test_checkout_cvs_common(self):`
		`+`
		`+ url = "cvs://default/cvsisdead?rpms/foo/EL3#sometag"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(self.config)`
		`+ scm.checkout(self.tempdir, session=self.session,`
		`+ uploadpath=self.uploadpath, logfile=self.logfile)`
		`+ self.assertEqual(scm.use_common, True)`
		`+ self.symlink.assert_called_once()`
		`+ # expected commands`
		`+ cmd = ['cvs', '-d', ':pserver:anonymous@default:/cvsisdead', 'checkout',`
		`+ '-r', 'sometag', 'rpms/foo/EL3']`
		`+ call1 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=False, env=None)`
		`+ cmd = ['cvs', '-d', ':pserver:anonymous@default:/cvsisdead', 'checkout',`
		`+ 'common']`
		`+ call2 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=True, env=None)`
		`+ self.log_output.assert_has_calls([call1, call2])`
		`+`
		`+ def test_checkout_cvs_ssh(self):`
		`+`
		`+ url = "cvs+ssh://user@nocommon/cvsisdead?rpms/foo/EL3#sometag"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(self.config)`
		`+ scm.checkout(self.tempdir, session=self.session,`
		`+ uploadpath=self.uploadpath, logfile=self.logfile)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.symlink.assert_not_called()`
		`+ # expected commands`
		`+ cmd = ['cvs', '-d', ':ext:user@nocommon:/cvsisdead', 'checkout', '-r',`
		`+ 'sometag', 'rpms/foo/EL3']`
		`+ call1 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=False, env={'CVS_RSH': 'ssh'})`
		`+ self.log_output.assert_has_calls([call1])`
		`+`
		`+ def test_checkout_svn(self):`
		`+`
		`+ url = "svn://nocommon/dist?rpms/foo/EL3#revision"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(self.config)`
		`+ scm.checkout(self.tempdir, session=self.session,`
		`+ uploadpath=self.uploadpath, logfile=self.logfile)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.symlink.assert_not_called()`
		`+ # expected commands`
		`+ cmd = ['svn', 'checkout', '-r', 'revision',`
		`+ 'svn://nocommon/dist/rpms/foo/EL3', 'rpms/foo/EL3']`
		`+ call1 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=False, env=None)`
		`+ self.log_output.assert_has_calls([call1])`
		`+`
		`+ def test_checkout_svn_ssh(self):`
		`+`
		`+ url = "svn+ssh://user@nocommon/dist?rpms/foo/EL3#revision"`
		`+ scm = SCM(url)`
		`+ scm.assert_allowed(self.config)`
		`+ scm.checkout(self.tempdir, session=self.session,`
		`+ uploadpath=self.uploadpath, logfile=self.logfile)`
		`+ self.assertEqual(scm.use_common, False)`
		`+ self.symlink.assert_not_called()`
		`+ # expected commands`
		`+ cmd = ['svn', 'checkout', '-r', 'revision',`
		`+ 'svn+ssh://user@nocommon/dist/rpms/foo/EL3', 'rpms/foo/EL3']`
		`+ call1 = mock.call(self.session, cmd[0], cmd, self.logfile,`
		`+ self.uploadpath, cwd=self.tempdir, logerror=1,`
		`+ append=False, env=None)`
		`+ self.log_output.assert_has_calls([call1])`

mikem commented 6 years ago

See the updated docstring for details. This change adds the ability to have entries like:

!host:repository

Which would explicitly block a host:repository pattern. This allows for somewhat more robust rules. In particular, it provides the option
to block specific subtrees of a host, but allow from it otherwise. E.g.

!my_pagure:forks/*
my_pagure:*:no

mikem commented 6 years ago

also included: unit tests for the SCM class

mikem commented 6 years ago

@pingou this should cover that one issue we were discussing

pingou commented on line 34 of koji/daemon.py 6 years ago

Should we warn so admins can see in the logs that something is wrong in the config?

pingou commented on line 34 of koji/daemon.py 6 years ago

Nevermind, we're already warning :)

pingou commented on line 68 of tests/test_scm.py 6 years ago

If you want to follow pep8 you'll need to remove one line here :)

pingou commented on line 72 of tests/test_scm.py 6 years ago

Completely nitpicking, but confused me for about half a second, maybe we could name the variable "configuration" rather than "allowed".

pingou commented 6 years ago

Couple of nitpicks but nothing major or blocking for me.

:ship: it!

1 new commit added

rename variable in unit test. clarify docstring

6 years ago

mikem commented 6 years ago

The variable in the tests was named to match the parameter in the call, but I suppose config is more accurate. I'd change it there too, but I'd rather not change a function signature in the library (apart from extension) for something like this.