PR#1405: Allow building compose with scratch builds defined by `pkgset_koji_scratch_tasks`. - pungi

pungi

#1405 Allow building compose with scratch builds defined by `pkgset_koji_scratch_tasks`.

Merged 3 years ago by lsedlar. Opened 3 years ago by jkaluza.

jkaluza/pungi scratch-builds into master

Move test for unsigned packages with pkgset_koji_scratch_tasks to PkgsetPhase class.

Jan Kaluza • 3 years ago

e35c250

Allow building compose with scratch builds defined by `pkgset_koji_scratch_tasks`.

Jan Kaluza • 3 years ago

4a15d13

doc/configuration.rst

file modified

		`@@ -522,6 +522,13 @@`
		`pkgset_koji_builds`
		`(str\|[str]) -- extra build(s) to include in a package set defined as NVRs.`

		`+ pkgset_koji_scratch_tasks`
		`+ (str\|[str]) -- RPM scratch build task(s) to include in a package set,`
		+ defined as task IDs. This option can be used only when ``compose_type``
		+ is set to ``test``. The RPM still needs to have higher NVR than any
		`+ other RPM with the same name comming from other sources in order to`
		`+ appear in the resulting compose.`
		`+`
		`pkgset_koji_module_tag`
		`(str\|[str]) -- tags to read module from. This option works similarly to`
		`listing tags in variants XML. If tags are specified and variants XML`

pungi/checks.py

file modified

		`@@ -778,6 +778,7 @@`
		`"koji_event": {"type": "number"},`
		`"pkgset_koji_tag": {"$ref": "#/definitions/strings"},`
		`"pkgset_koji_builds": {"$ref": "#/definitions/strings"},`
		`+ "pkgset_koji_scratch_tasks": {"$ref": "#/definitions/strings"},`
		`"pkgset_koji_module_tag": {"$ref": "#/definitions/strings", "default": []},`
		`"pkgset_koji_inherit": {"type": "boolean", "default": True},`
		`"pkgset_koji_inherit_modules": {"type": "boolean", "default": False},`

pungi/phases/pkgset/__init__.py

file modified

+12 -2

		`@@ -22,8 +22,9 @@`

		`name = "pkgset"`

		`- def __init__(self, args, *kwargs):`
		`- super(PkgsetPhase, self).__init__(args, *kwargs)`
		`+ def __init__(self, compose, args, *kwargs):`
		`+ super(PkgsetPhase, self).__init__(compose, args, *kwargs)`
		`+ self.compose = compose`
		`self.package_sets = []`
		`self.path_prefix = None`

		`@@ -36,3 +37,12 @@`
		`container = PkgsetSourceContainer()`
		`SourceClass = container[pkgset_source]`
		`self.package_sets, self.path_prefix = SourceClass(self.compose)()`
		`+`
		`+ def validate(self):`
		`+ extra_tasks = self.compose.conf.get("pkgset_koji_scratch_tasks", None)`
		`+ sigkeys = tuple(self.compose.conf["sigkeys"] or [None])`
		`+ if extra_tasks is not None and None not in sigkeys and "" not in sigkeys:`
		`+ raise ValueError(`
		`+ "Unsigned packages must be allowed to use the "`
		`+ '"pkgset_koji_scratch_tasks" option'`
		`+ )`

pungi/phases/pkgset/pkgsets.py

file modified

+69 -2

		`@@ -328,6 +328,7 @@`
		`populate_only_packages=False,`
		`cache_region=None,`
		`extra_builds=None,`
		`+ extra_tasks=None,`
		`):`
		`"""`
		`Creates new KojiPackageSet.`
		`@@ -357,6 +358,9 @@`
		`again.`
		`:param list extra_builds: Extra builds NVRs to get from Koji and include`
		`in the package set.`
		`+ :param list extra_tasks: Extra RPMs defined as Koji task IDs to get from Koji`
		`+ and include in the package set. Useful when building testing compose`
		`+ with RPM scratch builds.`
		`"""`
		`super(KojiPackageSet, self).__init__(`
		`name,`
		`@@ -371,6 +375,7 @@`
		`self.populate_only_packages = populate_only_packages`
		`self.cache_region = cache_region`
		`self.extra_builds = extra_builds or []`
		`+ self.extra_tasks = extra_tasks or []`
		`self.reuse = None`

		`def __getstate__(self):`
		`@@ -413,6 +418,53 @@`
		`rpms += rpms_in_build`
		`return rpms, builds`

		`+ def get_extra_rpms_from_tasks(self):`
		`+ """`
		`+ Returns manually constructed RPM infos from the Koji tasks defined`
		+ in `self.extra_tasks`.
		`+`
		`+ :rtype: list`
		`+ :return: List with RPM infos defined as dicts with following keys:`
		`+ - name, version, release, arch, src - as returned by parse_nvra.`
		`+ - path_from_task - Full path to RPM on /mnt/koji.`
		`+ - build_id - Always set to None.`
		`+ """`
		`+ if not self.extra_tasks:`
		`+ return []`
		`+`
		`+ # Get the IDs of children tasks - these are the tasks containing`
		`+ # the resulting RPMs.`
		`+ children_tasks = self.koji_wrapper.retrying_multicall_map(`
		`+ self.koji_proxy,`
		`+ self.koji_proxy.getTaskChildren,`
		`+ list_of_args=self.extra_tasks,`
		`+ )`
		`+ children_task_ids = []`
		`+ for tasks in children_tasks:`
		`+ children_task_ids += [t["id"] for t in tasks]`
		`+`
		`+ # Get the results of these children tasks.`
		`+ results = self.koji_wrapper.retrying_multicall_map(`
		`+ self.koji_proxy,`
		`+ self.koji_proxy.getTaskResult,`
		`+ list_of_args=children_task_ids,`
		`+ )`
		`+ rpms = []`
		`+ for result in results:`
		`+ rpms += result.get("rpms", [])`
		`+ rpms += result.get("srpms", [])`
		`+`
		`+ rpm_infos = []`
		`+ for rpm in rpms:`
		`+ rpm_info = kobo.rpmlib.parse_nvra(os.path.basename(rpm))`
		`+ rpm_info["path_from_task"] = os.path.join(`
		`+ self.koji_wrapper.koji_module.pathinfo.work(), rpm`
		`+ )`
		`+ rpm_info["build_id"] = None`
		`+ rpm_infos.append(rpm_info)`
		`+`
		`+ return rpm_infos`
		`+`
		`def get_latest_rpms(self, tag, event, inherit=True):`
		`if not tag:`
		`return [], []`
		`@@ -443,6 +495,12 @@`

		`def get_package_path(self, queue_item):`
		`rpm_info, build_info = queue_item`
		`+`
		`+ # Check if this RPM is comming from scratch task. In this case, we already`
		`+ # know the path.`
		`+ if "path_from_task" in rpm_info:`
		`+ return rpm_info["path_from_task"]`
		`+`
		`pathinfo = self.koji_wrapper.koji_module.pathinfo`
		`paths = []`
		`for sigkey in self.sigkey_ordering:`
		`@@ -521,6 +579,9 @@`
		`else:`
		`builds_by_id.setdefault(build_id, build_info)`

		`+ # Get extra RPMs from tasks.`
		`+ rpms += self.get_extra_rpms_from_tasks()`
		`+`
		`skipped_arches = []`
		`skipped_packages_count = 0`
		`# We need to process binary packages first, and then source packages.`
		`@@ -560,14 +621,20 @@`
		`skipped_packages_count += 1`
		`continue`

		`- build_info = builds_by_id[rpm_info["build_id"]]`
		`+ build_info = builds_by_id.get(rpm_info["build_id"], None)`
		`if _is_src(rpm_info):`
		`result_srpms.append((rpm_info, build_info))`
		`else:`
		`result_rpms.append((rpm_info, build_info))`
		`if self.populate_only_packages and self.packages:`
		`# Only add the package if we already have some whitelist.`
		`- self.packages.add(build_info["name"])`
		`+ if build_info:`
		`+ self.packages.add(build_info["name"])`
		`+ else:`
		`+ # We have no build info and therefore no Koji package name,`
		`+ # we can only guess that the Koji package name would be the same`
		`+ # one as the RPM name.`
		`+ self.packages.add(rpm_info["name"])`

		`if skipped_packages_count:`
		`self.log_debug(`

pungi/phases/pkgset/sources/source_koji.py

file modified

		`@@ -641,8 +641,10 @@`
		`compose.log_info("Loading package set for tag %s", compose_tag)`
		`if compose_tag in pkgset_koji_tags:`
		`extra_builds = force_list(compose.conf.get("pkgset_koji_builds", []))`
		`+ extra_tasks = force_list(compose.conf.get("pkgset_koji_scratch_tasks", []))`
		`else:`
		`extra_builds = []`
		`+ extra_tasks = []`

		`pkgset = pungi.phases.pkgset.pkgsets.KojiPackageSet(`
		`compose_tag,`
		`@@ -655,6 +657,7 @@`
		`populate_only_packages=populate_only_packages_to_gather,`
		`cache_region=compose.cache_region,`
		`extra_builds=extra_builds,`
		`+ extra_tasks=extra_tasks,`
		`)`

		`# Check if we have cache for this tag from previous compose. If so, use`

pungi/scripts/pungi_koji.py

file modified

		`@@ -232,6 +232,12 @@`
		`if compose_type == "production" and not opts.label and not opts.no_label:`
		`abort("must specify label for a production compose")`

		`+ if (`
		`+ compose_type != "test"`
		`+ and conf.get("pkgset_koji_scratch_tasks", None) is not None`
		`+ ):`
		`+ abort('pkgset_koji_scratch_tasks can be used only for "test" compose type')`
		`+`
		`# check if all requirements are met`
		`import pungi.checks`

tests/test_pkgset_phase.py

file added

+24

		`@@ -0,0 +1,24 @@`
		`+ # -- coding: utf-8 --`
		`+ from pungi.phases import pkgset`
		`+ from tests import helpers`
		`+`
		`+`
		`+ class TestPkgsetPhase(helpers.PungiTestCase):`
		`+ def test_validates_pkgset_koji_scratch_tasks_only_signed(self):`
		`+ cfg = {"pkgset_koji_scratch_tasks": ["123"], "sigkeys": ["sigkey"]}`
		`+ compose = helpers.DummyCompose(self.topdir, cfg)`
		`+ phase = pkgset.PkgsetPhase(compose)`
		`+`
		`+ with self.assertRaises(ValueError) as ctx:`
		`+ phase.validate()`
		`+ self.assertIn("Unsigned packages must be allowed", str(ctx.exception))`
		`+`
		`+ def test_validates_pkgset_koji_scratch_tasks_unsigned(self):`
		`+ for unsigned_obj in ["", None]:`
		`+ cfg = {`
		`+ "pkgset_koji_scratch_tasks": ["123"],`
		`+ "sigkeys": ["sigkey", unsigned_obj],`
		`+ }`
		`+ compose = helpers.DummyCompose(self.topdir, cfg)`
		`+ phase = pkgset.PkgsetPhase(compose)`
		`+ phase.validate()`

tests/test_pkgset_pkgsets.py

file modified

+70

		`@@ -33,6 +33,9 @@`
		`def rpm(self, rpm_info):`
		`return os.path.join("rpms", self.get_filename(rpm_info))`

		`+ def work(self):`
		`+ return "work"`
		`+`

		`class MockFile(object):`
		`def __init__(self, path):`
		`@@ -380,6 +383,73 @@`
		`},`
		`)`

		`+ def test_get_extra_rpms_from_tasks(self):`
		`+ pkgset = pkgsets.KojiPackageSet(`
		`+ "pkgset",`
		`+ self.koji_wrapper,`
		`+ [None],`
		`+ arches=["x86_64"],`
		`+ extra_tasks=["123", "456"],`
		`+ )`
		`+ children_tasks = [[{"id": 1}, {"id": 2}], [{"id": 3}, {"id": 4}]]`
		`+ task_results = [`
		`+ {`
		`+ "logs": [`
		`+ "tasks/root.log",`
		`+ "tasks/hw_info.log",`
		`+ "tasks/state.log",`
		`+ "tasks/build.log",`
		`+ "tasks/mock_output.log",`
		`+ "tasks/noarch_rpmdiff.json",`
		`+ ],`
		`+ "rpms": ["tasks/pungi-4.1.39-5.f30.noarch.rpm"],`
		`+ "srpms": ["tasks/pungi-4.1.39-5.f30.src.rpm"],`
		`+ },`
		`+ {`
		`+ "logs": [`
		`+ "tasks/5478/29155478/root.log",`
		`+ "tasks/5478/29155478/hw_info.log",`
		`+ "tasks/5478/29155478/state.log",`
		`+ "tasks/5478/29155478/build.log",`
		`+ ],`
		`+ "source": {`
		`+ "source": "pungi-4.1.39-5.f30.src.rpm",`
		`+ "url": "pungi-4.1.39-5.f30.src.rpm",`
		`+ },`
		`+ "srpm": "tasks/5478/29155478/pungi-4.1.39-5.f30.src.rpm",`
		`+ },`
		`+ ]`
		`+ self.koji_wrapper.retrying_multicall_map.side_effect = [`
		`+ children_tasks,`
		`+ task_results,`
		`+ ]`
		`+`
		`+ expected_rpms = [`
		`+ {`
		`+ "arch": "noarch",`
		`+ "build_id": None,`
		`+ "epoch": "",`
		`+ "name": "pungi",`
		`+ "path_from_task": "work/tasks/pungi-4.1.39-5.f30.noarch.rpm",`
		`+ "release": "5.f30",`
		`+ "src": False,`
		`+ "version": "4.1.39",`
		`+ },`
		`+ {`
		`+ "arch": "src",`
		`+ "build_id": None,`
		`+ "epoch": "",`
		`+ "name": "pungi",`
		`+ "path_from_task": "work/tasks/pungi-4.1.39-5.f30.src.rpm",`
		`+ "release": "5.f30",`
		`+ "src": True,`
		`+ "version": "4.1.39",`
		`+ },`
		`+ ]`
		`+`
		`+ rpms = pkgset.get_extra_rpms_from_tasks()`
		`+ self.assertEqual(rpms, expected_rpms)`
		`+`
		`def test_get_latest_rpms_cache(self):`
		`self._touch_files(`
		`[`

jkaluza commented 3 years ago

Signed-off-by: Jan Kaluza jkaluza@redhat.com

jkaluza commented 3 years ago

Example of such compose including "scratch" attr build: http://10.0.145.16/composes/odcs-355/

Edited 3 years ago by jkaluza

lsedlar commented 3 years ago

In general this looks good to me, though I would prefer to have an explicit check and only allow including scratch builds for composes with type test.

lsedlar commented 3 years ago

FYI there's a related ticket: https://pagure.io/pungi/issue/51
When a scratch build is included, but the tag already contains a real build with higher version, the scratch build will be silently ignored. I'm not sure how big of a problem that is. It's not difficult to ensure higher NVR for the test.

rebased onto a8fb43f61ae2367acb74d45a5466b6156f27cd18

3 years ago

rebased onto d26180fff7c742d0de668b02306ab9bd1384d12e

3 years ago

jkaluza commented 3 years ago

@lsedlar, I've added tests and documentation (also mentioned higher NVR there).

lsedlar commented on line 95 of pungi/phases/pkgset/pkgsets.py 3 years ago

This should probably move to validate method directly in PkgsetPhase class, and report invalid config if unsigned packages are not allowed but a task is specified. Current implementation would just ignore the tasks, which might be confusing to users. Being explicit and rejecting such configuration might be more user friendly.

1 new commit added