PR#1686: Fix mmd.copy calls to support PackagerV3 - fm-orchestrator

fm-orchestrator

#1686 Fix mmd.copy calls to support PackagerV3

Merged 3 years ago by mikem. Opened 3 years ago by breilly.

breilly/fm-orchestrator v3copyfix into master

Fix mmd.copy calls to support PackagerV3

Brendan Reilly • 3 years ago

d0579a1

module_build_service/common/submit.py

file modified

+26 -11

		`@@ -10,6 +10,7 @@`
		`from module_build_service.common import conf, log`
		`from module_build_service.common.errors import ValidationError`
		`from module_build_service.common.utils import load_mmd_file`
		`+ from module_build_service.common.modulemd import Modulemd`


		`def _is_eol_in_pdc(name, stream):`
		`@@ -46,7 +47,8 @@`
		`if not whitelist_url and mandatory_checks:`
		`scm.verify()`
		`cofn = scm.get_module_yaml()`
		`- mmd = load_mmd_file(cofn)`
		`+ # load_mmd_file can return either a ModuleStreamV2 or PackagerV3 object`
		`+ stream_or_packager = load_mmd_file(cofn)`
		`finally:`
		`try:`
		`if td is not None:`
		`@@ -60,38 +62,51 @@`
		`"Module {}:{} is marked as EOL in PDC.".format(scm.name, scm.branch))`

		`if not mandatory_checks:`
		`- return mmd, scm`
		`+ return stream_or_packager, scm`

		`# If the name was set in the modulemd, make sure it matches what the scmurl`
		`# says it should be`
		`- if mmd.get_module_name() and mmd.get_module_name() != scm.name:`
		`+ if stream_or_packager.get_module_name() and stream_or_packager.get_module_name() != scm.name:`
		`if not conf.allow_name_override_from_scm:`
		`raise ValidationError(`
		`'The name "{0}" that is stored in the modulemd is not valid'`
		`- .format(mmd.get_module_name())`
		`+ .format(stream_or_packager.get_module_name())`
		`)`
		`else:`
		`# Set the module name`
		`- mmd = mmd.copy(scm.name)`
		`+ if isinstance(stream_or_packager, Modulemd.ModuleStream):`
		`+ # this is a ModuleStreamV2 object`
		`+ stream_or_packager = stream_or_packager.copy(scm.name)`
		`+ else:`
		`+ # this is a PackagerV3 object`
		`+ stream_or_packager = stream_or_packager.copy()`
		`+ stream_or_packager.set_module_name(scm.name)`
otaylor commented 3 years ago I'll comment again that all three of the PackagerV3 branches are not covered by any test.

		`# If the stream was set in the modulemd, make sure it matches what the repo`
		`# branch is`
		`- if mmd.get_stream_name() and mmd.get_stream_name() != scm.branch:`
		`+ if stream_or_packager.get_stream_name() and stream_or_packager.get_stream_name() != scm.branch:`
		`if not conf.allow_stream_override_from_scm:`
		`raise ValidationError(`
		`'The stream "{0}" that is stored in the modulemd does not match the branch "{1}"'`
		`- .format(mmd.get_stream_name(), scm.branch)`
		`+ .format(stream_or_packager.get_stream_name(), scm.branch)`
		`)`
		`else:`
		`# Set the module stream`
		`- mmd = mmd.copy(mmd.get_module_name(), scm.branch)`
		`+ if isinstance(stream_or_packager, Modulemd.ModuleStream):`
		`+ # this is a ModuleStreamV2 object`
		`+ stream_or_packager = stream_or_packager.copy(stream_or_packager.get_module_name(),`
		`+ scm.branch)`
		`+ else:`
		`+ # this is a PackagerV3 object`
		`+ stream_or_packager = stream_or_packager.copy()`
		`+ stream_or_packager.set_stream_name(scm.branch)`

		`# If the version is in the modulemd, throw an exception since the version`
		`# since the version is generated by MBS`
		`- if mmd.get_mdversion() == 2 and mmd.get_version():`
		`+ if stream_or_packager.get_mdversion() == 2 and stream_or_packager.get_version():`
		`raise ValidationError(`
		`'The version "{0}" is already defined in the modulemd but it shouldn\'t be since the '`
		`- "version is generated based on the commit time".format(mmd.get_version())`
		`+ "version is generated based on the commit time".format(stream_or_packager.get_version())`
		`)`

		`- return mmd, scm`
		`+ return stream_or_packager, scm`

module_build_service/web/submit.py

file modified

+41 -25

		`@@ -111,28 +111,40 @@`
		`db_session, username, handle, params, stream=None, skiptests=False`
		`):`
		`yaml_file = to_text_type(handle.read())`
		`- mmd = load_mmd(yaml_file)`
		`+ # load_mmd can return either a ModuleStreamV2 or PackagerV3 object`
		`+ # PackagerV3 objects become ModuleStreamV2 objects in submit_module_build`
		`+ stream_or_packager = load_mmd(yaml_file)`
		`if hasattr(handle, "filename"):`
		`def_name = str(os.path.splitext(os.path.basename(handle.filename))[0])`
		`- elif not mmd.get_module_name():`
		`+ elif not stream_or_packager.get_module_name():`
		`raise ValidationError(`
		`"The module's name was not present in the modulemd file. Please use the "`
		`'"module_name" parameter'`
		`)`
		`- module_name = mmd.get_module_name() or def_name`
		`- module_stream = stream or mmd.get_stream_name() or "master"`
		`- if module_name != mmd.get_module_name() or module_stream != mmd.get_stream_name():`
		`+ module_name = stream_or_packager.get_module_name() or def_name`
		`+ module_stream = stream or stream_or_packager.get_stream_name() or "master"`
		`+ if module_name != stream_or_packager.get_module_name() or \`
		`+ module_stream != stream_or_packager.get_stream_name():`
		`# This is how you set the name and stream in the modulemd`
		`- mmd = mmd.copy(module_name, module_stream)`
		`- if skiptests:`
		`- buildopts = mmd.get_buildopts() or Modulemd.Buildopts()`
		`+ if isinstance(stream_or_packager, Modulemd.ModuleStream):`
		`+ # This is a ModuleStreamV2 object`
		`+ stream_or_packager = stream_or_packager.copy(module_name, module_stream)`
		`+ else:`
		`+ # This is a PackagerV3 object`
		`+ stream_or_packager = stream_or_packager.copy()`
		`+ stream_or_packager.set_module_name(module_name)`
		`+ stream_or_packager.set_stream_name(module_stream)`
		`+ if skiptests and isinstance(stream_or_packager, Modulemd.ModuleStream):`
		`+ # PackagerV3 objects do not have buildopts methods`
		`+ buildopts = stream_or_packager.get_buildopts() or Modulemd.Buildopts()`
		`macros = buildopts.get_rpm_macros() or ""`
		`buildopts.set_rpm_macros(macros + "\n\n%__spec_check_pre exit 0\n")`
		`- mmd.set_buildopts(buildopts)`
		`+ stream_or_packager.set_buildopts(buildopts)`

		`- module_stream_version = provide_module_stream_version_from_mmd(mmd)`
		`+ module_stream_version = provide_module_stream_version_from_mmd(stream_or_packager)`

		`- return submit_module_build(db_session, username, mmd, params, module_stream_version)`
		`+ return submit_module_build(db_session, username, stream_or_packager, params,`
		`+ module_stream_version)`


		`_url_check_re = re.compile(r"^[^:/]+:.*$")`
		`@@ -146,11 +158,12 @@`
		`log.info("'{}' is not a valid URL, assuming local path".format(url))`
		`url = os.path.abspath(url)`
		`url = "file://" + url`
		`- mmd, scm = fetch_mmd(url, branch, allow_local_url)`
		`+ stream_or_packager, scm = fetch_mmd(url, branch, allow_local_url)`

		`module_stream_version = int(scm.version)`

		`- return submit_module_build(db_session, username, mmd, params, module_stream_version)`
		`+ return submit_module_build(db_session, username, stream_or_packager, params,`
		`+ module_stream_version)`


		`def _apply_dep_overrides(mmd, params):`
		`@@ -540,13 +553,14 @@`
		`_modify_buildtime_streams(db_session, mmd, new_streams_func)`


		`- def submit_module_build(db_session, username, mmd, params, module_stream_version):`
		`+ def submit_module_build(db_session, username, stream_or_packager, params, module_stream_version):`
		`"""`
		`Submits new module build.`

		`:param db_session: SQLAlchemy session object.`
		`:param str username: Username of the build's owner.`
		`- :param Modulemd.ModuleStream mmd: Modulemd defining the build.`
		`+ :type stream_or_packager: Modulemd.ModuleStream or Modulemd.PackagerV3`
		`+ Modulemd.ModuleStream or PackagerV3 object defining the build.`
		`:param dict params: the API parameters passed in by the user`
		`:rtype: list with ModuleBuild`
		`:return: List with submitted module builds.`
		`@@ -562,7 +576,8 @@`
		`if "default_streams" in params:`
		`default_streams = params["default_streams"]`

		`- input_mmds, static_context = process_module_context_configuration(mmd)`
		`+ # PackagerV3 objects become ModuleStreamV2 objects at this point`
		`+ input_mmds, static_context = process_module_context_configuration(stream_or_packager)`

		`for mmd in input_mmds:`
		`mmd.set_version(module_stream_version)`
		`@@ -712,17 +727,17 @@`
		`return modules`


		`- def process_module_context_configuration(mmd):`
		`+ def process_module_context_configuration(stream_or_packager):`
		`"""`
		`Processes initial module metadata context configurations and creates individual module`
		`metadata for each context, if static context configuration is present.`

		`- :param Modulemd.ModuleStream packager_mmd: Packager (initial) modulemd which kickstarts`
		`- the build.`
		`+ :type stream_or_packager: Modulemd.ModuleStream or Modulemd.PackagerV3`
		`+ Packager (initial) modulemd which kickstarts the build.`
		`:rtype: list with ModuleBuild`
		`:return: list of generated module metadata from context configurations.`
		`"""`
		`- mdversion = mmd.get_mdversion()`
		`+ mdversion = stream_or_packager.get_mdversion()`
		`static_context = False`

		`# we check what version of the metadata format we are using.`
		`@@ -730,7 +745,7 @@`
		`# v3 we always talking about a new build and the static context`
		`# will be always True`
		`static_context = True`
		`- mdindex = mmd.convert_to_index()`
		`+ mdindex = stream_or_packager.convert_to_index()`
		`streams = mdindex.search_streams()`

		`for stream in streams:`
		`@@ -753,19 +768,20 @@`

		`return streams, static_context`
		`else:`
		`- xmd = mmd.get_xmd()`
		`+ xmd = stream_or_packager.get_xmd()`
		`# check if we are handling rebuild of a static context module`
		`if "mbs" in xmd:`
		`# check if it is a static context`
		`- if "static_context" in xmd["mbs"] or mmd.is_static_context():`
		`+ if "static_context" in xmd["mbs"] or stream_or_packager.is_static_context():`
		`static_context = True`
		`- return [mmd], static_context`
		`+ return [stream_or_packager], static_context`

		# we check if static contexts are enabled by the `contexts` property defined by the user i
		`# as an build option.`
		`static_context = "mbs_options" in xmd and "contexts" in xmd["mbs_options"]`
		`# if the static context configuration exists we expand it. If not we just return`
		`# the mmd unchanged, for futher processing.`
		`- streams = generate_mmds_from_static_contexts(mmd) if static_context else [mmd]`
		`+ streams = generate_mmds_from_static_contexts(stream_or_packager) if static_context \`
		`+ else [stream_or_packager]`

		`return streams, static_context`

tests/staged_data/v3/mmd_packager.yaml

file modified

+1 -1

		`@@ -4,7 +4,7 @@`
		`version: 3`
		`data:`
		`name: foo`
		`- stream: latest`
		`+ stream: master`
		`summary: An example module`
		`description: >-`
		`A module for the demonstration of the metadata format. Also,`

tests/test_common/test_submit.py

file modified

+31 -1

		`@@ -4,7 +4,9 @@`

		`import mock`

		`- from module_build_service.common.submit import _is_eol_in_pdc`
		`+ from module_build_service.common.modulemd import Modulemd`
		`+ from module_build_service.common.submit import _is_eol_in_pdc, fetch_mmd`
		`+ from tests.test_web.test_views import FakeSCM`


		`@mock.patch("module_build_service.common.submit.requests")`
		`@@ -32,3 +34,31 @@`

		`is_eol = _is_eol_in_pdc("mariadb", "10.1")`
		`assert is_eol`
		`+`
		`+`
		`+ @mock.patch("module_build_service.common.scm.SCM")`
		`+ def test_fetch_mmd(mocked_scm):`
		`+ """ Test behavior for fetch_mmd """`
		`+`
		`+ FakeSCM(`
		`+ mocked_scm,`
		`+ "testmodule",`
		`+ "testmodule.yaml",`
		`+ "620ec77321b2ea7b0d67d82992dda3e1d67055b4")`
		`+`
		`+ mmd, scm = fetch_mmd('testurl')`
		`+ assert isinstance(mmd, Modulemd.ModuleStream)`
		`+`
		`+`
		`+ @mock.patch("module_build_service.common.scm.SCM")`
		`+ def test_fetch_mmd_packager_v3(mocked_scm):`
		`+ """ Test PackagerV3 behavior for fetch_mmd """`
		`+`
		`+ FakeSCM(`
		`+ mocked_scm,`
		`+ "foo",`
		`+ "v3/mmd_packager.yaml",`
		`+ "620ec77321b2ea7b0d67d82992dda3e1d67055b4")`
		`+`
		`+ mmd, scm = fetch_mmd('testurl')`
		`+ assert not isinstance(mmd, Modulemd.ModuleStream)`

tests/test_web/test_submit.py

file modified

+30

		`@@ -300,6 +300,36 @@`
		`assert username_arg == username`
		`rmtree(module_dir)`

		`+ @mock.patch("module_build_service.web.submit.submit_module_build")`
		`+ def test_submit_module_build_from_yaml_packager_v3(self, mock_submit):`
		`+ """`
		`+ Tests local module build from a yaml file with the skiptests option`
		`+`
		`+ Args:`
		`+ mock_submit (MagickMock): mocked function submit_module_build, which we then`
		`+ inspect if it was called with correct arguments`
		`+ """`
		`+ module_dir = tempfile.mkdtemp()`
		`+ modulemd_yaml = read_staged_data("v3/mmd_packager")`
		`+ modulemd_file_path = path.join(module_dir, "testmodule.yaml")`
		`+`
		`+ username = "test"`
		`+ stream = "dev"`
		`+`
		`+ with io.open(modulemd_file_path, "w", encoding="utf-8") as fd:`
		`+ fd.write(modulemd_yaml)`
		`+`
		`+ with open(modulemd_file_path, "rb") as fd:`
		`+ handle = FileStorage(fd)`
		`+ submit_module_build_from_yaml(`
		`+ db_session, username, handle, {}, stream=stream, skiptests=False)`
		`+ mock_submit_args = mock_submit.call_args[0]`
		`+ username_arg = mock_submit_args[1]`
		`+ mmd_arg = mock_submit_args[2]`
		`+ assert mmd_arg.get_stream_name() == stream`
		`+ assert username_arg == username`
		`+ rmtree(module_dir)`
		`+`
		`@mock.patch("module_build_service.web.submit.generate_expanded_mmds")`
		`def test_submit_build_new_mse_build(self, generate_expanded_mmds):`
		`"""`

breilly commented 3 years ago

Fixes #1685

breilly commented 3 years ago

I decided to keep the behavior 1:1, but those mmd = mmd.copy() lines may be superfluous.

mkovarik commented 3 years ago

Build 33c0aa2fee49289367cd0984f8f37df47c46d8c4 FAILED!
Rebase or make new commits to rebuild.

otaylor commented on line 6 of module_build_service/builder/KojiContentGenerator.py 3 years ago

A bit surprising that we're carrying around 'mmd' objects that could be either ModuleStreamV2 or ModulePackagerV3 with no common baseclass. Glad we're in Python and we can duck-type! When we can't duck-type, though, what we care about is the type of the object, not the version of the modulemd file.

I'd do:

```
if isinstance(mmd, Modulemd.ModuleStream):
# old stuff
else:
# new stuff

Maybe a convenience function though?

otaylor commented 3 years ago

Other than the stylistic comment above, this looks like it should work, as far as it goes, but doesn't look sufficient to make things work with V3 objects? E.g., there are a lot of uses of mmd.get_dependencies() in the codebase.

Edited 3 years ago by otaylor

mikem commented 3 years ago

what we care about is the type of the object, not the version of the modulemd file

I think @breilly is carrying forward some existing logic from the previous PR.

I had asked in #1667 for the patch to clearly distinguish between the two formats, but it doesn't look like that happened, otherwise there would probably be some code similar to what @otaylor is suggesting already.

mikem commented 3 years ago

When were set_module_name and set_stream_name added? Can we just use the new method unconditionally?

otaylor commented 3 years ago

When were set_module_name and set_stream_name added? Can we just use the new method unconditionally?

I dug into this in some detail, and tested it out to make sure, and the behavior is that if you use the new Modulemd.read_packager_file() [added in 2.12] you get either a Modulemd.ModuleStream() object, with one prototype for copy() or a Modulemd.PackagerV3() that has a vaguely similar, but different API, including different prototype for copy() and the set_module_name() and set_stream_name() methods.

mcurlej commented 3 years ago

When were set_module_name and set_stream_name added? Can we just use the new method unconditionally?

I dug into this in some detail, and tested it out to make sure, and the behavior is that if you use the new Modulemd.read_packager_file() [added in 2.12] you get either a Modulemd.ModuleStream() object, with one prototype for copy() or a Modulemd.PackagerV3() that has a vaguely similar, but different API, including different prototype for copy() and the set_module_name() and set_stream_name() methods.

This was not something that was requested. It seems that it was changed nonetheless. The request was to leave the API the same.

Other than the stylistic comment above, this looks like it should work, as far as it goes, but doesn't look sufficient to make things work with V3 objects? E.g., there are a lot of uses of mmd.get_dependencies() in the codebase.

The v3 object is used only until submit_module_build then its a standard v2 stream. V3 is never used anywhere else. Also v3 object is not stored in the DB only v2 stream.

@breilly The patch looks good. +1

Edited 3 years ago by mcurlej

otaylor commented 3 years ago

Thanks for the explanation, @mcurlej ! I think that given that ModuleStreamV2 and PackagerV3 objects have different API's, we should avoid:

Having aload_mmd() that returns either type of object depending on the type of the input, used in circumstances where we expect only streams without any assertion. Maybe load_mmd(stream_ok=True, packager_ok=False) Or split into multiple methods.
Having variables called mmd that could be either a stream or a packager object when the vast majority of mmd variables in the codebase are guaranteed to be Modulemd.ModuleStreamV2. Maybe: stream_or_packager?

That would make it clear that the KojiContentGenerator chunk here isn't needed and doesn't make sense.

I'm also curious why the tests in the previous version of this patch didn't catch the problem - I guess it's because of the optimization where the code only sets the name/stream if they are different. Shouldn't the tests here be extended so that the new code paths are hit?

rebased onto ff862c6e2979f605fa6822c4fc0e10f23ac13add

3 years ago

mkovarik commented 3 years ago

Build ff862c6e2979f605fa6822c4fc0e10f23ac13add FAILED!
Rebase or make new commits to rebuild.

breilly commented 3 years ago

@mcurlej @otaylor @mikem thanks for the reviews, I've updated the PR.

I changed the duck-type'd variable and added a few more comments for clarification. Because of this, I discovered that the skiptests section of submit_module_build_from_yaml does not work for PackagerV3 objects, as they support neither get_buildopts() or set_buildopts().

rebased onto d99bec4c0ac11ef2b9015e85555549880fc6899c

3 years ago

mkovarik commented 3 years ago

Build d99bec4c0ac11ef2b9015e85555549880fc6899c FAILED!
Rebase or make new commits to rebuild.

otaylor commented on line 45 of module_build_service/common/submit.py 3 years ago

I'll comment again that all three of the PackagerV3 branches are not covered by any test.

otaylor commented on line 73 of module_build_service/web/submit.py 3 years ago

Thanks for going through and doing the renames! I like the increased clarity! The mmd parameter to process_module_context_configuration should also get renamed as a duck type - especially since 2 lines later, mmd is reused as a non-duck-type must-be-a-stream.

rebased onto 5900db1b7dd8acac69405e17c9ef4bb1b87329c9

3 years ago

mkovarik commented 3 years ago

Build 5900db1b7dd8acac69405e17c9ef4bb1b87329c9 FAILED!
Rebase or make new commits to rebuild.

rebased onto 3f17949a9446ec7ca583b382068cca2a92579a5e

3 years ago

breilly commented 3 years ago

Thanks, updated submit_module_build and added unit tests.

mkovarik commented 3 years ago

Build 3f17949a9446ec7ca583b382068cca2a92579a5e FAILED!
Rebase or make new commits to rebuild.

otaylor commented 3 years ago

Looks good to me!

mcurlej commented 3 years ago

@breilly LGTM! + Thanks for the tests.

mikem commented 3 years ago

load_mmd can return either a ModuleStreamV2 or PackagerV3 object

This is not true. load_mmd can return either a packager or stream object. It automatically upgrades either from version 1 to 2, but it does not force packager objects to v3. We could get a v2 packager object.

this is a ModuleStreamV2 object
...
this is a PackagerV3 object

There are numerous comments like these that tell the reader to assume something about about the format version that is not actually a given at that point in the code. It would be better to clarify these.

Edited 3 years ago by mikem

mikem commented 3 years ago

+    :param stream_or_packager: Modulemd.ModuleStream or PackagerV3 object defining the build.

We've dropped the type here. It's unwieldy to fit the complex type specification on one line, but perhaps we should add a :type entry for this field.

https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists

otaylor commented 3 years ago

This is not true. load_mmd can return either a packager or stream object. It automatically upgrades either from version 1 to 2, but it does not force packager objects to v3. We could get a v2 packager object.

Im pretty sure that there is no PackagerV2 class, and PackagerV3 has no "Packager" superclass - it inherits directly from GObject. (https://github.com/fedora-modularity/libmodulemd/blob/main/modulemd/include/modulemd-2.0/modulemd-packager-v3.h) The modulemd-packager document type is new in V3 of the specification.

mikem commented 3 years ago

You've changed the parameter name for submit_module_build, but you haven't done the same in process_module_context_configuration, which is also receiving the stream_or_packager value. It seems a little inconsistent, esp given that this is the point where the packager object gets converted.

mikem commented 3 years ago