From ca7d6256e5b06390470169b0095fef1f6d4eab42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Wed, 21 Nov 2018 10:56:22 +0100 Subject: [PATCH] Move resolving git reference to config validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of multiple places handling the same thing duplicating the logic, it's better to do it once upfront. This allows easy caching of the results. Additional advantage of this approach is that the config dump will include resolved URLs. The original reference will still be available in the copy of the original config. JIRA: COMPOSE-3065 Signed-off-by: Lubomír Sedlář --- bin/pungi-config-validate | 11 +++-- pungi/checks.py | 85 +++++++++++++++++++---------------- pungi/phases/base.py | 31 +++---------- pungi/phases/osbs.py | 2 +- pungi/util.py | 18 ++++++-- tests/helpers.py | 4 +- tests/test_checks.py | 32 +++++++++++++ tests/test_config.py | 6 ++- tests/test_imagebuildphase.py | 41 ----------------- tests/test_liveimagesphase.py | 27 +++-------- tests/test_livemediaphase.py | 33 ++++---------- tests/test_osbs_phase.py | 81 +++++++++++++++------------------ tests/test_phase_base.py | 61 +------------------------ tests/test_util.py | 13 +++++- 14 files changed, 177 insertions(+), 268 deletions(-) diff --git a/bin/pungi-config-validate b/bin/pungi-config-validate index d18acc06..39d82a64 100755 --- a/bin/pungi-config-validate +++ b/bin/pungi-config-validate @@ -77,11 +77,11 @@ def read_variants(compose, config): compose.all_variants[child.uid] = child -def run(config, topdir, has_old): +def run(config, topdir, has_old, offline): conf = kobo.conf.PyConfigParser() conf.load_from_file(config) - errors, warnings = pungi.checks.validate(conf) + errors, warnings = pungi.checks.validate(conf, offline=offline) if errors or warnings: for error in errors + warnings: print(error) @@ -146,10 +146,15 @@ def main(args=None): help='configuration file to validate') parser.add_argument('--old-composes', action='store_true', help='indicate if pungi-koji will be run with --old-composes option') + parser.add_argument( + "--offline", + action="store_true", + help="Do not validate git references in URLs", + ) opts = parser.parse_args(args) with pungi.util.temp_dir() as topdir: - errors = run(opts.config, topdir, opts.old_composes) + errors = run(opts.config, topdir, opts.old_composes, opts.offline) for msg in errors: print(msg) diff --git a/pungi/checks.py b/pungi/checks.py index 65e97ea6..ad4dcbd5 100644 --- a/pungi/checks.py +++ b/pungi/checks.py @@ -36,7 +36,6 @@ When a new config option is added, the schema must be updated (see the from __future__ import print_function -import contextlib import multiprocessing import os.path import platform @@ -166,16 +165,19 @@ def _check_dep(name, value, lst, matcher, fmt): yield fmt.format(name, value, dep) -def validate(config): +def validate(config, offline=False): """Test the configuration against schema. Undefined values for which a default value exists will be filled in. """ schema = make_schema() - DefaultValidator = _extend_with_default_and_alias(jsonschema.Draft4Validator) - validator = DefaultValidator(schema, - {'array': (tuple, list), - 'regex': six.string_types}) + DefaultValidator = _extend_with_default_and_alias( + jsonschema.Draft4Validator, offline=offline + ) + validator = DefaultValidator( + schema, + {"array": (tuple, list), "regex": six.string_types, "url": six.string_types} + ) errors = [] warnings = [] for error in validator.iter_errors(config): @@ -224,18 +226,17 @@ UNKNOWN = 'WARNING: Unrecognized config option: {0}.' UNKNOWN_SUGGEST = 'WARNING: Unrecognized config option: {0}. Did you mean {1}?' -def _extend_with_default_and_alias(validator_class): +def _extend_with_default_and_alias(validator_class, offline=False): validate_properties = validator_class.VALIDATORS["properties"] validate_type = validator_class.VALIDATORS['type'] validate_required = validator_class.VALIDATORS['required'] validate_additional_properties = validator_class.VALIDATORS['additionalProperties'] + resolver = util.GitUrlResolver(offline=offline) - @contextlib.contextmanager def _hook_errors(properties, instance, schema): """ Hook the instance and yield errors and warnings. """ - errors = [] for property, subschema in properties.items(): # update instance for alias option # If alias option for the property is present and property is not specified, @@ -245,11 +246,11 @@ def _extend_with_default_and_alias(validator_class): msg = "WARNING: Config option '%s' is deprecated and now an alias to '%s', " \ "please use '%s' instead. " \ "In:\n%s" % (subschema['alias'], property, property, instance) - errors.append(ConfigOptionWarning(msg)) + yield ConfigOptionWarning(msg) if property in instance: msg = "ERROR: Config option '%s' is an alias of '%s', only one can be used." \ % (subschema['alias'], property) - errors.append(ConfigOptionError(msg)) + yield ConfigOptionError(msg) instance.pop(subschema['alias']) else: instance.setdefault(property, instance.pop(subschema['alias'])) @@ -263,50 +264,53 @@ def _extend_with_default_and_alias(validator_class): if append in instance: msg = "WARNING: Config option '%s' is deprecated, its value will be appended to option '%s'. " \ "In:\n%s" % (append, property, instance) - errors.append(ConfigOptionWarning(msg)) + yield ConfigOptionWarning(msg) if property in instance: msg = "WARNING: Value from config option '%s' is now appended to option '%s'." \ % (append, property) - errors.append(ConfigOptionWarning(msg)) + yield ConfigOptionWarning(msg) instance[property] = force_list(instance[property]) instance[property].extend(force_list(instance.pop(append))) else: msg = "WARNING: Config option '%s' is not found, but '%s' is specified, value from '%s' " \ "is now added as '%s'." % (property, append, append, property) - errors.append(ConfigOptionWarning(msg)) + yield ConfigOptionWarning(msg) instance[property] = instance.pop(append) - yield errors - def _set_defaults(validator, properties, instance, schema): + def properties_validator(validator, properties, instance, schema): """ Assign default values to options that have them defined and are not - specified. + specified. Resolve URLs to Git repos """ for property, subschema in properties.items(): if "default" in subschema and property not in instance: instance.setdefault(property, subschema["default"]) - with _hook_errors(properties, instance, schema) as errors: - for error in errors: - yield error + # Resolve git URL references to actual commit hashes + if subschema.get("type") == "url" and property in instance: + try: + instance[property] = resolver(instance[property]) + except util.GitUrlResolveError as exc: + yield ConfigOptionError(exc) + + for error in _hook_errors(properties, instance, schema): + yield error for error in validate_properties(validator, properties, instance, schema): yield error def _validate_additional_properties(validator, aP, instance, schema): properties = schema.get("properties", {}) - with _hook_errors(properties, instance, schema) as errors: - for error in errors: - yield error + for error in _hook_errors(properties, instance, schema): + yield error for error in validate_additional_properties(validator, aP, instance, schema): yield error def _validate_required(validator, required, instance, schema): properties = schema.get("properties", {}) - with _hook_errors(properties, instance, schema) as errors: - for error in errors: - yield error + for error in _hook_errors(properties, instance, schema): + yield error for error in validate_required(validator, required, instance, schema): yield error @@ -355,12 +359,15 @@ def _extend_with_default_and_alias(validator_class): ) return jsonschema.validators.extend( - validator_class, {"properties": _set_defaults, - "deprecated": error_on_deprecated, - "type": validate_regex_type, - "required": _validate_required, - "additionalProperties": _validate_additional_properties, - "anyOf": _validate_any_of}, + validator_class, + { + "properties": properties_validator, + "deprecated": error_on_deprecated, + "type": validate_regex_type, + "required": _validate_required, + "additionalProperties": _validate_additional_properties, + "anyOf": _validate_any_of, + }, ) @@ -457,7 +464,7 @@ def make_schema(): "type": "object", "properties": { "kickstart": {"type": "string"}, - "ksurl": {"type": "string"}, + "ksurl": {"type": "url"}, "name": {"type": "string"}, "subvariant": {"type": "string"}, "target": {"type": "string"}, @@ -478,7 +485,7 @@ def make_schema(): "osbs_config": { "type": "object", "properties": { - "url": {"type": "string"}, + "url": {"type": "url"}, "target": {"type": "string"}, "name": {"type": "string"}, "version": {"type": "string"}, @@ -765,7 +772,7 @@ def make_schema(): "buildinstall_use_guestmount": {"type": "boolean", "default": True}, "buildinstall_skip": _variant_arch_mapping({"type": "boolean"}), - "global_ksurl": {"type": "string"}, + "global_ksurl": {"type": "url"}, "global_version": {"type": "string"}, "global_target": {"type": "string"}, "global_release": {"$ref": "#/definitions/optional_string"}, @@ -854,17 +861,17 @@ def make_schema(): "type": "boolean", "default": False, }, - "live_images_ksurl": {"type": "string"}, + "live_images_ksurl": {"type": "url"}, "live_images_target": {"type": "string"}, "live_images_release": {"$ref": "#/definitions/optional_string"}, "live_images_version": {"type": "string"}, - "image_build_ksurl": {"type": "string"}, + "image_build_ksurl": {"type": "url"}, "image_build_target": {"type": "string"}, "image_build_release": {"$ref": "#/definitions/optional_string"}, "image_build_version": {"type": "string"}, - "live_media_ksurl": {"type": "string"}, + "live_media_ksurl": {"type": "url"}, "live_media_target": {"type": "string"}, "live_media_release": {"$ref": "#/definitions/optional_string"}, "live_media_version": {"type": "string"}, @@ -983,7 +990,7 @@ def make_schema(): "install_tree_from": {"type": "string"}, "kickstart": {"type": "string"}, "ksversion": {"type": "string"}, - "ksurl": {"type": "string"}, + "ksurl": {"type": "url"}, "version": {"type": "string"}, "scratch": {"type": "boolean"}, "skip_tag": {"type": "boolean"}, diff --git a/pungi/phases/base.py b/pungi/phases/base.py index 9b0600a8..739cbb27 100644 --- a/pungi/phases/base.py +++ b/pungi/phases/base.py @@ -137,7 +137,6 @@ class ImageConfigMixin(object): def __init__(self, *args, **kwargs): super(ImageConfigMixin, self).__init__(*args, **kwargs) - self._phase_ksurl = None def get_config(self, cfg, opt): return cfg.get( @@ -174,31 +173,11 @@ class ImageConfigMixin(object): Get ksurl from `cfg`. If not present, fall back to phase defined one or global one. """ - if 'ksurl' in cfg: - return util.resolve_git_url(cfg['ksurl']) - if '%s_ksurl' % self.name in self.compose.conf: - return self.phase_ksurl - if 'global_ksurl' in self.compose.conf: - return self.global_ksurl - return None - - @property - def phase_ksurl(self): - """Get phase level ksurl, making sure to resolve it only once.""" - # The phase-level setting is cached as instance attribute of the phase. - if not self._phase_ksurl: - ksurl = self.compose.conf.get('%s_ksurl' % self.name) - self._phase_ksurl = util.resolve_git_url(ksurl) - return self._phase_ksurl - - @property - def global_ksurl(self): - """Get global ksurl setting, making sure to resolve it only once.""" - # The global setting is cached in the configuration object. - if 'private_global_ksurl' not in self.compose.conf: - ksurl = self.compose.conf.get('global_ksurl') - self.compose.conf['private_global_ksurl'] = util.resolve_git_url(ksurl) - return self.compose.conf['private_global_ksurl'] + return ( + cfg.get("ksurl") + or self.compose.conf.get("%s_ksurl" % self.name) + or self.compose.conf.get("global_ksurl") + ) class PhaseLoggerMixin(object): diff --git a/pungi/phases/osbs.py b/pungi/phases/osbs.py index 4f5c6599..a0e94564 100644 --- a/pungi/phases/osbs.py +++ b/pungi/phases/osbs.py @@ -50,7 +50,7 @@ class OSBSThread(WorkerThread): koji.login() # Start task - source = util.resolve_git_url(config.pop('url')) + source = config.pop('url') target = config.pop('target') priority = config.pop('priority', None) gpgkey = config.pop('gpgkey', None) diff --git a/pungi/util.py b/pungi/util.py index 34469a63..f878a83c 100644 --- a/pungi/util.py +++ b/pungi/util.py @@ -246,6 +246,10 @@ def _get_git_ref(fragment): return None +class GitUrlResolveError(RuntimeError): + pass + + def resolve_git_url(url): """Given a url to a Git repo specifying HEAD or origin/ as a ref, replace that specifier with actual SHA1 of the commit. @@ -270,12 +274,13 @@ def resolve_git_url(url): lines = [line for line in output.split('\n') if line] if len(lines) == 0: # Branch does not exist in remote repo - raise RuntimeError('Failed to resolve %s: ref does not exist in remote repo' - % url) + raise GitUrlResolveError( + "Failed to resolve %s: ref does not exist in remote repo" % url + ) if len(lines) != 1: # This should never happen. HEAD can not match multiple commits in a # single repo, and there can not be a repo without a HEAD. - raise RuntimeError('Failed to resolve %s', url) + raise GitUrlResolveError("Failed to resolve %s", url) fragment = lines[0].split()[0] result = urllib.parse.urlunsplit((r.scheme, r.netloc, r.path, r.query, fragment)) @@ -297,7 +302,12 @@ class GitUrlResolver(object): if self.offline: return url if url not in self.cache: - self.cache[url] = resolve_git_url(url) + try: + self.cache[url] = resolve_git_url(url) + except GitUrlResolveError as exc: + self.cache[url] = exc + if isinstance(self.cache[url], GitUrlResolveError): + raise self.cache[url] return self.cache[url] diff --git a/tests/helpers.py b/tests/helpers.py index 4b1c8c99..467d3897 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -53,7 +53,7 @@ class PungiTestCase(BaseTestCase): raise def assertValidConfig(self, conf): - self.assertEqual(checks.validate(conf), ([], [])) + self.assertEqual(checks.validate(conf, offline=True), ([], [])) class MockVariant(mock.Mock): @@ -147,7 +147,7 @@ class DummyCompose(object): ) self.topdir = topdir self.conf = load_config(PKGSET_REPOS, **config) - checks.validate(self.conf) + checks.validate(self.conf, offline=True) self.paths = paths.Paths(self) self.has_comps = True self.variants = { diff --git a/tests/test_checks.py b/tests/test_checks.py index bcb24641..a46e7b96 100644 --- a/tests/test_checks.py +++ b/tests/test_checks.py @@ -528,6 +528,38 @@ class TestSchemaValidator(unittest.TestCase): self.assertRegexpMatches(warnings[1], r"^WARNING: Config option 'repo' is not found, but 'repo_from' is specified, value from 'repo_from' is now added as 'repo'.*") self.assertEqual(config.get("live_images")[0][1]['armhfp']['repo'], 'Everything') + @mock.patch("pungi.util.resolve_git_url") + @mock.patch('pungi.checks.make_schema') + def test_resolve_url(self, make_schema, resolve_git_url): + resolve_git_url.return_value = "git://example.com/repo.git#CAFE" + make_schema.return_value = { + "$schema": "http://json-schema.org/draft-04/schema#", + "title": "Pungi Configuration", + "type": "object", + "properties": {"foo": {"type": "url"}}, + } + config = self._load_conf_from_string("foo = 'git://example.com/repo.git#HEAD'") + errors, warnings = checks.validate(config) + self.assertEqual(errors, []) + self.assertEqual(warnings, []) + self.assertEqual(config["foo"], resolve_git_url.return_value) + + @mock.patch("pungi.util.resolve_git_url") + @mock.patch('pungi.checks.make_schema') + def test_resolve_url_when_offline(self, make_schema, resolve_git_url): + make_schema.return_value = { + "$schema": "http://json-schema.org/draft-04/schema#", + "title": "Pungi Configuration", + "type": "object", + "properties": {"foo": {"type": "url"}}, + } + config = self._load_conf_from_string("foo = 'git://example.com/repo.git#HEAD'") + errors, warnings = checks.validate(config, offline=True) + self.assertEqual(errors, []) + self.assertEqual(warnings, []) + self.assertEqual(config["foo"], "git://example.com/repo.git#HEAD") + self.assertEqual(resolve_git_url.call_args_list, []) + class TestUmask(unittest.TestCase): def setUp(self): diff --git a/tests/test_config.py b/tests/test_config.py index d169aa0d..9e09f46d 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -372,7 +372,8 @@ class OstreeInstallerConfigTestCase(ConfigTestCase): class LiveMediaConfigTestCase(ConfigTestCase): - def test_global_config_validation(self): + @mock.patch("pungi.util.resolve_git_url") + def test_global_config_validation(self, resolve_git_url): cfg = load_config( PKGSET_REPOS, live_media_ksurl='git://example.com/repo.git#HEAD', @@ -381,7 +382,10 @@ class LiveMediaConfigTestCase(ConfigTestCase): live_media_version='Rawhide', ) + resolve_git_url.side_effect = lambda x: x.replace("HEAD", "CAFE") + self.assertValidation(cfg) + self.assertEqual(cfg["live_media_ksurl"], "git://example.com/repo.git#CAFE") def test_global_config_null_release(self): cfg = load_config( diff --git a/tests/test_imagebuildphase.py b/tests/test_imagebuildphase.py index d07364c8..fad8a820 100644 --- a/tests/test_imagebuildphase.py +++ b/tests/test_imagebuildphase.py @@ -549,47 +549,6 @@ class TestImageBuildPhase(PungiTestCase): args, kwargs = phase.pool.queue_put.call_args self.assertTrue(args[0][1].get('scratch')) - @mock.patch('pungi.util.resolve_git_url') - @mock.patch('pungi.phases.image_build.ThreadPool') - def test_image_build_resolve_ksurl(self, ThreadPool, resolve_git_url): - compose = DummyCompose(self.topdir, { - 'image_build': { - '^Server$': [ - { - 'image-build': { - 'format': ['docker'], - 'name': 'Fedora-Docker-Base', - 'target': 'f24', - 'version': 'Rawhide', - 'ksurl': 'git://git.fedorahosted.org/git/spin-kickstarts.git?#HEAD', - 'kickstart': "fedora-docker-base.ks", - 'distro': 'Fedora-20', - 'disk_size': 3, - 'arches': ['x86_64'], - 'scratch': True, - } - } - ] - }, - 'koji_profile': 'koji', - }) - - self.assertValidConfig(compose.conf) - - resolve_git_url.return_value = 'git://git.fedorahosted.org/git/spin-kickstarts.git?#BEEFCAFE' - - phase = ImageBuildPhase(compose) - - phase.run() - - # assert at least one thread was started - self.assertTrue(phase.pool.add.called) - - self.assertTrue(phase.pool.queue_put.called_once) - args, kwargs = phase.pool.queue_put.call_args - self.assertEqual(args[0][1]['image_conf'].get('image-build', {}).get('ksurl'), - resolve_git_url.return_value) - @mock.patch('pungi.phases.image_build.ThreadPool') def test_image_build_optional(self, ThreadPool): compose = DummyCompose(self.topdir, { diff --git a/tests/test_liveimagesphase.py b/tests/test_liveimagesphase.py index 09718ac5..b2a906ae 100644 --- a/tests/test_liveimagesphase.py +++ b/tests/test_liveimagesphase.py @@ -241,14 +241,13 @@ class TestLiveImagesPhase(PungiTestCase): 'amd64'))]) @mock.patch('pungi.phases.live_images.ThreadPool') - @mock.patch('pungi.util.resolve_git_url') - def test_spin_appliance(self, resolve_git_url, ThreadPool): + def test_spin_appliance(self, ThreadPool): compose = DummyCompose(self.topdir, { 'live_images': [ ('^Client$', { 'amd64': { 'kickstart': 'test.ks', - 'ksurl': 'https://git.example.com/kickstarts.git?#HEAD', + 'ksurl': 'https://git.example.com/kickstarts.git?#CAFEBABE', 'repo': ['http://example.com/repo/', 'Everything'], 'type': 'appliance', 'target': 'f27', @@ -259,8 +258,6 @@ class TestLiveImagesPhase(PungiTestCase): self.assertValidConfig(compose.conf) - resolve_git_url.return_value = 'https://git.example.com/kickstarts.git?#CAFEBABE' - phase = LiveImagesPhase(compose) phase.run() @@ -291,14 +288,11 @@ class TestLiveImagesPhase(PungiTestCase): 'ksurl': 'https://git.example.com/kickstarts.git?#CAFEBABE'}, compose.variants['Client'], 'amd64'))]) - self.assertEqual(resolve_git_url.mock_calls, - [mock.call('https://git.example.com/kickstarts.git?#HEAD')]) @mock.patch('pungi.phases.live_images.ThreadPool') - @mock.patch('pungi.util.resolve_git_url') - def test_spin_appliance_phase_global_settings(self, resolve_git_url, ThreadPool): + def test_spin_appliance_phase_global_settings(self, ThreadPool): compose = DummyCompose(self.topdir, { - 'live_images_ksurl': 'https://git.example.com/kickstarts.git?#HEAD', + 'live_images_ksurl': 'https://git.example.com/kickstarts.git?#CAFEBABE', 'live_images_release': None, 'live_images_version': 'Rawhide', 'live_images_target': 'f27', @@ -315,8 +309,6 @@ class TestLiveImagesPhase(PungiTestCase): self.assertValidConfig(compose.conf) - resolve_git_url.return_value = 'https://git.example.com/kickstarts.git?#CAFEBABE' - phase = LiveImagesPhase(compose) phase.run() @@ -347,14 +339,11 @@ class TestLiveImagesPhase(PungiTestCase): 'ksurl': 'https://git.example.com/kickstarts.git?#CAFEBABE'}, compose.variants['Client'], 'amd64'))]) - self.assertEqual(resolve_git_url.mock_calls, - [mock.call('https://git.example.com/kickstarts.git?#HEAD')]) @mock.patch('pungi.phases.live_images.ThreadPool') - @mock.patch('pungi.util.resolve_git_url') - def test_spin_appliance_global_settings(self, resolve_git_url, ThreadPool): + def test_spin_appliance_global_settings(self, ThreadPool): compose = DummyCompose(self.topdir, { - 'global_ksurl': 'https://git.example.com/kickstarts.git?#HEAD', + 'global_ksurl': 'https://git.example.com/kickstarts.git?#CAFEBABE', 'global_release': None, 'global_version': 'Rawhide', 'global_target': 'f27', @@ -371,8 +360,6 @@ class TestLiveImagesPhase(PungiTestCase): self.assertValidConfig(compose.conf) - resolve_git_url.return_value = 'https://git.example.com/kickstarts.git?#CAFEBABE' - phase = LiveImagesPhase(compose) phase.run() @@ -403,8 +390,6 @@ class TestLiveImagesPhase(PungiTestCase): 'ksurl': 'https://git.example.com/kickstarts.git?#CAFEBABE'}, compose.variants['Client'], 'amd64'))]) - self.assertEqual(resolve_git_url.mock_calls, - [mock.call('https://git.example.com/kickstarts.git?#HEAD')]) @mock.patch('pungi.phases.live_images.ThreadPool') def test_live_image_build_custom_type(self, ThreadPool): diff --git a/tests/test_livemediaphase.py b/tests/test_livemediaphase.py index f05e8fd1..dba5c155 100644 --- a/tests/test_livemediaphase.py +++ b/tests/test_livemediaphase.py @@ -104,11 +104,10 @@ class TestLiveMediaPhase(PungiTestCase): 'failable_arches': ['amd64', 'x86_64'], }))]) - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.livemedia_phase.ThreadPool') - def test_live_media_with_phase_global_opts(self, ThreadPool, resolve_git_url): + def test_live_media_with_phase_global_opts(self, ThreadPool): compose = DummyCompose(self.topdir, { - 'live_media_ksurl': 'git://example.com/repo.git#HEAD', + 'live_media_ksurl': 'git://example.com/repo.git#BEEFCAFE', 'live_media_target': 'f24', 'live_media_release': 'RRR', 'live_media_version': 'Rawhide', @@ -137,15 +136,10 @@ class TestLiveMediaPhase(PungiTestCase): self.assertValidConfig(compose.conf) - resolve_git_url.return_value = 'git://example.com/repo.git#BEEFCAFE' - phase = LiveMediaPhase(compose) phase.run() self.assertTrue(phase.pool.add.called) - self.assertItemsEqual(resolve_git_url.mock_calls, - [mock.call('git://example.com/repo.git#HEAD'), - mock.call('git://different.com/repo.git')]) self.assertEqual(phase.pool.queue_put.call_args_list, [mock.call((compose, compose.variants['Server'], @@ -190,7 +184,7 @@ class TestLiveMediaPhase(PungiTestCase): { 'arches': ['amd64', 'x86_64'], 'ksfile': 'yet-another.ks', - 'ksurl': 'git://example.com/repo.git#BEEFCAFE', + 'ksurl': 'git://different.com/repo.git', 'ksversion': None, 'name': 'Fedora Server Live', 'release': 'XXX', @@ -205,11 +199,10 @@ class TestLiveMediaPhase(PungiTestCase): 'failable_arches': [], }))]) - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.livemedia_phase.ThreadPool') - def test_live_media_with_global_opts(self, ThreadPool, resolve_git_url): + def test_live_media_with_global_opts(self, ThreadPool): compose = DummyCompose(self.topdir, { - 'global_ksurl': 'git://example.com/repo.git#HEAD', + 'global_ksurl': 'git://example.com/repo.git#BEEFCAFE', 'global_target': 'f24', 'global_release': 'RRR', 'global_version': 'Rawhide', @@ -238,15 +231,10 @@ class TestLiveMediaPhase(PungiTestCase): self.assertValidConfig(compose.conf) - resolve_git_url.return_value = 'git://example.com/repo.git#BEEFCAFE' - phase = LiveMediaPhase(compose) phase.run() self.assertTrue(phase.pool.add.called) - self.assertItemsEqual(resolve_git_url.mock_calls, - [mock.call('git://example.com/repo.git#HEAD'), - mock.call('git://different.com/repo.git')]) self.assertEqual(phase.pool.queue_put.call_args_list, [mock.call((compose, compose.variants['Server'], @@ -291,7 +279,7 @@ class TestLiveMediaPhase(PungiTestCase): { 'arches': ['amd64', 'x86_64'], 'ksfile': 'yet-another.ks', - 'ksurl': 'git://example.com/repo.git#BEEFCAFE', + 'ksurl': 'git://different.com/repo.git', 'ksversion': None, 'name': 'Fedora Server Live', 'release': 'XXX', @@ -356,16 +344,15 @@ class TestLiveMediaPhase(PungiTestCase): with self.assertRaisesRegexp(RuntimeError, r'There is no variant Missing to get repo from.'): phase.run() - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.livemedia_phase.ThreadPool') - def test_live_media_full(self, ThreadPool, resolve_git_url): + def test_live_media_full(self, ThreadPool): compose = DummyCompose(self.topdir, { 'live_media': { '^Server$': [ { 'target': 'f24', 'kickstart': 'file.ks', - 'ksurl': 'git://example.com/repo.git#HEAD', + 'ksurl': 'git://example.com/repo.git#BEEFCAFE', 'name': 'Fedora Server Live', 'scratch': True, 'skip_tag': True, @@ -385,8 +372,6 @@ class TestLiveMediaPhase(PungiTestCase): self.assertValidConfig(compose.conf) - resolve_git_url.return_value = 'resolved' - phase = LiveMediaPhase(compose) phase.run() @@ -397,7 +382,7 @@ class TestLiveMediaPhase(PungiTestCase): { 'arches': ['x86_64'], 'ksfile': 'file.ks', - 'ksurl': 'resolved', + 'ksurl': 'git://example.com/repo.git#BEEFCAFE', 'ksversion': '24', 'name': 'Fedora Server Live', 'release': '20151203.t.0', diff --git a/tests/test_osbs_phase.py b/tests/test_osbs_phase.py index 5a212af1..c6b360a1 100644 --- a/tests/test_osbs_phase.py +++ b/tests/test_osbs_phase.py @@ -178,8 +178,7 @@ class OSBSThreadTest(helpers.PungiTestCase): ] }) - def _setupMock(self, KojiWrapper, resolve_git_url, scratch=False): - resolve_git_url.return_value = 'git://example.com/repo?#BEEFCAFE' + def _setupMock(self, KojiWrapper, scratch=False): self.wrapper = KojiWrapper.return_value self.wrapper.koji_proxy.buildContainer.return_value = 12345 if scratch: @@ -241,7 +240,7 @@ class OSBSThreadTest(helpers.PungiTestCase): config['osbs'] = { '^Server$': cfg } - self.assertEqual(([], []), checks.validate(config)) + self.assertEqual(([], []), checks.validate(config, offline=True)) def _assertConfigMissing(self, cfg, key): config = copy.deepcopy(self.compose.conf) @@ -250,17 +249,16 @@ class OSBSThreadTest(helpers.PungiTestCase): } self.assertEqual( (['Failed validation in osbs.^Server$: %r is not valid under any of the given schemas' % cfg], []), - checks.validate(config)) + checks.validate(config, offline=True)) - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_minimal_run(self, KojiWrapper, resolve_git_url): + def test_minimal_run(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', 'git_branch': 'f24-docker', } - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) self._assertConfigCorrect(cfg) self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) @@ -269,16 +267,15 @@ class OSBSThreadTest(helpers.PungiTestCase): self._assertCorrectMetadata() self._assertRepoFile() - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_run_failable(self, KojiWrapper, resolve_git_url): + def test_run_failable(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', 'git_branch': 'f24-docker', 'failable': ['*'] } - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) self._assertConfigCorrect(cfg) self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) @@ -287,17 +284,16 @@ class OSBSThreadTest(helpers.PungiTestCase): self._assertCorrectMetadata() self._assertRepoFile() - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_run_with_more_args(self, KojiWrapper, resolve_git_url): + def test_run_with_more_args(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', 'git_branch': 'f24-docker', 'name': 'my-name', 'version': '1.0', } - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) self._assertConfigCorrect(cfg) self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) @@ -306,18 +302,17 @@ class OSBSThreadTest(helpers.PungiTestCase): self._assertCorrectMetadata() self._assertRepoFile() - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_run_with_extra_repos(self, KojiWrapper, resolve_git_url): + def test_run_with_extra_repos(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', 'git_branch': 'f24-docker', 'name': 'my-name', 'version': '1.0', 'repo': ['Everything', 'http://pkgs.example.com/my.repo'] } - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) self._assertConfigCorrect(cfg) self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) @@ -336,11 +331,10 @@ class OSBSThreadTest(helpers.PungiTestCase): self._assertCorrectMetadata() self._assertRepoFile(['Server', 'Everything']) - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_run_with_extra_repos_in_list(self, KojiWrapper, resolve_git_url): + def test_run_with_extra_repos_in_list(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', 'git_branch': 'f24-docker', 'name': 'my-name', @@ -348,7 +342,7 @@ class OSBSThreadTest(helpers.PungiTestCase): 'repo': ['Everything', 'Client', 'http://pkgs.example.com/my.repo'], } self._assertConfigCorrect(cfg) - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) @@ -367,12 +361,11 @@ class OSBSThreadTest(helpers.PungiTestCase): self._assertCorrectMetadata() self._assertRepoFile(['Server', 'Everything', 'Client']) - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_run_with_gpgkey_enabled(self, KojiWrapper, resolve_git_url): + def test_run_with_gpgkey_enabled(self, KojiWrapper): gpgkey = 'file:///etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-release' cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', 'git_branch': 'f24-docker', 'name': 'my-name', @@ -381,17 +374,16 @@ class OSBSThreadTest(helpers.PungiTestCase): 'gpgkey': gpgkey, } self._assertConfigCorrect(cfg) - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) self._assertRepoFile(['Server', 'Everything', 'Client'], gpgkey=gpgkey) - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_run_with_extra_repos_missing_variant(self, KojiWrapper, resolve_git_url): + def test_run_with_extra_repos_missing_variant(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', 'git_branch': 'f24-docker', 'name': 'my-name', @@ -399,7 +391,7 @@ class OSBSThreadTest(helpers.PungiTestCase): 'repo': 'Gold', } self._assertConfigCorrect(cfg) - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) with self.assertRaises(RuntimeError) as ctx: self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) @@ -416,7 +408,7 @@ class OSBSThreadTest(helpers.PungiTestCase): def test_run_with_missing_target(self): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'git_branch': 'f24-docker', 'name': 'my-name', } @@ -424,21 +416,20 @@ class OSBSThreadTest(helpers.PungiTestCase): def test_run_with_missing_git_branch(self): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', } self._assertConfigMissing(cfg, 'git_branch') - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_failing_task(self, KojiWrapper, resolve_git_url): + def test_failing_task(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'fedora-24-docker-candidate', 'git_branch': 'f24-docker', } self._assertConfigCorrect(cfg) - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) self.wrapper.watch_task.return_value = 1 with self.assertRaises(RuntimeError) as ctx: @@ -446,31 +437,29 @@ class OSBSThreadTest(helpers.PungiTestCase): self.assertRegexpMatches(str(ctx.exception), r"task 12345 failed: see .+ for details") - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_failing_task_with_failable(self, KojiWrapper, resolve_git_url): + def test_failing_task_with_failable(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'fedora-24-docker-candidate', 'git_branch': 'f24-docker', 'failable': ['*'] } self._assertConfigCorrect(cfg) - self._setupMock(KojiWrapper, resolve_git_url) + self._setupMock(KojiWrapper) self.wrapper.watch_task.return_value = 1 self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) - @mock.patch('pungi.util.resolve_git_url') @mock.patch('pungi.phases.osbs.kojiwrapper.KojiWrapper') - def test_scratch_metadata(self, KojiWrapper, resolve_git_url): + def test_scratch_metadata(self, KojiWrapper): cfg = { - 'url': 'git://example.com/repo?#HEAD', + 'url': 'git://example.com/repo?#BEEFCAFE', 'target': 'f24-docker-candidate', 'git_branch': 'f24-docker', 'scratch': True, } - self._setupMock(KojiWrapper, resolve_git_url, scratch=True) + self._setupMock(KojiWrapper, scratch=True) self._assertConfigCorrect(cfg) self.t.process((self.compose, self.compose.variants['Server'], cfg), 1) diff --git a/tests/test_phase_base.py b/tests/test_phase_base.py index ba80afe3..7ac0f522 100644 --- a/tests/test_phase_base.py +++ b/tests/test_phase_base.py @@ -13,65 +13,8 @@ import time sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) -from pungi.phases import base, weaver -from tests.helpers import DummyCompose, PungiTestCase, boom - - -class Phase1(base.ImageConfigMixin, base.PhaseBase): - name = 'phase1' - - -class Phase2(base.ImageConfigMixin, base.PhaseBase): - name = 'phase2' - - -class Phase3(base.ImageConfigMixin, base.PhaseBase): - name = 'phase3' - - -class DummyResolver(object): - def __init__(self): - self.num = 0 - - def __call__(self, url): - self.num += 1 - return url.replace('HEAD', 'RES' + str(self.num)) - - -class ImageConfigMixinTestCase(PungiTestCase): - - @mock.patch('pungi.util.resolve_git_url', new_callable=DummyResolver) - def test_git_url_resolved_once(self, resolve_git_url): - compose = DummyCompose(self.topdir, { - 'global_ksurl': 'git://example.com/repo.git?#HEAD', - 'phase1_ksurl': 'git://example.com/another.git?#HEAD', - }) - - p1 = Phase1(compose) - p2 = Phase2(compose) - p3 = Phase3(compose) - - self.assertEqual(p1.get_ksurl({}), - 'git://example.com/another.git?#RES1') - # Phase-level setting retrieved second time. - self.assertEqual(p1.get_ksurl({}), - 'git://example.com/another.git?#RES1') - - self.assertEqual(p2.get_ksurl({}), - 'git://example.com/repo.git?#RES2') - # Global setting retrieved again from same phase. - self.assertEqual(p2.get_ksurl({}), - 'git://example.com/repo.git?#RES2') - - # Global setting retrieved from another phase. - self.assertEqual(p3.get_ksurl({}), - 'git://example.com/repo.git?#RES2') - - # Local setting ignores global ones. - self.assertEqual(p3.get_ksurl({'ksurl': 'git://example.com/more.git?#HEAD'}), - 'git://example.com/more.git?#RES3') - - self.assertEqual(resolve_git_url.num, 3, 'Resolver was not called three times') +from pungi.phases import weaver +from tests.helpers import DummyCompose, boom class TestWeaver(unittest.TestCase): diff --git a/tests/test_util.py b/tests/test_util.py index 279c69cb..04e3c883 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -18,7 +18,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) from pungi import compose from pungi import util -from tests.helpers import touch, PungiTestCase +from tests.helpers import touch, PungiTestCase, mk_boom class TestGitRefResolver(unittest.TestCase): @@ -134,6 +134,17 @@ class TestGitRefResolver(unittest.TestCase): self.assertEqual(resolver(url2), "2") self.assertEqual(mock_resolve.call_args_list, [mock.call(url1), mock.call(url2)]) + @mock.patch("pungi.util.resolve_git_url") + def test_resolver_caches_failure(self, mock_resolve): + url = "http://example.com/repo.git#HEAD" + mock_resolve.side_effect = mk_boom(util.GitUrlResolveError, "failed") + resolver = util.GitUrlResolver() + with self.assertRaises(util.GitUrlResolveError): + resolver(url) + with self.assertRaises(util.GitUrlResolveError): + resolver(url) + self.assertEqual(mock_resolve.call_args_list, [mock.call(url)]) + class TestGetVariantData(unittest.TestCase): def test_get_simple(self):