#1261 Add support for the base_module_stream_suffix API parameter
Closed 4 years ago by mprahl. Opened 4 years ago by mprahl.

file modified
+2
@@ -128,6 +128,8 @@ 

    Only allowed if ``scratch`` is ``True``.

  - ``rebuild_strategy`` - a string of the desired rebuild strategy (affects what components get

    rebuilt). For the available options, please look at the "Rebuild Strategies" section below.

+ - ``base_module_stream_suffix`` - a string to append at the end of the buildrequired base module

+   streams. If a stream already contains that suffix, it wont be appended.

  

  

  Rebuild Strategies

@@ -658,14 +658,17 @@ 

              )

  

  

- def _handle_base_module_virtual_stream_br(mmd):

+ def _modify_buildtime_streams(mmd, new_streams_func):

      """

-     Translate a base module virtual stream buildrequire to an actual stream on the input modulemd.

+     Modify buildtime streams using the input new_streams_func.

+ 

+     This is a helper method for _handle_base_module_virtual_stream_br and

+     _process_base_module_stream_suffix.

  

      :param Modulemd.ModuleStream mmd: the modulemd to apply the overrides on

+     :param function new_streams: a function that takes the parameters (module_name, module_streams),

+         and returns the streams that should be set on the buildtime dependency.

      """

-     from module_build_service.resolver import system_resolver

- 

      deps = mmd.get_dependencies()

      for dep in deps:

          overridden = False
@@ -675,50 +678,8 @@ 

          new_dep = Modulemd.Dependencies()

  

          for name, streams in brs.items():

-             if name not in conf.base_module_names:

-                 if streams == []:

-                     new_dep.set_empty_buildtime_dependencies_for_module(name)

-                 else:

-                     for stream in streams:

-                         new_dep.add_buildtime_stream(name, stream)

-                 continue

- 

-             new_streams = copy.deepcopy(streams)

-             for i, stream in enumerate(streams):

-                 # Ignore streams that start with a minus sign, since those are handled in the

-                 # MSE code

-                 if stream.startswith("-"):

-                     continue

- 

-                 # Check if the base module stream is available

-                 log.debug('Checking to see if the base module "%s:%s" is available', name, stream)

-                 if system_resolver.get_module_count(name=name, stream=stream) > 0:

-                     continue

- 

-                 # If the base module stream is not available, check if there's a virtual stream

-                 log.debug(

-                     'Checking to see if there is a base module "%s" with the virtual stream "%s"',

-                     name, stream,

-                 )

-                 base_module_mmd = system_resolver.get_latest_with_virtual_stream(

-                     name=name, virtual_stream=stream

-                 )

-                 if not base_module_mmd:

-                     # If there isn't this base module stream or virtual stream available, skip it,

-                     # and let the dep solving code deal with it like it normally would

-                     log.warning(

-                         'There is no base module "%s" with stream/virtual stream "%s"',

-                         name, stream,

-                     )

-                     continue

- 

-                 latest_stream = base_module_mmd.get_stream_name()

-                 log.info(

-                     'Replacing the buildrequire "%s:%s" with "%s:%s", since "%s" is a virtual '

-                     "stream",

-                     name, stream, name, latest_stream, stream

-                 )

-                 new_streams[i] = latest_stream

+             new_streams = new_streams_func(name, streams)

+             if streams != new_streams:

                  overridden = True

  

              if new_streams == []:
@@ -741,6 +702,96 @@ 

              mmd.add_dependencies(new_dep)

  

  

+ def _handle_base_module_virtual_stream_br(mmd):

+     """

+     Translate a base module virtual stream buildrequire to an actual stream on the input modulemd.

+ 

+     :param Modulemd.ModuleStream mmd: the modulemd to apply the overrides on

+     """

+     from module_build_service.resolver import system_resolver

+ 

+     def new_streams_func(name, streams):

Why not define this at the python module level? Usually functions are defined on the fly like this if we want to either hide things, or use closure.

+         if name not in conf.base_module_names:

+             return streams

+ 

+         new_streams = copy.deepcopy(streams)

+         for i, stream in enumerate(streams):

+             # Ignore streams that start with a minus sign, since those are handled in the

+             # MSE code

+             if stream.startswith("-"):

+                 continue

+ 

+             # Check if the base module stream is available

+             log.debug('Checking to see if the base module "%s:%s" is available', name, stream)

+             if system_resolver.get_module_count(name=name, stream=stream) > 0:

+                 continue

+ 

+             # If the base module stream is not available, check if there's a virtual stream

+             log.debug(

+                 'Checking to see if there is a base module "%s" with the virtual stream "%s"',

+                 name, stream,

+             )

+             base_module_mmd = system_resolver.get_latest_with_virtual_stream(

+                 name=name, virtual_stream=stream

+             )

+             if not base_module_mmd:

+                 # If there isn't this base module stream or virtual stream available, skip it,

+                 # and let the dep solving code deal with it like it normally would

+                 log.warning(

+                     'There is no base module "%s" with stream/virtual stream "%s"',

+                     name, stream,

+                 )

+                 continue

+ 

+             latest_stream = base_module_mmd.get_stream_name()

+             log.info(

+                 'Replacing the buildrequire "%s:%s" with "%s:%s", since "%s" is a virtual '

+                 "stream",

+                 name, stream, name, latest_stream, stream

+             )

+             new_streams[i] = latest_stream

+ 

+         return new_streams

+ 

+     _modify_buildtime_streams(mmd, new_streams_func)

+ 

+ 

+ def _process_base_module_stream_suffix(mmd, params):

+     """

+     Process the base_module_stream_suffix parameter.

+ 

+     If provided by the user, the value will be appended to the streams of the base modules that are

+     buildrequired.

+ 

+     :param Modulemd.ModuleStream mmd: the modulemd to apply the overrides on

+     :param dict params: the API parameters passed in by the user

+     """

+     if not params.get("base_module_stream_suffix"):

+         return

+ 

+     def new_streams_func(name, streams):

+         if name not in conf.base_module_names:

+             return streams

+ 

+         new_streams = copy.deepcopy(streams)

+         for i, stream in enumerate(streams):

+             # If the stream already ends with that suffix, it should be skipped

+             if stream.endswith(params["base_module_stream_suffix"]):

+                 continue

+ 

+             new_stream = stream + params["base_module_stream_suffix"]

+             log.info(

+                 'Replacing the buildrequire "%s:%s" with "%s:%s", since "base_module_stream_suffix"'

+                 " was provided with a value of %s",

+                 name, stream, name, new_stream, stream, params["base_module_stream_suffix"]

+             )

+             new_streams[i] = new_stream

+ 

+         return new_streams

+ 

+     _modify_buildtime_streams(mmd, new_streams_func)

+ 

+ 

  def submit_module_build(username, mmd, params):

      """

      Submits new module build.
@@ -774,6 +825,7 @@ 

          default_streams = params["default_streams"]

      _apply_dep_overrides(mmd, params)

      _handle_base_module_virtual_stream_br(mmd)

+     _process_base_module_stream_suffix(mmd, params)

  

      mmds = generate_expanded_mmds(db.session, mmd, raise_if_stream_ambigous, default_streams)

      if not mmds:

@@ -309,6 +309,7 @@ 

          "scmurl",

          "scratch",

          "srpms",

+         "base_module_stream_suffix",

      ])

  

      def __init__(self, request, data=None):
@@ -382,6 +383,12 @@ 

          self._validate_dep_overrides_format("buildrequire_overrides")

          self._validate_dep_overrides_format("require_overrides")

  

+         if (

+             "base_module_stream_suffix" in self.data

+             and not isinstance(self.data["base_module_stream_suffix"], string_types)

+         ):

+             raise ValidationError("The base_module_stream_suffix must be a string")

+ 

  

  class SCMHandler(BaseHandler):

      def validate(self, skip_branch=False, skip_optional_params=False):

@@ -2435,3 +2435,36 @@ 

          dep = mmd.get_dependencies()[0]

          assert dep.get_buildtime_streams("platform") == ["el8.25.0"]

          assert dep.get_runtime_streams("platform") == ["el8"]

+ 

+     @patch("module_build_service.auth.get_user", return_value=user)

+     @patch("module_build_service.scm.SCM")

+     def test_submit_build_request_z_stream(self, mocked_scm, mocked_get_user):

+         # Create a Z stream el8.0.0 platform

+         mmd = load_mmd_file(path.join(base_dir, "staged_data", "platform.yaml"))

+         mmd = mmd.copy(mmd.get_module_name(), "el8.0.0z")

+         import_mmd(db.session, mmd)

+ 

+         # Use a testmodule that buildrequires platform:el8.0.0

+         FakeSCM(

+             mocked_scm,

+             "testmodule",

+             "testmodule_el800.yaml",

+             "620ec77321b2ea7b0d67d82992dda3e1d67055b4",

+         )

+ 

+         post_url = "/module-build-service/2/module-builds/"

+         scm_url = (

+             "https://src.stg.fedoraproject.org/modules/testmodule.git?#"

+             "68931c90de214d9d13feefbd35246a81b6cb8d49"

+         )

+         payload = {"branch": "master", "scmurl": scm_url, "base_module_stream_suffix": "z"}

+         rv = self.client.post(post_url, json=payload)

+         data = json.loads(rv.data)

+ 

+         mmd = load_mmd(data[0]["modulemd"])

+         assert len(mmd.get_dependencies()) == 1

+         dep = mmd.get_dependencies()[0]

+         # The stream suffix should be appended to the buildtime platform stream

+         assert dep.get_buildtime_streams("platform") == ["el8.0.0z"]

+         # The runtime stream suffix should remain unchanged

+         assert dep.get_runtime_streams("platform") == ["el8.0.0"]

In certain use-cases, a module's buildrequires may remain the same in the modulemd, but a different support stream of the buildrequired base module should be used. A common use-case will be supplying base_module_stream_suffix="z" so that platform:el8.0.0 becomes platform:el8.0.0z.

The first commit refactors _handle_base_module_virtual_stream_br so that some of its functionality can be reused in the second commit.

@jkaluza could you please review this since it's high priority?

2 new commits added

  • Add support for the base_module_stream_suffix API parameter
  • Breakout some of the functionality in _handle_base_module_virtual_stream_br so it can be reused for a future feature
4 years ago

Build 8415e1f64128490a9146ed0aa3678189143c0482 FAILED!
Rebase or make new commits to rebuild.

2 new commits added

  • Add support for the base_module_stream_suffix API parameter
  • Breakout some of the functionality in _handle_base_module_virtual_stream_br so it can be reused for a future feature
4 years ago

Why not define this at the python module level? Usually functions are defined on the fly like this if we want to either hide things, or use closure.

I see that the suffix is applied to every base module. Is this going to cause issues later on? For instance, what if I want platform:el8.0.0z and platform:el8.0.1 ?

@lucarval, I'm not sure if this is possible use-case right now, but I can image this to happen when platform:el9 is introduced and people start using platform: [el8, el9]. But this will most likely need even more policies and changes to get this supported with regards of zstream and so on.

@mprahl: Can you please describe more how the whole workflow around this feature is going to look like?

Will the rpkg detect that the suffix should be added to stream and use it in a request?

Is the code to detect that already in the rpkg even for modular branches? I would not like to repeat the issue with buildrequire_overrides when we added the logic to rpkg and then moved it back to MBS.

I'll refactor this to use Product Pages directly instead.

Pull-Request has been closed by mprahl

4 years ago