#1032 Prefix the module version and disttag based on the platform it buildrequires
Merged 9 months ago by mprahl. Opened 9 months ago by mprahl.

@@ -411,9 +411,9 @@ 

              'desc': ('The distinguished name of the container or organizational unit containing '

                       'the groups in LDAP')},

          'base_module_names': {

-             'type': set,

-             'default': set(['platform', 'bootstrap']),

-             'desc': ("Set of module names which defines the product version "

+             'type': list,

+             'default': ['platform'],

+             'desc': ("List of base module names which define the product version "

                       "(by their stream) of modules depending on them.")},

          'base_module_koji_arches': {

              'type': dict,

@@ -156,7 +156,7 @@ 

  

      def resolve_profiles(self, mmd, keys):

          """

-         :param mmd: ModuleMetadata instance of module

+         :param mmd: Modulemd.Module instance of module

          :param keys: list of modulemd installation profiles to include in

                       the result.

          :return: Dictionary with keys set according to `keys` param and values

@@ -213,8 +213,8 @@ 

          :param strict: Normally this function returns None if no module can be

              found.  If strict=True, then an UnprocessableEntity is raised.

          :return: a mapping containing buildrequire modules info in key/value pairs,

-             where key is koji_tag and value is the ModuleMetadata object.

-         :rtype: dict(str, :class:`ModuleMetadata`)

+             where key is koji_tag and value is the Modulemd.Module object.

+         :rtype: dict(str, :class:`Modulemd.Module`)

          """

  

          if mmd:

@@ -223,7 +223,7 @@ 

          cg_build_koji_tag = conf.koji_cg_default_build_tag

          if conf.system not in ['koji', 'test']:

              # In case of non-koji backend, we want to get the dependencies

-             # of the local module build based on ModuleMetadata, because the

+             # of the local module build based on Modulemd.Module, because the

              # local build is not stored in the external MBS and therefore we

              # cannot query it using the `query` as for Koji below.

              dependencies = resolver.get_module_build_dependencies(

@@ -212,8 +212,26 @@ 

      mse_build_ids = module_build.siblings + [module_build.id or 0]

      mse_build_ids.sort()

      index = mse_build_ids[0]

-     return "{prefix}{index}+{dist_hash}".format(

+     try:

+         buildrequires = module_build.mmd().get_xmd()['mbs']['buildrequires']

+     except (ValueError, KeyError):

+         log.warn('Module build {0} does not have buildrequires in its xmd'.format(module_build.id))

+         buildrequires = None

+ 

+     base_module_stream = ''

+     if buildrequires:

+         for base_module in conf.base_module_names:

+             base_module_stream = buildrequires.get(base_module, {}).get('stream', '')

+             if base_module_stream:

+                 base_module_stream += '+'

+                 break

+         else:

The mighty for/else block! :)

+             log.warn('Module build {0} does not buildrequire a base module ({1})'

+                      .format(module_build.id, ' or '.join(conf.base_module_names)))

+ 

+     return '{prefix}{base_module_stream}{index}+{dist_hash}'.format(

          prefix=conf.default_dist_tag_prefix,

+         base_module_stream=base_module_stream,

          index=index,

          dist_hash=dist_hash,

      )

@@ -121,7 +121,7 @@ 

      the list of module metadata objects defined by `requires` dict.

  

      :param session: SQLAlchemy DB session.

-     :param requires: Modulemetadata requires or buildrequires.

+     :param requires: Modulemd.Module requires or buildrequires.

      :param mmds: Dictionary with already handled name:streams as a keys and lists

          of resulting mmds as values.

      :param recursive: If True, the requires are checked recursively.

@@ -32,6 +32,7 @@ 

  

  import kobo.rpmlib

  import requests

+ from gi.repository import GLib

  

  import module_build_service.scm

  import module_build_service.resolver

@@ -116,7 +117,7 @@ 

      Prepares the modulemd for the MBS. This does things such as replacing the

      branches of components with commit hashes and adding metadata in the xmd

      dictionary.

-     :param mmd: the ModuleMetadata object to format

+     :param mmd: the Modulemd.Module object to format

      :param scmurl: the url to the modulemd

      """

      # Import it here, because SCM uses utils methods and fails to import

@@ -207,6 +208,70 @@ 

      mmd.set_xmd(glib.dict_values(xmd))

  

  

+ def get_prefixed_version(mmd):

+         """

+         Return the prefixed version of the module based on the buildrequired base module stream.

+ 

+         :param mmd: the Modulemd.Module object to format

+         :return: the prefixed version

+         :rtype: int

+         """

+         xmd = mmd.get_xmd()

+         version = mmd.get_version()

+         base_module_stream = None

+         for base_module in conf.base_module_names:

+             # xmd is a GLib Variant and doesn't support .get() syntax

+             try:

+                 base_module_stream = xmd['mbs']['buildrequires'].get(base_module, {}).get('stream')

+                 if base_module_stream:

+                     # Break after finding the first base module that is buildrequired

+                     break

+             except KeyError:

+                 log.warning('The module\'s mmd is missing information in the xmd section')

+                 return version

+         else:

+             log.warning('This module does not buildrequire a base module ({0})'

+                         .format(' or '.join(conf.base_module_names)))

+             return version

+ 

+         # The platform version (e.g. prefix1.2.0 => 010200)

+         version_prefix = ''

+         for char in base_module_stream:

+             try:

+                 # See if the current character is an integer, signifying the version

+                 # has started

+                 int(char)

+                 version_prefix += char

+             except ValueError:

+                 # If version_prefix isn't set, then a digit hasn't been encountered

+                 if version_prefix:

+                     # If the character is a period and the version_prefix is set, then

+                     # the loop is still processing the version part of the stream

+                     if char == '.':

+                         version_prefix += '.'

+                     # If the version_prefix is set and the character is not a period or

+                     # digit, then the remainder of the stream is a suffix like "-beta"

+                     else:

+                         break

+ 

+         # Remove the periods and pad the numbers if necessary

+         version_prefix = ''.join([section.zfill(2) for section in version_prefix.split('.')])

+ 

+         if not version_prefix:

+             log.warning('The "{0}" stream "{1}" couldn\'t be used to prefix the module\'s '

+                         'version'.format(base_module, base_module_stream))

+             return version

+ 

+         # Since the version must be stored as a number, we convert the string back to

+         # an integer which consequently drops the leading zero if there is one

+         new_version = int(version_prefix + str(version))

+         if new_version > GLib.MAXUINT64:

+             log.warning('The "{0}" stream "{1}" caused the module\'s version prefix to be '

+                         'too long'.format(base_module, base_module_stream))

+             return version

+         return new_version

+ 

+ 

  def validate_mmd(mmd):

      """Validate module metadata

  

@@ -397,12 +462,15 @@ 

      modules = []

  

      for mmd in mmds:

+         # Prefix the version of the modulemd based on the base module it buildrequires

+         version = get_prefixed_version(mmd)

+         mmd.set_version(version)

+         version_str = str(version)

+ 

          log.debug('Checking whether module build already exists: %s.',

-                   ":".join([mmd.get_name(), mmd.get_stream(),

-                            str(mmd.get_version()), mmd.get_context()]))

+                   ":".join([mmd.get_name(), mmd.get_stream(), version_str, mmd.get_context()]))

          module = models.ModuleBuild.get_build_from_nsvc(

-             db.session, mmd.get_name(), mmd.get_stream(), str(mmd.get_version()),

-             mmd.get_context())

+             db.session, mmd.get_name(), mmd.get_stream(), version_str, mmd.get_context())

          if module:

              if module.state != models.BUILD_STATES['failed']:

                  err_msg = ('Module (state=%s) already exists. Only a new build or resubmission of '

@@ -438,7 +506,7 @@ 

                  conf,

                  name=mmd.get_name(),

                  stream=mmd.get_stream(),

-                 version=str(mmd.get_version()),

+                 version=version_str,

                  modulemd=mmd.dumps(),

                  scmurl=url,

                  username=username,

@@ -451,7 +519,7 @@ 

          db.session.commit()

          modules.append(module)

          log.info("%s submitted build of %s, stream=%s, version=%s, context=%s", username,

-                  mmd.get_name(), mmd.get_stream(), mmd.get_version(), mmd.get_context())

+                  mmd.get_name(), mmd.get_stream(), version_str, mmd.get_context())

      return modules

  

  

@@ -869,7 +869,7 @@ 

          build_one = models.ModuleBuild()

          build_one.name = 'testmodule'

          build_one.stream = 'master'

-         build_one.version = 20180205135154

+         build_one.version = '2820180205135154'

          build_one.build_context = 'return_runtime_context'

          build_one.ref_build_context = 'return_runtime_context'

          build_one.runtime_context = '9c690d0e'

@@ -994,7 +994,7 @@ 

          build_one = models.ModuleBuild()

          build_one.name = 'testmodule'

          build_one.stream = 'master'

-         build_one.version = 20180205135154

+         build_one.version = '2820180205135154'

          build_one.build_context = 'return_runtime_context'

          build_one.ref_build_context = 'return_runtime_context'

          build_one.runtime_context = '9c690d0e'

@@ -29,7 +29,8 @@ 

  from module_build_service import models, conf

  from module_build_service.errors import ProgrammingError, ValidationError, UnprocessableEntity

  from tests import (

-     reuse_component_init_data, db, reuse_shared_userspace_init_data, clean_database, init_data)

+     reuse_component_init_data, db, reuse_shared_userspace_init_data, clean_database, init_data,

+     scheduler_init_data)

  import mock

  import koji

  import pytest

@@ -281,6 +282,12 @@ 

          assert release_one == "module+2+b8645bbb"

          assert release_two == "module+2+17e35784"

  

+     def test_get_rpm_release_platform_stream(self):

+         scheduler_init_data(1)

+         build_one = models.ModuleBuild.query.get(2)

+         release = module_build_service.utils.get_rpm_release(build_one)

+         assert release == 'module+f28+2+814cfa39'

+ 

      @pytest.mark.parametrize('scmurl', [

          ('git://pkgs.stg.fedoraproject.org/modules/testmodule.git'

           '?#620ec77321b2ea7b0d67d82992dda3e1d67055b4'),

@@ -579,6 +586,22 @@ 

          is_eol = module_build_service.utils.submit._is_eol_in_pdc('mariadb', '10.1')

          assert is_eol

  

+     def test_get_prefixed_version_f28(self):

+         scheduler_init_data(1)

+         build_one = models.ModuleBuild.query.get(2)

+         v = module_build_service.utils.submit.get_prefixed_version(build_one.mmd())

+         assert v == 2820180205135154

+ 

+     def test_get_prefixed_version_fl701(self):

+         scheduler_init_data(1)

+         build_one = models.ModuleBuild.query.get(2)

+         mmd = build_one.mmd()

+         xmd = glib.from_variant_dict(mmd.get_xmd())

+         xmd['mbs']['buildrequires']['platform']['stream'] = 'fl7.0.1-beta'

+         mmd.set_xmd(glib.dict_values(xmd))

+         v = module_build_service.utils.submit.get_prefixed_version(mmd)

+         assert v == 7000120180205135154

+ 

  

  class DummyModuleBuilder(GenericBuilder):

      """

@@ -685,7 +685,7 @@ 

          assert data['name'] == 'testmodule'

          assert data['scmurl'] == ('git://pkgs.stg.fedoraproject.org/modules/testmodule.git'

                                    '?#68931c90de214d9d13feefbd35246a81b6cb8d49')

-         assert data['version'] == '1'

+         assert data['version'] == '281'

          assert data['time_submitted'] is not None

          assert data['time_modified'] is not None

          assert data['time_completed'] is None

This PR makes it so that if a module buildrequires platform:f7.0.1, then the module version is prefixed with 70001 and the dist tag of the module components will be something like module+f7.0.1+2+814cfa39.

This addresses FACTORY-2924.

@ralph could you please review this when you can?

rebased onto 314688f

9 months ago

3 new commits added

  • Prefix the component disttag with the platform stream
  • Prefix the module version based on the platform it buildrequires
  • Correct the docstrings referring to the old modulemd type
9 months ago

Not a big issue, but wouldn't it make sense to use entries from conf.base_module_names instead of hardcoded "platform"? This applies also to other places where "platform" is.

I'm not sure if that's an issue or not, but I think this is not going to work for versions of base module greater than 18 if you use three x.y.z versioning, because MAXUINT64 is 18446744073709551615 and f19.0.0 would be transformed to 19000020180205135154 if I imagine that right based on the tests. This is probably not an issue for RHEL or Fedora (unless fedora starts doing 28.0.0 base modules for some reason), but just something I wanted to point out.

I'm not sure if that's an issue or not, but I think this is not going to work for versions of base module greater than 18 if you use three x.y.z versioning, because MAXUINT64 is 18446744073709551615 and f19.0.0 would be transformed to 19000020180205135154 if I imagine that right based on the tests. This is probably not an issue for RHEL or Fedora (unless fedora starts doing 28.0.0 base modules for some reason), but just something I wanted to point out.

Yes, it's one of the shortcomings of the design and in the future, we'll likely have to make the module version a string if we encounter these issues.

4 new commits added

  • Prefix the component disttag with the platform stream
  • Prefix the module version based on the platform it buildrequires
  • Set the default base_module_names config option to be platform
  • Correct the docstrings referring to the old modulemd type
9 months ago

4 new commits added

  • Prefix the component disttag with the platform stream
  • Prefix the module version based on the platform it buildrequires
  • Set the default base_module_names config option to be platform
  • Correct the docstrings referring to the old modulemd type
9 months ago

4 new commits added

  • Prefix the component disttag with the platform stream
  • Prefix the module version based on the platform it buildrequires
  • Set the default base_module_names config option to be platform
  • Correct the docstrings referring to the old modulemd type
9 months ago

@jkaluza could you please review again?

5 new commits added

  • Prefix the component disttag with the platform stream
  • Prefix the module version based on the platform it buildrequires
  • Change the config base_module_names to be a list instead of a set to allow ordering by the admin
  • Set the default base_module_names config option to be platform
  • Correct the docstrings referring to the old modulemd type
9 months ago

Pull-Request has been merged by mprahl

9 months ago