Move resolving git reference to config validation

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ář <lsedlar@redhat.com>

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)

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)
+    DefaultValidator = _extend_with_default_and_alias(
-    validator = DefaultValidator(schema,
+        jsonschema.Draft4Validator, offline=offline
-                                 {'array': (tuple, list),
+    )
-                                  'regex': six.string_types})
+    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:
+            # Resolve git URL references to actual commit hashes
-            for error in errors:
+            if subschema.get("type") == "url" and property in instance:
-                yield error
+                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 _hook_errors(properties, instance, schema):
-            for error in errors:
+            yield error
-                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 _hook_errors(properties, instance, schema):
-            for error in errors:
+            yield error
-                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,
+        validator_class,
-                          "deprecated": error_on_deprecated,
+        {
-                          "type": validate_regex_type,
+            "properties": properties_validator,
-                          "required": _validate_required,
+            "deprecated": error_on_deprecated,
-                          "additionalProperties": _validate_additional_properties,
+            "type": validate_regex_type,
-                          "anyOf": _validate_any_of},
+            "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"},

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 (
-            return util.resolve_git_url(cfg['ksurl'])
+            cfg.get("ksurl")
-        if '%s_ksurl' % self.name in self.compose.conf:
+            or self.compose.conf.get("%s_ksurl" % self.name)
-            return self.phase_ksurl
+            or self.compose.conf.get("global_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']
 
 
 class PhaseLoggerMixin(object):

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)

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/<branch> 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'
+        raise GitUrlResolveError(
-                           % url)
+            "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]
 
 

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 = {

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):

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(

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, {

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, ThreadPool):
-    def test_spin_appliance(self, resolve_git_url, 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, ThreadPool):
-    def test_spin_appliance_phase_global_settings(self, resolve_git_url, 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, ThreadPool):
-    def test_spin_appliance_global_settings(self, resolve_git_url, 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):

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',

tests/test_osbs_phase.py

@@ -178,8 +178,7 @@ class OSBSThreadTest(helpers.PungiTestCase):
             ]
         })
 
-    def _setupMock(self, KojiWrapper, resolve_git_url, scratch=False):
+    def _setupMock(self, KojiWrapper, scratch=False):
-        resolve_git_url.return_value = 'git://example.com/repo?#BEEFCAFE'
         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)

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 pungi.phases import weaver
-from tests.helpers import DummyCompose, PungiTestCase, boom
+from tests.helpers import DummyCompose, 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')
 
 
 class TestWeaver(unittest.TestCase):

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):