PR#979: [WIP] Resolve runtime dependencies of MSE generated builds to get the possible platform streams. - fm-orchestrator

fm-orchestrator

#979 [WIP] Resolve runtime dependencies of MSE generated builds to get the possible platform streams.

Closed 5 years ago by jkaluza. Opened 5 years ago by jkaluza.

jkaluza/fm-orchestrator 974 into master

Resolve runtime dependencies of MSE generated builds to get the possible platform streams.

Jan Kaluza • 5 years ago

a710682

module_build_service/builder/KojiContentGenerator.py

file modified

+22 -24

		`@@ -38,7 +38,7 @@`
		`from six import text_type`
		`import koji`

		`- from module_build_service import log, build_logs`
		`+ from module_build_service import log, build_logs, conf`

		`logging.basicConfig(level=logging.DEBUG)`

		`@@ -379,34 +379,32 @@`
		`"""`
		`session = get_session(self.config, self.owner)`

		`- tag_name = self.module.cg_build_koji_tag`
		`- if not tag_name:`
		`- log.info("%r: Not tagging Content Generator build, no "`
		`- "cg_build_koji_tag set", self.module)`
		`- return`
		`-`
		`- tag_names_to_try = [tag_name, self.config.koji_cg_default_build_tag]`
		`- for tag in tag_names_to_try:`
		`- log.info("Trying %s", tag)`
		`- tag_info = session.getTag(tag)`
		`- if tag_info:`
		`- break`
		`-`
		`- log.info("%r: Tag %s not found in Koji, trying next one.",`
		`- self.module, tag)`
		`+ primary_tags = []`

		`- if not tag_info:`
		`- log.warn("%r:, Not tagging Content Generator build, no "`
		`- "available tag found, tried %r", self.module,`
		`- tag_names_to_try)`
		`- return`
		`+ mmd = self.module.mmd()`
		`+ xmd_mbs = mmd.get_xmd().get('mbs')`
		`+ if xmd_mbs and 'base_modules' in xmd_mbs.keys():`
		`+ for base_module_ns in xmd_mbs["base_modules"]:`
		`+ name, stream = base_module_ns.split(":")`
		`+ tag = conf.koji_cg_build_tag_template.format(stream)`
		`+ primary_tags.append(tag)`

		`build = self._get_build()`
		`nvr = "%s-%s-%s" % (build["name"], build["version"], build["release"])`

		`- log.info("Content generator build %s will be tagged as %s in "`
		`- "Koji", nvr, tag)`
		`- session.tagBuild(tag_info["id"], nvr)`
		`+ for tags in [primary_tags, [conf.koji_cg_default_build_tag]]:`
		`+ tagged = False`
		`+ for tag in tags:`
		`+ tag_info = session.getTag(tag)`
		`+ if tag_info:`
		`+ log.info("Content generator build %s will be tagged as %s in "`
		`+ "Koji", nvr, tag)`
		`+ session.tagBuild(tag_info["id"], nvr)`
		`+ tagged = True`
		`+ else:`
		`+ log.warn("Tag %s does not exist in Koji", tag)`
		`+ if tagged:`
		`+ break`

		`def koji_import(self):`
		`"""This method imports given module into the configured koji instance as`

module_build_service/mmd_resolver.py

file modified

+2 -2

		`@@ -166,8 +166,8 @@`
		`# Source solvables have Req: (X and Y and Z)`
		`# Binary solvables have Req: ((X and Y) or (X and Z))`
		`# They do use "or" within "and", so simple string split won't work for binary packages.`
		`- if src.arch != "src":`
		`- raise NotImplementedError`
		`+ #if src.arch != "src":`
mprahl commented 5 years ago Could you please explain this change?
jkaluza commented 5 years ago This currently waits for Igor to rewrite.
		`+ #raise NotImplementedError`
		`deps = str(requires).split(" and ")`
		`if len(deps) > 1:`
		`deps[0] = deps[0][1:]`

module_build_service/scheduler/handlers/modules.py

file modified

+2 -17

		`@@ -218,7 +218,6 @@`
		`are going to build. We use private method here to allow "retry"`
		`on failure.`
		`"""`
		`- cg_build_koji_tag = conf.koji_cg_default_build_tag`
		`if conf.system not in ['koji', 'test']:`
		`# In case of non-koji backend, we want to get the dependencies`
		`# of the local module build based on ModuleMetadata, because the`
		`@@ -239,23 +238,13 @@`
		`build.name, build.stream, build.version, build.context, strict=True)`
		`dependencies = set(deps_dict.keys())`

		`- # Find out the name of Koji tag to which the module's Content`
		`- # Generator build should be tagged once the build finishes.`
		`- module_names_streams = {mmd.get_name(): mmd.get_stream()`
		`- for mmd in deps_dict.values()}`
		`- for base_module_name in conf.base_module_names:`
		`- if base_module_name in module_names_streams:`
		`- cg_build_koji_tag = conf.koji_cg_build_tag_template.format(`
		`- module_names_streams[base_module_name])`
		`- break`
		`-`
		`log.info('Getting tag for {0}'.format(nsvc))`
		`tag = generate_koji_tag(build.name, build.stream, build.version, build.context)`

		`- return dependencies, tag, cg_build_koji_tag`
		`+ return dependencies, tag`

		`try:`
		`- dependencies, tag, cg_build_koji_tag = _get_deps_and_tag()`
		`+ dependencies, tag = _get_deps_and_tag()`
		`except ValueError:`
		`reason = "Failed to get module info from MBS. Max retries reached."`
		`log.exception(reason)`
		`@@ -271,10 +260,6 @@`
		`log.debug("Assigning koji tag=%s to module build" % tag)`
		`build.koji_tag = tag`

		`- log.debug("Assigning Content Generator build koji tag=%s to module "`
		`- "build", cg_build_koji_tag)`
		`- build.cg_build_koji_tag = cg_build_koji_tag`
mprahl commented 5 years ago It seems like we no longer use "cg_build_koji_tag". Could you please remove it from models and create a database migration.
		`-`
		`builder = module_build_service.builder.GenericBuilder.create_from_module(`
		`session, build, config)`

module_build_service/utils/mse.py

file modified

+60 -3

		`@@ -170,7 +170,8 @@`


		`def get_mmds_required_by_module_recursively(`
		`- session, mmd, default_streams=None, raise_if_stream_ambigous=False):`
		`+ session, mmd, default_streams=None, raise_if_stream_ambigous=False,`
		`+ runtime_requires=False):`
		`"""`
		`Returns the list of Module metadata objects of all modules required while`
		building the module defined by `mmd` module metadata. This presumes the
		`@@ -190,6 +191,8 @@`
		`:param bool raise_if_stream_ambigous: When True, raises a StreamAmbigous exception in case`
		`there are multiple streams for some dependency of module and the module name is not`
		defined in `default_streams`, so it is not clear which stream should be used.
		`+ :param bool runtime: When True, check the runtime requires of intput module instead`
mprahl commented 5 years ago "intput" => "input"
mprahl commented 5 years ago ":param bool runtime:" => ":param bool runtime_requires:"
		`+ of buildrequires.`
		`:rtype: list of Modulemd metadata`
		`:return: List of all modulemd metadata of all modules required to build`
		the module `mmd`.
		`@@ -202,9 +205,12 @@`

		`# At first get all the buildrequires of the module of interest.`
mprahl commented 5 years ago "buildrequires" => "buildrequires/requires"
		`for deps in mmd.get_dependencies():`
		`+ if runtime_requires:`
		`+ reqs = deps.get_requires()`
		`+ else:`
		`+ reqs = deps.get_buildrequires()`
		`mmds = _get_mmds_from_requires(`
		`- session, deps.get_buildrequires(), mmds, False, default_streams,`
		`- raise_if_stream_ambigous)`
		`+ session, reqs, mmds, False, default_streams, raise_if_stream_ambigous)`

		`# Now get the requires of buildrequires recursively.`
mprahl commented 5 years ago Could you change `buildrequires` to `buildrequires/requires`?
		`for mmd_key in list(mmds.keys()):`
		`@@ -221,6 +227,55 @@`
		`return res`


		`+ def resolve_platform_modules(session, mmd, raise_if_stream_ambigous=False, default_streams=None):`
		`+ """`
		`+ Returns list of platform (base) modules this module can require. The module can be built`
		`+ with requires "platform: [f28, f29]" or even "platform: []". In this case, we want to know`
		`+ on which platform modules the module can actually be installed to tag it into proper Koji`
		`+ tag later.`
		`+ """`
		`+ # Create local copy of mmd, because we will expand its dependencies,`
		`+ # which would change the module.`
		`+ # TODO: Use copy method once its in released libmodulemd:`
		`+ # https://github.com/fedora-modularity/libmodulemd/pull/20`
		`+ current_mmd = Modulemd.Module.new_from_string(mmd.dumps())`
		`+`
		`+ # MMDResolver expects the input MMD to have some context to resolve its "requires".`
		`+ current_mmd.set_context("00000000")`
		`+`
		`+ # Expands the MSE streams. This mainly handles '-' prefix in MSE streams.`
		`+ expand_mse_streams(session, current_mmd, default_streams, raise_if_stream_ambigous)`
		`+`
		`+ # Get the list of all MMDs which this module can be possibly built against`
		`+ # and add them to MMDResolver.`
		`+ mmd_resolver = MMDResolver()`
		`+ mmds_for_resolving = get_mmds_required_by_module_recursively(`
		`+ session, current_mmd, default_streams, raise_if_stream_ambigous)`
		`+ for m in mmds_for_resolving:`
		`+ mmd_resolver.add_modules(m)`
		`+`
		`+ # Show log.info message with the NSVCs we have added to mmd_resolver.`
		`+ nsvcs_to_solve = [`
		`+ ":".join([m.get_name(), m.get_stream(), str(m.get_version()), str(m.get_context())])`
		`+ for m in mmds_for_resolving]`
		`+ log.info("Starting resolving platform module with following input modules: %r",`
		`+ nsvcs_to_solve)`
		`+`
		`+ # Resolve the dependencies between modules and get the list of all valid`
		`+ # combinations in which we can build this module.`
		`+ requires_combinations = mmd_resolver.solve(current_mmd)`
		`+`
		`+ platforms = set()`
		`+ for requires in requires_combinations:`
		`+ for nsvca in requires:`
		`+ req_name, req_stream, _, req_context, req_arch = nsvca.split(":")`
		`+ if req_name in conf.base_module_names:`
		`+ platforms.add("%s:%s" % (req_name, req_stream))`
		`+`
		`+ log.info("Platform resolving done, possible platforms: %r", platforms)`
		`+ return list(platforms)`
		`+`
		`+`
		`def generate_expanded_mmds(session, mmd, raise_if_stream_ambigous=False, default_streams=None):`
		`"""`
		`Returns list with MMDs with buildrequires and requires set according`
		`@@ -358,6 +413,8 @@`
		`xmd['mbs'] = {}`
		`resolver = module_build_service.resolver.GenericResolver.create(conf)`
		`xmd['mbs']['buildrequires'] = resolver.resolve_requires(br_list)`
		`+ xmd['mbs']['base_modules'] = resolve_platform_modules(`
		`+ session, mmd_copy, raise_if_stream_ambigous, default_streams)`
		`xmd['mbs']['mse'] = True`

		`mmd_copy.set_xmd(glib.dict_values(xmd))`

tests/staged_data/nginx_mmd.yaml

file modified

		`@@ -29,3 +29,7 @@`
		`# files or extra patches`
		`module:`
		`- MIT`
		`+ xmd:`
		`+ mbs:`
		`+ base_modules:`
		`+ - platform:f27`
		`\ No newline at end of file`

tests/test_content_generator.py

file modified

+3 -17

		`@@ -49,7 +49,6 @@`
		`def setup_method(self, test_method):`
		`init_data(1, contexts=True)`
		`module = models.ModuleBuild.query.filter_by(id=2).one()`
		`- module.cg_build_koji_tag = "f27-module-candidate"`
		`self.cg = KojiContentGenerator(module, conf)`

		`# Ensure that there is no build log from other tests`
		`@@ -163,7 +162,7 @@`
		`""" Test preparation of directory with output files """`
		`dir_path = self.cg._prepare_file_directory()`
		`with open(path.join(dir_path, "modulemd.txt")) as mmd:`
		`- assert len(mmd.read()) == 1134`
		`+ assert len(mmd.read()) == 1206`

		`@patch("module_build_service.builder.KojiContentGenerator.get_session")`
		`def test_tag_cg_build(self, get_session):`
		`@@ -175,7 +174,7 @@`

		`self.cg._tag_cg_build()`

		`- koji_session.getTag.assert_called_once_with(self.cg.module.cg_build_koji_tag)`
		`+ koji_session.getTag.assert_called_once_with("f27-modular-updates-candidate")`
		`koji_session.tagBuild.assert_called_once_with(123, "nginx-0-2.10e50d06")`

		`@patch("module_build_service.builder.KojiContentGenerator.get_session")`
		`@@ -189,24 +188,11 @@`
		`self.cg._tag_cg_build()`

		`assert koji_session.getTag.mock_calls == [`
		`- call(self.cg.module.cg_build_koji_tag),`
		`+ call("f27-modular-updates-candidate"),`
		`call(conf.koji_cg_default_build_tag)]`
		`koji_session.tagBuild.assert_called_once_with(123, "nginx-0-2.10e50d06")`

		`@patch("module_build_service.builder.KojiContentGenerator.get_session")`
		`- def test_tag_cg_build_no_tag_set(self, get_session):`
		`- """ Test that the CG build is not tagged when no tag set. """`
		`- koji_session = MagicMock()`
		`- koji_session.getUser.return_value = GET_USER_RV`
		`- koji_session.getTag.side_effect = [{}, {'id': 123}]`
		`- get_session.return_value = koji_session`
		`-`
		`- self.cg.module.cg_build_koji_tag = None`
		`- self.cg._tag_cg_build()`
		`-`
		`- koji_session.tagBuild.assert_not_called()`
		`-`
		`- @patch("module_build_service.builder.KojiContentGenerator.get_session")`
		`def test_tag_cg_build_no_tag_available(self, get_session):`
		`""" Test that the CG build is not tagged when no tag available. """`
		`koji_session = MagicMock()`

tests/test_get_generator_json_expected_output.json

file modified

+3 -3

		`@@ -625,8 +625,8 @@`
		`}`
		`],`
		`"arch": "noarch",`
		`- "filesize": 1134,`
		`- "checksum": "bf1615b15f6a0fee485abe94af6b56b6",`
		`+ "filesize": 1206,`
		`+ "checksum": "51bc051ee409a475291e763c224a1b18",`
		`"checksum_type": "md5",`
		`"type": "file",`
		`"extra": {`
		`@@ -652,7 +652,7 @@`
		`"version": "2",`
		`"module_build_service_id": 2,`
		`"content_koji_tag": "module-nginx-1.2",`
		- "modulemd_str": "# Document type identifier\ndocument: modulemd\n# Module metadata format version\nversion: 1\ndata:\n # Module name, optional\n # Typically filled in by the buildsystem, using the VCS repository\n # name as the name of the module.\n name: nginx\n # Module update stream, optional\n # Typically filled in by the buildsystem, using the VCS branch name\n # as the name of the stream.\n stream: 1\n # Module version, integer, optional, cannot be negative\n # Typically filled in by the buildsystem, using the VCS commit\n # timestamp. Module version defines upgrade path for the particular\n # update stream.\n version: 2\n # A short summary describing the module, required\n summary: An example nginx module\n # A verbose description of the module, required\n description: >\n A module for the tests of module build service\n # Module and content licenses in the Fedora license identifier\n # format, required\n license:\n # Module license, required\n # This list covers licenses used for the module metadata, SPEC\n # files or extra patches\n module:\n - MIT\n"
		+ "modulemd_str": "# Document type identifier\ndocument: modulemd\n# Module metadata format version\nversion: 1\ndata:\n # Module name, optional\n # Typically filled in by the buildsystem, using the VCS repository\n # name as the name of the module.\n name: nginx\n # Module update stream, optional\n # Typically filled in by the buildsystem, using the VCS branch name\n # as the name of the stream.\n stream: 1\n # Module version, integer, optional, cannot be negative\n # Typically filled in by the buildsystem, using the VCS commit\n # timestamp. Module version defines upgrade path for the particular\n # update stream.\n version: 2\n # A short summary describing the module, required\n summary: An example nginx module\n # A verbose description of the module, required\n description: >\n A module for the tests of module build service\n # Module and content licenses in the Fedora license identifier\n # format, required\n license:\n # Module license, required\n # This list covers licenses used for the module metadata, SPEC\n # files or extra patches\n module:\n - MIT\n xmd:\n mbs:\n base_modules:\n - platform:f27"
		`}`
		`}`
		`},`

tests/test_get_generator_json_expected_output_with_log.json

file modified

+3 -3

		`@@ -625,8 +625,8 @@`
		`}`
		`],`
		`"arch": "noarch",`
		`- "filesize": 1134,`
		`- "checksum": "bf1615b15f6a0fee485abe94af6b56b6",`
		`+ "filesize": 1206,`
		`+ "checksum": "51bc051ee409a475291e763c224a1b18",`
		`"checksum_type": "md5",`
		`"type": "file",`
		`"extra": {`
		`@@ -661,7 +661,7 @@`
		`"version": "2",`
		`"module_build_service_id": 2,`
		`"content_koji_tag": "module-nginx-1.2",`
		- "modulemd_str": "# Document type identifier\ndocument: modulemd\n# Module metadata format version\nversion: 1\ndata:\n # Module name, optional\n # Typically filled in by the buildsystem, using the VCS repository\n # name as the name of the module.\n name: nginx\n # Module update stream, optional\n # Typically filled in by the buildsystem, using the VCS branch name\n # as the name of the stream.\n stream: 1\n # Module version, integer, optional, cannot be negative\n # Typically filled in by the buildsystem, using the VCS commit\n # timestamp. Module version defines upgrade path for the particular\n # update stream.\n version: 2\n # A short summary describing the module, required\n summary: An example nginx module\n # A verbose description of the module, required\n description: >\n A module for the tests of module build service\n # Module and content licenses in the Fedora license identifier\n # format, required\n license:\n # Module license, required\n # This list covers licenses used for the module metadata, SPEC\n # files or extra patches\n module:\n - MIT\n"
		+ "modulemd_str": "# Document type identifier\ndocument: modulemd\n# Module metadata format version\nversion: 1\ndata:\n # Module name, optional\n # Typically filled in by the buildsystem, using the VCS repository\n # name as the name of the module.\n name: nginx\n # Module update stream, optional\n # Typically filled in by the buildsystem, using the VCS branch name\n # as the name of the stream.\n stream: 1\n # Module version, integer, optional, cannot be negative\n # Typically filled in by the buildsystem, using the VCS commit\n # timestamp. Module version defines upgrade path for the particular\n # update stream.\n version: 2\n # A short summary describing the module, required\n summary: An example nginx module\n # A verbose description of the module, required\n description: >\n A module for the tests of module build service\n # Module and content licenses in the Fedora license identifier\n # format, required\n license:\n # Module license, required\n # This list covers licenses used for the module metadata, SPEC\n # files or extra patches\n module:\n - MIT\n xmd:\n mbs:\n base_modules:\n - platform:f27"
		`}`
		`}`
		`},`

tests/test_scheduler/test_module_wait.py

file modified

+1 -89

		`@@ -28,7 +28,7 @@`
		`import koji`
		`from tests import conf, db, app, scheduler_init_data`
		`from module_build_service import build_logs, Modulemd`
		`- from module_build_service.models import ComponentBuild, ModuleBuild`
		`+ from module_build_service.models import ComponentBuild`

		`base_dir = os.path.dirname(os.path.dirname(__file__))`

		`@@ -161,91 +161,3 @@`
		`module_build_service.scheduler.handlers.modules.wait(`
		`config=conf, session=db.session, msg=msg)`
		`assert koji_session.newRepo.called`
		`-`
		`- @patch("module_build_service.builder.GenericBuilder.default_buildroot_groups",`
		`- return_value={'build': [], 'srpm-build': []})`
		`- @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session")`
		`- @patch("module_build_service.builder.GenericBuilder.create_from_module")`
		`- @patch('module_build_service.resolver.DBResolver')`
		`- @patch('module_build_service.resolver.GenericResolver')`
		`- def test_set_cg_build_koji_tag_fallback_to_default(`
		`- self, generic_resolver, resolver, create_builder, koji_get_session, dbg):`
		`- """`
		`- Test that build.cg_build_koji_tag fallbacks to default tag.`
		`- """`
		`- with app.app_context():`
		`- base_mmd = Modulemd.Module()`
		`- base_mmd.set_name("base-runtime")`
		`- base_mmd.set_stream("f27")`
		`-`
		`- scheduler_init_data()`
		`- koji_session = mock.MagicMock()`
		`- koji_session.newRepo.return_value = 123456`
		`- koji_get_session.return_value = koji_session`
		`-`
		`- builder = mock.MagicMock()`
		`- builder.koji_session = koji_session`
		`- builder.module_build_tag = {"name": "module-123-build"}`
		`- builder.get_disttag_srpm.return_value = 'some srpm disttag'`
		`- builder.build.return_value = (1234, koji.BUILD_STATES['BUILDING'], "",`
		`- "module-build-macros-1-1")`
		`- create_builder.return_value = builder`
		`-`
		`- resolver = mock.MagicMock()`
		`- resolver.backend = 'db'`
		`- resolver.get_module_tag.return_value = "module-testmodule-master-20170109091357"`
		`- resolver.get_module_build_dependencies.return_value = {`
		`- "module-bootstrap-tag": base_mmd}`
		`- generic_resolver.create.return_value = resolver`
		`-`
		`- msg = module_build_service.messaging.MBSModule(msg_id=None, module_build_id=2,`
		`- module_build_state='some state')`
		`- module_build_service.scheduler.handlers.modules.wait(`
		`- config=conf, session=db.session, msg=msg)`
		`- module_build = ModuleBuild.query.filter_by(id=2).one()`
		`- assert module_build.cg_build_koji_tag == "modular-updates-candidate"`
		`-`
		`- @patch("module_build_service.builder.GenericBuilder.default_buildroot_groups",`
		`- return_value={'build': [], 'srpm-build': []})`
		`- @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session")`
		`- @patch("module_build_service.builder.GenericBuilder.create_from_module")`
		`- @patch('module_build_service.resolver.DBResolver')`
		`- @patch('module_build_service.resolver.GenericResolver')`
		`- @patch("module_build_service.config.Config.base_module_names",`
		`- new_callable=mock.PropertyMock, return_value=set(["base-runtime"]))`
		`- def test_set_cg_build_koji_tag(`
		`- self, cfg, generic_resolver, resolver, create_builder, koji_get_session, dbg):`
		`- """`
		`- Test that build.cg_build_koji_tag is set.`
		`- """`
		`- with app.app_context():`
		`- base_mmd = Modulemd.Module()`
		`- base_mmd.set_name("base-runtime")`
		`- base_mmd.set_stream("f27")`
		`-`
		`- scheduler_init_data()`
		`- koji_session = mock.MagicMock()`
		`- koji_session.newRepo.return_value = 123456`
		`- koji_get_session.return_value = koji_session`
		`-`
		`- builder = mock.MagicMock()`
		`- builder.koji_session = koji_session`
		`- builder.module_build_tag = {"name": "module-123-build"}`
		`- builder.get_disttag_srpm.return_value = 'some srpm disttag'`
		`- builder.build.return_value = (1234, koji.BUILD_STATES['BUILDING'], "",`
		`- "module-build-macros-1-1")`
		`- create_builder.return_value = builder`
		`-`
		`- resolver = mock.MagicMock()`
		`- resolver.backend = 'db'`
		`- resolver.get_module_tag.return_value = "module-testmodule-master-20170109091357"`
		`- resolver.get_module_build_dependencies.return_value = {`
		`- "module-bootstrap-tag": base_mmd}`
		`- generic_resolver.create.return_value = resolver`
		`-`
		`- msg = module_build_service.messaging.MBSModule(msg_id=None, module_build_id=2,`
		`- module_build_state='some state')`
		`- module_build_service.scheduler.handlers.modules.wait(`
		`- config=conf, session=db.session, msg=msg)`
		`- module_build = ModuleBuild.query.filter_by(id=2).one()`
		`- assert module_build.cg_build_koji_tag == "f27-modular-updates-candidate"`

tests/test_utils/test_utils_mse.py

file modified

+8 -10

		`@@ -369,16 +369,14 @@`
		`self._make_module("platform:f29:0:c11", {}, {})`

		`@pytest.mark.parametrize('requires,build_requires,expected', [`
		`- ({}, {"gtk": ["1"]},`
		`- ['foo:1:1:c2', 'base:f29:0:c3', 'platform:f29:0:c11',`
		`- 'bar:1:1:c2', 'gtk:1:1:c2', 'lorem:1:1:c2']),`
		`-`
		`- ({}, {"foo": ["1"]},`
		`- ['foo:1:1:c2', 'base:f29:0:c3', 'platform:f29:0:c11',`
		`- 'bar:1:1:c2', 'lorem:1:1:c2']),`
		`+ ({"gtk": ["1"]}, {"gtk": ["1"]},`
		`+ ["platform:f29"]),`
mprahl commented 5 years ago I don't understand why `["platform:f29"]` is expected here? Since there is a "gtk:1" that requires "platform:f28" and another that requires "platform:f29", and "app:1:0:c1" doesn't require platform at all, I would have expected both "platform:f28" and "platform:f29" to be options.
jkaluza commented 5 years ago This method currently uses `_generate_default_modules_recursion()`, which generates just single `gtk:1:1` which requires just `platform:f29`. The test-case will later be expanded more and will probably use different set of test modules, but for now it works as expected.
		`])`
		`- def test_get_required_modules_recursion(self, requires, build_requires, expected):`
		`+ def test_resolve_platform_modules(self, requires, build_requires, expected):`
		`module_build = self._make_module("app:1:0:c1", requires, build_requires)`
		`self._generate_default_modules_recursion()`
		`- nsvcs = self._get_mmds_required_by_module_recursively(module_build)`
		`- assert set(nsvcs) == set(expected)`
		`+`
		`+ mmd = module_build.mmd()`
		`+ platforms = module_build_service.utils.resolve_platform_modules(db.session, mmd)`
		`+`
		`+ assert set(platforms) == set(expected)`

no initial comment

mprahl commented on line 7 of module_build_service/mmd_resolver.py 5 years ago

Could you please explain this change?

mprahl commented on line 15 of module_build_service/utils/mse.py 5 years ago

"intput" => "input"

mprahl commented on line 15 of module_build_service/utils/mse.py 5 years ago

":param bool runtime:" => ":param bool runtime_requires:"

mprahl commented on line 33 of module_build_service/utils/mse.py 5 years ago

Could you change buildrequires to buildrequires/requires?

jkaluza commented on line 7 of module_build_service/mmd_resolver.py 5 years ago

This currently waits for Igor to rewrite.

mprahl commented on line 22 of module_build_service/utils/mse.py 5 years ago

"buildrequires" => "buildrequires/requires"

mprahl commented on line 41 of module_build_service/scheduler/handlers/modules.py 5 years ago

It seems like we no longer use "cg_build_koji_tag". Could you please remove it from models and create a database migration.

mprahl commented on line 13 of tests/test_utils/test_utils_mse.py 5 years ago

I don't understand why ["platform:f29"] is expected here? Since there is a "gtk:1" that requires "platform:f28" and another that requires "platform:f29", and "app:1:0:c1" doesn't require platform at all, I would have expected both "platform:f28" and "platform:f29" to be options.

mprahl commented 5 years ago

@jkaluza looks good. I just left a few comments/questions. Let me know when you're ready for another review.

Edited 5 years ago by mprahl

jkaluza commented on line 13 of tests/test_utils/test_utils_mse.py 5 years ago

This method currently uses _generate_default_modules_recursion(), which generates just single gtk:1:1 which requires just platform:f29. The test-case will later be expanded more and will probably use different set of test modules, but for now it works as expected.

jkaluza commented 5 years ago

I'm closing this PR without merging. The requirements changed - we want to tag the builds based on the buildrequires and not requires. Also resolving based on requires is not possible to implement easily using current libsolv python API.