PR#213: Respect the module components buildorder - copr/copr

copr / copr

#213 Respect the module components buildorder

Merged 6 years ago by frostyx. Opened 6 years ago by frostyx.

copr/ frostyx/copr module-batches into master

[frontend] use defaultdict instead of conditionaly creating a key

Jakub Kadlčík • 6 years ago

3c93c8b

[frontend] get_build_batches can be classmethod

Jakub Kadlčík • 6 years ago

911c340

[frontend] the build was actually not assigned to the batch

Jakub Kadlčík • 6 years ago

63af247

[frontend] implement the module buildorder logic

Jakub Kadlčík • 6 years ago

ce5e114

frontend/coprs_frontend/coprs/logic/modules_logic.py

file modified

+10 -3

		`@@ -4,6 +4,7 @@`
		`import json`
		`import requests`
		`import modulemd`
		`+ from collections import defaultdict`
		`from sqlalchemy import and_`
		`from datetime import datetime`
		`from coprs import models`
		`@@ -94,14 +95,18 @@`
		`self.add_builds(self.modulemd.components.rpms, module)`
		`return module`

		`- def get_build_batches(self, rpms):`
		`+ @classmethod`
		`+ def get_build_batches(cls, rpms):`
		`"""`
		`Determines Which component should be built in which batch. Returns an ordered list of grouped components,`
		`first group of components should be built as a first batch, second as second and so on.`
		`- Particular components groups are represented by lists and can by built in a random order within the batch.`
		`+ Particular components groups are represented by dicts and can by built in a random order within the batch.`
		`:return: list of lists`
		`"""`
		`- return [rpms]`
		`+ batches = defaultdict(dict)`
		`+ for pkgname, rpm in rpms.items():`
		`+ batches[rpm.buildorder][pkgname] = rpm`
		`+ return [batches[number] for number in sorted(batches.keys())]`

		`def add_builds(self, rpms, module):`
		`for group in self.get_build_batches(rpms):`
		`@@ -111,8 +116,10 @@`
		`clone_url = self.get_clone_url(pkgname, rpm)`
		`build = builds_logic.BuildsLogic.create_new_from_scm(self.user, self.copr, scm_type="git",`
		`clone_url=clone_url, committish=rpm.ref)`
		`+ build.batch = batch`
		`build.batch_id = batch.id`
		`build.module_id = module.id`
		`+ db.session.add(build)`

		`def get_clone_url(self, pkgname, rpm):`
		`return rpm.repository if rpm.repository else self.default_distgit.format(pkgname=pkgname)`

frontend/coprs_frontend/tests/test_logic/test_modules_logic.py

file modified

+34 -1

		`@@ -2,7 +2,40 @@`
		`from munch import Munch`
		`from mock import patch, ANY`
		`from tests.coprs_test_case import CoprsTestCase`
		`- from coprs.logic.modules_logic import ModulemdGenerator, MBSResponse, MBSProxy`
		`+ from coprs.logic.modules_logic import ModuleBuildFacade, ModulemdGenerator, MBSResponse, MBSProxy`
		`+ from modulemd.components.rpm import ModuleComponentRPM`
		`+`
		`+`
		`+ class TestModuleBuildFacade(CoprsTestCase):`
		`+ def test_get_build_batches(self):`
		`+ pkg1 = ModuleComponentRPM("pkg1", "rationale")`
		`+ pkg2 = ModuleComponentRPM("pkg2", "rationale")`
		`+ pkg3 = ModuleComponentRPM("pkg3", "rationale", buildorder=1)`
		`+ pkg4 = ModuleComponentRPM("pkg4", "rationale", buildorder=-20)`
		`+ pkg5 = ModuleComponentRPM("pkg5", "rationale", buildorder=50)`
		`+`
		`+ # Test trivial usage`
		`+ assert ModuleBuildFacade.get_build_batches({}) == []`
		`+`
		`+ # Test multiple components with same buildorder`
		`+ rpms = {"pkg1": pkg1, "pkg2": pkg2}`
		`+ expected_batches = [{"pkg1": pkg1, "pkg2": pkg2}]`
		`+ assert ModuleBuildFacade.get_build_batches(rpms) == expected_batches`
		`+`
		`+ # Test component with buildorder`
		`+ rpms = {"pkg3": pkg3, "pkg1": pkg1, "pkg2": pkg2}`
		`+ expected_batches = [{"pkg1": pkg1, "pkg2": pkg2}, {"pkg3": pkg3}]`
		`+ assert ModuleBuildFacade.get_build_batches(rpms) == expected_batches`
		`+`
		`+ # Test negative buildorder`
		`+ rpms = {"pkg1": pkg1, "pkg2": pkg2, "pkg4": pkg4}`
		`+ expected_batches = [{"pkg4": pkg4}, {"pkg1": pkg1, "pkg2": pkg2}]`
		`+ assert ModuleBuildFacade.get_build_batches(rpms) == expected_batches`
		`+`
		`+ # Test various buildorders at once`
		`+ rpms = {"pkg5": pkg5, "pkg3": pkg3, "pkg2": pkg2, "pkg4": pkg4, "pkg1":pkg1}`
		`+ expected_batches = [{"pkg4": pkg4}, {"pkg1": pkg1, "pkg2": pkg2}, {"pkg3": pkg3}, {"pkg5": pkg5}]`
		`+ assert ModuleBuildFacade.get_build_batches(rpms) == expected_batches`


		`class TestModulemdGenerator(CoprsTestCase):`

frostyx commented 6 years ago

Currently, we put all module packages into one batch. This PR modifies the behavior and separate builds into multiple batches based on buildorder.
https://pagure.io/modulemd/blob/ade28f3f3b39fcddcb626ca915df1a6ce35c14fd/f/spec.yaml#_222

The code was ready for it, I just (re)implemented the get_build_batches(...) to actually do something. You can see how it behaves in the tests.

clime commented 6 years ago

There are some minor things in the code that could be done better:
get_build_batches should be @classmethod. And defaultdict (from python collections module) could be used. Apart from that it looks nice.

I still have doubts about practical use of it (apart from batch building from a script, which is definitely a useful feature but could have been done in a much more simpler manner) but that's another story for now.

2 new commits added