PR#971: Use the libmodulemd API properly when setting values on existing objects - fm-orchestrator

fm-orchestrator

#971 Use the libmodulemd API properly when setting values on existing objects

Merged 5 years ago by mprahl. Opened 5 years ago by mprahl.

libmodulemd-proper-set into master

Use the libmodulemd API properly when setting values on existing objects

mprahl • 5 years ago

8173040

module_build_service/utils/submit.py

file modified

+8 -4

		`@@ -100,11 +100,13 @@`
		`else:`
		`xmd['mbs']['commit'] = scm.get_latest()`

		`- if mmd.get_rpm_components() or mmd.get_module_components():`
		`+ rpm_components = mmd.get_rpm_components()`
		`+ module_components = mmd.get_module_components()`
		`+ if rpm_components or module_components:`
		`if 'rpms' not in xmd['mbs']:`
		`xmd['mbs']['rpms'] = {}`
		`# Add missing data in RPM components`
		`- for pkgname, pkg in mmd.get_rpm_components().items():`
		`+ for pkgname, pkg in rpm_components.items():`
		`if pkg.get_repository() and not conf.rpms_allow_repository:`
		`raise Forbidden(`
		`"Custom component repositories aren't allowed. "`
		`@@ -119,9 +121,10 @@`
		`pkg.set_cache(conf.rpms_default_cache + pkgname)`
		`if not pkg.get_ref():`
		`pkg.set_ref('master')`
		`+ mmd.set_rpm_components(rpm_components)`

		`# Add missing data in included modules components`
		`- for modname, mod in mmd.get_module_components().items():`
		`+ for modname, mod in module_components.items():`
		`if mod.get_repository() and not conf.modules_allow_repository:`
		`raise Forbidden(`
		`"Custom module repositories aren't allowed. "`
		`@@ -130,11 +133,12 @@`
		`mod.set_repository(conf.modules_default_repository + modname)`
		`if not mod.get_ref():`
		`mod.set_ref('master')`
		`+ mmd.set_module_components(module_components)`

		`# Check that SCM URL is valid and replace potential branches in pkg refs`
		`# by real SCM hash and store the result to our private xmd place in modulemd.`
		`pool = ThreadPool(20)`
		`- pkg_dicts = pool.map(_scm_get_latest, mmd.get_rpm_components().values())`
		`+ pkg_dicts = pool.map(_scm_get_latest, rpm_components.values())`
		`err_msg = ""`
		`for pkg_dict in pkg_dicts:`
		`if pkg_dict["error"]:`

tests/__init__.py

file modified

+3 -1

		`@@ -291,7 +291,9 @@`
		`current_dir, 'staged_data', 'formatted_testmodule.yaml')`
		`mmd = Modulemd.Module().new_from_file(formatted_testmodule_yml_path)`
		`mmd.upgrade()`
		`- mmd.get_rpm_components()['tangerine'].set_buildorder(0)`
		`+ components = mmd.get_rpm_components()`
		`+ components['tangerine'].set_buildorder(0)`
		`+ mmd.set_rpm_components(components)`

		`build_one = module_build_service.models.ModuleBuild()`
		`build_one.name = 'testmodule'`

tests/test_utils/test_utils.py

file modified

+10 -5

		`@@ -90,8 +90,9 @@`
		`second_module_build = models.ModuleBuild.query.filter_by(id=3).one()`
		`if changed_component:`
		`mmd = second_module_build.mmd()`
		`- mmd.get_rpm_components()['tangerine'].set_ref(`
		`- '00ea1da4192a2030f9ae023de3b3143ed647bbab')`
		`+ components = mmd.get_rpm_components()`
		`+ components['tangerine'].set_ref('00ea1da4192a2030f9ae023de3b3143ed647bbab')`
		`+ mmd.set_rpm_components(components)`
		`second_module_build.modulemd = mmd.dumps()`
		`second_module_changed_component = models.ComponentBuild.query.filter_by(`
		`package=changed_component, module_id=3).one()`
		`@@ -204,7 +205,9 @@`
		`mmd = second_module_build.mmd()`
		`br_list = Modulemd.SimpleSet()`
		`br_list.add('master')`
		`- mmd.get_dependencies()[0].set_buildrequires({'some_module': br_list})`
		`+ deps = mmd.get_dependencies()`
		`+ deps[0].set_buildrequires({'some_module': br_list})`
		`+ mmd.set_dependencies(deps)`
		`xmd = glib.from_variant_dict(mmd.get_xmd())`
		`xmd['mbs']['buildrequires'] = {`
		`'some_module': {`
		`@@ -306,9 +309,11 @@`
		`mmd = Modulemd.Module().new_from_file(`
		`path.join(BASE_DIR, '..', 'staged_data', 'testmodule.yaml'))`
		`mmd.upgrade()`
		`+ components = mmd.get_rpm_components()`
		`# Modify the component branches so we can identify them later on`
		`- mmd.get_rpm_components()['perl-Tangerine'].set_ref('f28')`
		`- mmd.get_rpm_components()['tangerine'].set_ref('f27')`
		`+ components['perl-Tangerine'].set_ref('f28')`
		`+ components['tangerine'].set_ref('f27')`
		`+ mmd.set_rpm_components(components)`
		`module_build_service.utils.format_mmd(mmd, scmurl)`

		`# Make sure that original refs are not changed.`

mprahl commented 5 years ago

MBS has been using the libmodulemd API incorrectly by assuming that methods like get_rpm_components return the actual object in memory and not a copy. This has been true but wasn't something sgallagh intended. We found this out after he had me test his new version of libmodulemd. This PR removes those assumptions.

@sgallagh could you please review?

rebased onto 9b09f1fdf00a2265e00760150243909d2ab7277e

5 years ago

rebased onto 4c59d8c083de91a78aaca28734af91083a0b15d7

5 years ago

rebased onto 8173040

5 years ago

mprahl commented 5 years ago

Tests pass at:
https://ci.centos.org/job/factory2-fm-orchestrator/job/libmodulemd-proper-set/4/

sgallagh commented 5 years ago

LGTM

Pull-Request has been merged by mprahl

5 years ago

mprahl commented 5 years ago

@sgallagh since you decided to leave libmodulemd exposing the actual value in memory and not a copy, and this PR didn't actually fix all those instances where we weren't assuming it was a copy, I decided to revert this. This will reduce unnecessary risk in the next release and deployment.

sgallagh commented 5 years ago

@mprahl Makes sense to me. Thanks for the update.