fm-orchestrator

Clone
Source Code
GIT

#1363 Rewrite import_mmd
Merged 4 years ago by cqi. Opened 4 years ago by cqi.
cqi/fm-orchestrator remove-dropped-virtual-streams  into  master

file modified
+40 -2 
@@ -1093,7 +1093,7 @@ 
 

  

      def get_buildrequired_base_modules(self, db_session):
 

          """
 

-         Find the base modules in the modulemd's xmd section.
 

+         Find the base modules in the modulemd's xmd/mbs/buildrequires section.
 

  

          :param db_session: the SQLAlchemy database session to use to query
 

          :return: a list of ModuleBuild objects of the base modules that are buildrequired with the
 
@@ -1107,7 +1107,7 @@ 
 

              try:
 

                  bm_dict = xmd["mbs"]["buildrequires"].get(bm)
 

              except KeyError:
 

-                 raise RuntimeError("The module's mmd is missing information in the xmd section")
 

+                 raise RuntimeError("The module's mmd is missing xmd/mbs or xmd/mbs/buildrequires.")
 

  

              if not bm_dict:
 

                  continue
 
@@ -1139,6 +1139,40 @@ 
 

              self.state_reason,
 

          )
 

  

+     def update_virtual_streams(self, db_session, virtual_streams):
 

+         """Add and remove virtual streams to and from this build
 

+ 

+         If a virtual stream is only associated with this build, remove it from
 

+         database as well.
 

+ 

+         :param db_session: SQLAlchemy session object.
 

+         :param virtual_streams: list of virtual streams names used to update
 

+             this build's virtual streams.
 

+         :type virtual_streams: list[str]
 

+         """
 

+         orig_virtual_streams = set(item.name for item in self.virtual_streams)
 

+         new_virtual_streams = set(virtual_streams)
 

+ 

+         dropped_virtual_streams = orig_virtual_streams - new_virtual_streams
 

+         newly_added_virtual_streams = new_virtual_streams - orig_virtual_streams
 

+ 

+         for stream_name in newly_added_virtual_streams:
 

+             virtual_stream = VirtualStream.get_by_name(db_session, stream_name)
 

+             if not virtual_stream:
 

+                 virtual_stream = VirtualStream(name=stream_name)
 

+             self.virtual_streams.append(virtual_stream)
 

+ 

+         for stream_name in dropped_virtual_streams:
 

+             virtual_stream = VirtualStream.get_by_name(db_session, stream_name)
 

+             only_associated_with_self = (
 

+                 len(virtual_stream.module_builds) == 1
 

+                 and virtual_stream.module_builds[0].id == self.id
 

+             )
 

+ 

+             self.virtual_streams.remove(virtual_stream)
 

+             if only_associated_with_self:
 

+                 db_session.delete(virtual_stream)
 

+ 

  

  class VirtualStream(MBSBase):
 

      __tablename__ = "virtual_streams"
 
@@ -1151,6 +1185,10 @@ 
 

      def __repr__(self):
 

          return "<VirtualStream id={} name={}>".format(self.id, self.name)
 

  

+     @classmethod
 

+     def get_by_name(cls, db_session, name):
 

+         return db_session.query(cls).filter_by(name=name).first()
 

+ 

  

  class ModuleArch(MBSBase):
 

      __tablename__ = "module_arches"

file modified
+53 -61 
@@ -412,6 +412,8 @@ 
 

      The ModuleBuild.owner is set to "mbs_import".
 

  

      :param db_session: SQLAlchemy session object.
 

+     :param mmd: module metadata being imported into database.
 

+     :type mmd: Modulemd.ModuleStream
 

      :param bool check_buildrequires: When True, checks that the buildrequires defined in the MMD
 

          have matching records in the `mmd["xmd"]["mbs"]["buildrequires"]` and also fills in
 

          the `ModuleBuild.buildrequires` according to this data.
 
@@ -419,31 +421,29 @@ 
 

               log messages collected during import (list)
 

      :rtype: tuple
 

      """
 

+     xmd = mmd.get_xmd()
 

+     # Set some defaults in xmd["mbs"] if they're not provided by the user
 

+     if "mbs" not in xmd:
 

+         xmd["mbs"] = {"mse": True}
 

+ 

      if not mmd.get_context():
 

          mmd.set_context(models.DEFAULT_MODULE_CONTEXT)
 

+ 

+     # NSVC is used for logging purpose later.
 

+     nsvc = mmd.get_nsvc()
 

+     if nsvc is None:
 

+         msg = "Both the name and stream must be set for the modulemd being imported."
 

+         log.error(msg)
 

+         raise UnprocessableEntity(msg)
 

+ 

      name = mmd.get_module_name()
 

      stream = mmd.get_stream_name()
 

      version = str(mmd.get_version())
 

      context = mmd.get_context()
 

  

-     try:
 

-         disttag_marking = mmd.get_xmd()["mbs"]["disttag_marking"]
 

-     except (ValueError, KeyError):
 

-         disttag_marking = None
 

- 

-     try:
 

-         virtual_streams = mmd.get_xmd()["mbs"]["virtual_streams"]
 

-     except (ValueError, KeyError):
 

-         virtual_streams = []
 

+     xmd_mbs = xmd["mbs"]
 

  

-     # Verify that the virtual streams are the correct type
 

-     if virtual_streams and (
 

-         not isinstance(virtual_streams, list)
 

-         or any(not isinstance(vs, string_types) for vs in virtual_streams)
 

-     ):
 

-         msg = "The virtual streams must be a list of strings"
 

-         log.error(msg)
 

-         raise UnprocessableEntity(msg)
 

+     disttag_marking = xmd_mbs.get("disttag_marking")
 

  

      # If it is a base module, then make sure the value that will be used in the RPM disttags
 

      # doesn't contain a dash since a dash isn't allowed in the release field of the NVR
 
@@ -452,46 +452,49 @@ 
 

              msg = "The disttag_marking cannot contain a dash"
 

              log.error(msg)
 

              raise UnprocessableEntity(msg)
 

-         elif not disttag_marking and "-" in stream:
 

+         if not disttag_marking and "-" in stream:
 

              msg = "The stream cannot contain a dash unless disttag_marking is set"
 

              log.error(msg)
 

              raise UnprocessableEntity(msg)
 

  

-     # Log messages collected during import
 

-     msgs = []
 

+     virtual_streams = xmd_mbs.get("virtual_streams", [])
 

  

-     # NSVC is used for logging purpose later.
 

-     try:
 

-         nsvc = ":".join([name, stream, version, context])
 

-     except TypeError:
 

-         msg = "Incomplete NSVC: {}:{}:{}:{}".format(name, stream, version, context)
 

+     # Verify that the virtual streams are the correct type
 

+     if virtual_streams and (
 

+         not isinstance(virtual_streams, list)
 

+         or any(not isinstance(vs, string_types) for vs in virtual_streams)
 

+     ):
 

+         msg = "The virtual streams must be a list of strings"
 

          log.error(msg)
 

          raise UnprocessableEntity(msg)
 

  

-     if len(mmd.get_dependencies()) > 1:
 

-         raise UnprocessableEntity(
 

-             "The imported module's dependencies list should contain just one element")
 

- 

-     xmd = mmd.get_xmd()
 

-     # Set some defaults in xmd["mbs"] if they're not provided by the user
 

-     if "mbs" not in xmd:
 

-         xmd["mbs"] = {"mse": True}
 

- 

-     if check_buildrequires and mmd.get_dependencies():
 

-         brs = set(mmd.get_dependencies()[0].get_buildtime_modules())
 

-         xmd_brs = set(xmd["mbs"].get("buildrequires", {}).keys())
 

-         if brs - xmd_brs:
 

+     if check_buildrequires:
 

+         deps = mmd.get_dependencies()
 

+         if len(deps) > 1:
 

              raise UnprocessableEntity(
 

-                 "The imported module buildrequires other modules, but the metadata in the "
 

-                 'xmd["mbs"]["buildrequires"] dictionary is missing entries'
 

-             )
 

-     elif "buildrequires" not in xmd["mbs"]:
 

-         xmd["mbs"]["buildrequires"] = {}
 

-         mmd.set_xmd(xmd)
 

- 

-     koji_tag = xmd["mbs"].get("koji_tag")
 

-     if koji_tag is None:
 

+                 "The imported module's dependencies list should contain just one element")
 

+ 

+         if "buildrequires" not in xmd_mbs:
 

+             # Always set buildrequires if it is not there, because
 

+             # get_buildrequired_base_modules requires xmd/mbs/buildrequires exists.
 

+             xmd_mbs["buildrequires"] = {}
 

+             mmd.set_xmd(xmd)
 

+ 

+         if len(deps) > 0:
 

+             brs = set(deps[0].get_buildtime_modules())
 

+             xmd_brs = set(xmd_mbs["buildrequires"].keys())
 

+             if brs - xmd_brs:
 

+                 raise UnprocessableEntity(
 

+                     "The imported module buildrequires other modules, but the metadata in the "
 

+                     'xmd["mbs"]["buildrequires"] dictionary is missing entries'
 

+                 )
 

+ 

+     if "koji_tag" not in xmd_mbs:
 

          log.warning("'koji_tag' is not set in xmd['mbs'] for module {}".format(nsvc))
 

+         log.warning("koji_tag will be set to None for imported module build.")
 

+ 

+     # Log messages collected during import
 

+     msgs = []
 

  

      # Get the ModuleBuild from DB.
 

      build = models.ModuleBuild.get_build_from_nsvc(db_session, name, stream, version, context)
 
@@ -501,11 +504,12 @@ 
 

          msgs.append(msg)
 

      else:
 

          build = models.ModuleBuild()
 

+         db_session.add(build)
 

  

      build.name = name
 

      build.stream = stream
 

      build.version = version
 

-     build.koji_tag = koji_tag
 

+     build.koji_tag = xmd_mbs.get("koji_tag")
 

      build.state = models.BUILD_STATES["ready"]
 

      build.modulemd = mmd_to_str(mmd)
 

      build.context = context
 
@@ -523,19 +527,7 @@ 
 

              if base_module not in build.buildrequires:
 

                  build.buildrequires.append(base_module)
 

  

-     db_session.add(build)
 

-     db_session.commit()
 

- 

-     for virtual_stream in virtual_streams:
 

-         vs_obj = db_session.query(models.VirtualStream).filter_by(name=virtual_stream).first()
 

-         if not vs_obj:
 

-             vs_obj = models.VirtualStream(name=virtual_stream)
 

-             db_session.add(vs_obj)
 

-             db_session.commit()
 

- 

-         if vs_obj not in build.virtual_streams:
 

-             build.virtual_streams.append(vs_obj)
 

-             db_session.add(build)
 

+     build.update_virtual_streams(db_session, virtual_streams)
 

  

      db_session.commit()

file modified
+57 
@@ -455,6 +455,63 @@ 
 

          else:
 

              module_build_service.utils.import_mmd(db_session, mmd)
 

  

+     def test_import_mmd_remove_dropped_virtual_streams(self, db_session):
 

+         mmd = load_mmd(read_staged_data("formatted_testmodule"))
 

+ 

+         # Add some virtual streams
 

+         xmd = mmd.get_xmd()
 

+         xmd["mbs"]["virtual_streams"] = ["f28", "f29", "f30"]
 

+         mmd.set_xmd(xmd)
 

+ 

+         # Import mmd into database to simulate the next step to reimport a module
 

+         module_build_service.utils.general.import_mmd(db_session, mmd)
 

+ 

+         # Now, remove some virtual streams from module metadata
 

+         xmd = mmd.get_xmd()
 

+         xmd["mbs"]["virtual_streams"] = ["f28", "f29"]  # Note that, f30 is removed
 

+         mmd.set_xmd(xmd)
 

+ 

+         # Test import modulemd again and the f30 should be removed from database.
 

+         module_build, _ = module_build_service.utils.general.import_mmd(db_session, mmd)

module_build = module_build_service.utils.general.import_mmd(db_session, mmd)[0]

 
+ 

+         db_session.refresh(module_build)
 

+         assert ["f28", "f29"] == sorted(item.name for item in module_build.virtual_streams)
 

+         assert 0 == db_session.query(models.VirtualStream).filter_by(name="f30").count()
 

+ 

+     def test_import_mmd_dont_remove_dropped_virtual_streams_associated_with_other_modules(
 

+         self, db_session
 

+     ):
 

+         mmd = load_mmd(read_staged_data("formatted_testmodule"))
 

+         # Add some virtual streams to this module metadata
 

+         xmd = mmd.get_xmd()
 

+         xmd["mbs"]["virtual_streams"] = ["f28", "f29", "f30"]
 

+         mmd.set_xmd(xmd)
 

+         module_build_service.utils.general.import_mmd(db_session, mmd)
 

+ 

+         # Import another module which has overlapping virtual streams
 

+         another_mmd = load_mmd(read_staged_data("formatted_testmodule-more-components"))
 

+         # Add some virtual streams to this module metadata
 

+         xmd = another_mmd.get_xmd()
 

+         xmd["mbs"]["virtual_streams"] = ["f29", "f30"]
 

+         another_mmd.set_xmd(xmd)
 

+         another_module_build, _ = module_build_service.utils.general.import_mmd(
 

+             db_session, another_mmd)
 

+ 

+         # Now, remove f30 from mmd
 

+         xmd = mmd.get_xmd()
 

+         xmd["mbs"]["virtual_streams"] = ["f28", "f29"]
 

+         mmd.set_xmd(xmd)
 

+ 

+         # Reimport formatted_testmodule again
 

+         module_build, _ = module_build_service.utils.general.import_mmd(db_session, mmd)
 

+ 

+         db_session.refresh(module_build)
 

+         assert ["f28", "f29"] == sorted(item.name for item in module_build.virtual_streams)
 

+ 

+         # The overlapped f30 should be still there.
 

+         db_session.refresh(another_module_build)
 

+         assert ["f29", "f30"] == sorted(item.name for item in another_module_build.virtual_streams)
 

+ 

      def test_get_rpm_release_mse(self, db_session):
 

          init_data(contexts=True)

file modified
+2 -1 
@@ -2019,7 +2019,8 @@ 
 

          data = json.loads(rv.data)
 

  

          assert data["error"] == "Unprocessable Entity"
 

-         assert data["message"] == "Incomplete NSVC: None:None:0:00000000"
 

+         expected_msg = "Both the name and stream must be set for the modulemd being imported."
 

+         assert data["message"] == expected_msg
 

  

      @pytest.mark.parametrize("api_version", [1, 2])
 

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

  • xmd/mbs is always set if it is not present in xmd, so move the code on
    the top of function. This change is also helpful for accessing keys
    under xmd/mbs.
  • By setting xmd/mbs in the beginning, code is simplified to get
    disttag_marking and virtual_streams. The result is much straightforwar
    for getting a default value for them.
  • Move disttag_marking validation code next to the line getting
    disttag_marking from xmd/mbs. As a result, the code structure is
    easier to read as getting disttag_marking and validate it, getting
    virtual_streams and validate.
  • Rewrite the part of code for check_buildrequires. Always set
    xmd/mbs/buildrequires if it is not present and check_buildrequires is
    set to True, as it is required by
    ModuleBuild.get_buildrequired_base_modules.
  • Using in operator instead of dict.get to check if key koji_tag exists.
    Using dict.get would be ambiguous because even if koji_tag exists
    under xmd/mbs, but due to its value is set to None occasionally, there
    is still a message logged to tell koji_tag is not set.
  • Rwrite all lines of code for updating virtual streams. A new method
    update_virtual_streams is added to ModuleBuild. This also fixes
    FACTORY-4561.
  • Tests are added for the rewrite of virtual streams update.

Signed-off-by: Chenxiong Qi cqi@redhat.com

module_build = module_build_service.utils.general.import_mmd(db_session, mmd)[0]

pretty please pagure-ci rebuild
4 years ago

module_build = module_build_service.utils.general.import_mmd(db_session, mmd)[0]

@vmaljulin Thanks. I personally like current way to get the result as it is much straightforward than getting via an index from a list or tuple.

Instead of a try and except. Could you do return db_session.query(cls).filter_by(name=name).first()?

In the future, changes like this should be in a separate commit from any refactoring if possible.

How about the following?

msg = "Both the name and stream must be set for the modulemd being imported"

In this case, there should be an empty dict in xmd.mbs.buildrequires.

This should be if "koji_tag" not in xmd_mbs:

rebased onto 116597c7a85d6accd3dc5aedab979caa5ab3b320
4 years ago

rebased onto b600cee67dfaad3d3f093062c72eea57772798a0
4 years ago

@mprahl Thanks. All comments are addressed.

Build #315 failed (commit: b600cee67dfaad3d3f093062c72eea57772798a0).
Rebase or make new commits to rebuild.

rebased onto f7bb71138d6e0dbb7791fff9393a56467f8266c2
4 years ago

rebased onto 9c6c4da
4 years ago

:thumbsup:

Commit b5df145 fixes this pull-request

Pull-Request has been merged by cqi
4 years ago

Pull-Request has been merged by cqi
4 years ago
Changes Summary 4
+40 -2
file changed
module_build_service/models.py
+53 -61
file changed
module_build_service/utils/general.py
+57 -0
file changed
tests/test_utils/test_utils.py
+2 -1
file changed
tests/test_views/test_views.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.