#1035 MBSResolver: only return the latest version for get_module_modulemds()
Merged 5 years ago by mprahl. Opened 5 years ago by otaylor.
otaylor/fm-orchestrator mbs-resolver-versions  into  master

@@ -74,7 +74,8 @@ 

  

          :param str name: module's name.

          :param str stream: module's stream.

-         :kwarg str version: module's version. Optional.

+         :kwarg str version: a string or int of the module's version. When None,

+             latest version will be returned.

          :kwarg str context: module's context. Optional.

          :kwarg str state: module's state. Defaults to ``ready``.

          :kwarg bool strict: Normally this function returns None if no module can be
@@ -111,7 +112,11 @@ 

              else:

                  return None

  

-         return modules

+         if version is None:

+             # Only return the latest version

+             return [m for m in modules if m["version"] == modules[0]["version"]]

+         else:

+             return modules

  

      def _get_module(self, name, stream, version, context, state="ready", strict=False):

          return self._get_modules(name, stream, version, context, state, strict)[0]

get_module_modulemds() was documented to return only the latest version,
but actually returned all versions. Because this wasn't anticipated in
the libsolv usage for module resolution, a more-or-less arbitrary version
would be picked to build local builds against instead of the latest one.

+1. For other reviewers, the list of modules returned by MBS are ordered_desc_by "version" earlier in the code, so the way how @otaylor gets the latest module is correct.

It might be useful to have a test for that. You would need to add new testmodule test data in ./tests/conftest.py the same way as TESTMODULE_MODULEMD_SECOND_CONTEXT is constructed, but with the lower version.

Then probably extend test_get_module_modulemds_partial in ./tests/test_resolver/test_mbs.py to include that new testmodule in the mocked response and check that it is not included in result.

Commit 8f79c2f fixes this pull-request

Pull-Request has been merged by mprahl

5 years ago

Pull-Request has been merged by mprahl

5 years ago

@otaylor adding the test case in a separate PR would be appreciated. I merged this so I can include this in the next release.