#973 Import module API
Merged 7 months ago by mprahl. Opened 9 months ago by fivaldi.
fivaldi/fm-orchestrator master  into  master

file modified
+69

@@ -667,6 +667,75 @@ 

  - ``task_id``

  

  

+ Import module

+ -------------

+ 

+ Importing of modules is done via posting the SCM URL of a repository

+ which contains the generated modulemd YAML file. Name, stream, version,

+ context and other important information must be present in the metadata.

+ 

+ ::

+ 

+     POST /module-build-service/1/import-module/

+ 

+ ::

+ 

+     {

+       "scmurl": "git://pkgs.fedoraproject.org/modules/foo.git?#21f92fb05572d81d78fd9a27d313942d45055840"

+     }

+ 

+ 

+ If the module build is imported successfully, JSON containing the most

+ important information is returned from MBS. The JSON also contains log

+ messages collected during the import.

+ 

+ ::

+ 

+     HTTP 201 Created

+ 

+ ::

+ 

+     {

+       "module": {

+         "component_builds": [],

+         "context": "00000000",

+         "id": 3,

+         "koji_tag": "",

+         "name": "mariadb",

+         "owner": "mbs_import",

+         "rebuild_strategy": "all",

+         "scmurl": null,

+         "siblings": [],

+         "state": 5,

+         "state_name": "ready",

+         "state_reason": null,

+         "stream": "10.2",

+         "time_completed": "2018-07-24T12:58:14Z",

+         "time_modified": "2018-07-24T12:58:14Z",

+         "time_submitted": "2018-07-24T12:58:14Z",

+         "version": "20180724000000"

+       },

+       "messages": [

+         "Updating existing module build mariadb:10.2:20180724000000:00000000.",

+         "Module mariadb:10.2:20180724000000:00000000 imported"

+       ]

+     }

+ 

+ 

+ If the module import fails, an error message is returned.

+ 

+ ::

+ 

+     HTTP 422 Unprocessable Entity

+ 

+ ::

+ 

+     {

+       "error": "Unprocessable Entity",

+       "message": "Incomplete NSVC: None:None:0:00000000"

+     }

+ 

+ 

  Listing about

  -------------

  

file modified
+4

@@ -62,6 +62,8 @@ 

          # 'modularity-wg',

      ])

  

+     ALLOWED_GROUPS_TO_IMPORT_MODULE = set()

+ 

      # Available backends are: console and file

      LOG_BACKEND = 'console'

  

@@ -120,6 +122,8 @@ 

      AUTH_METHOD = 'oidc'

      RESOLVER = 'db'

  

+     ALLOWED_GROUPS_TO_IMPORT_MODULE = set(['mbs-import-module'])

+ 

  

  class ProdConfiguration(BaseConfiguration):

      pass

@@ -247,6 +247,10 @@ 

              'type': set,

              'default': set(['packager']),

              'desc': 'The set of groups allowed to submit builds.'},

+         'allowed_groups_to_import_module': {

+             'type': set,

+             'default': set(),

+             'desc': 'The set of groups allowed to import module builds.'},

          'log_backend': {

              'type': str,

              'default': None,

@@ -350,7 +354,7 @@ 

          'yaml_submit_allowed': {

              'type': bool,

              'default': False,

-             'desc': 'Is it allowed to directly submit modulemd yaml file?'},

+             'desc': 'Is it allowed to directly submit build by modulemd yaml file?'},

          'num_concurrent_builds': {

              'type': int,

              'default': 0,

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

  from datetime import datetime

  

  from module_build_service import conf, log, models

- from module_build_service.errors import ValidationError, ProgrammingError

+ from module_build_service.errors import (

+     ValidationError, ProgrammingError, UnprocessableEntity)

  

  

  def scm_url_schemes(terse=False):

@@ -258,6 +259,10 @@ 

      the module, we have no idea what build_context or runtime_context is - we only

      know the resulting "context", but there is no way to store it into do DB.

      By now, we just ignore mmd.get_context() and use default 00000000 context instead.

+ 

+     :return: module build (ModuleBuild),

+              log messages collected during import (list)

+     :rtype: tuple

      """

      mmd.set_context("00000000")

      name = mmd.get_name()

@@ -265,22 +270,33 @@ 

      version = str(mmd.get_version())

      context = mmd.get_context()

  

+     # Log messages collected during import

+     msgs = []

+ 

      # NSVC is used for logging purpose later.

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

+     try:

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

+     except TypeError:

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

+         log.error(msg)

+         raise UnprocessableEntity(msg)

  

      # Get the koji_tag.

-     xmd = mmd.get_xmd()

-     if "mbs" in xmd.keys() and "koji_tag" in xmd["mbs"].keys():

+     try:

+         xmd = mmd.get_xmd()

          koji_tag = xmd["mbs"]["koji_tag"]

-     else:

-         log.warn("'koji_tag' is not set in xmd['mbs'] for module %s", nsvc)

-         koji_tag = ""

+     except KeyError:

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

+         log.error(msg)

+         raise UnprocessableEntity(msg)

  

      # Get the ModuleBuild from DB.

      build = models.ModuleBuild.get_build_from_nsvc(

          session, name, stream, version, context)

      if build:

-         log.info("Updating existing module build %s.", nsvc)

+         msg = "Updating existing module build {}.".format(nsvc)

+         log.info(msg)

+         msgs.append(msg)

      else:

          build = models.ModuleBuild()

  

@@ -298,4 +314,22 @@ 

      build.time_completed = datetime.utcnow()

      session.add(build)

      session.commit()

-     log.info("Module %s imported", nsvc)

+     msg = "Module {} imported".format(nsvc)

+     log.info(msg)

+     msgs.append(msg)

+ 

+     return build, msgs

+ 

+ 

+ def get_mmd_from_scm(url):

+     """

+     Provided an SCM URL, fetch mmd from the corresponding module YAML

+     file. If ref is specified within the URL, the mmd will be returned

+     as of the ref.

+     """

+     from module_build_service.utils.submit import _fetch_mmd

+ 

+     mmd, _ = _fetch_mmd(url, branch=None, allow_local_url=False,

+                         whitelist_url=False, mandatory_checks=False)

+ 

+     return mmd

@@ -462,7 +462,8 @@ 

      return not results[0]['active']

  

  

- def _fetch_mmd(url, branch=None, allow_local_url=False, whitelist_url=False):

+ def _fetch_mmd(url, branch=None, allow_local_url=False, whitelist_url=False,

+                mandatory_checks=True):

      # Import it here, because SCM uses utils methods

      # and fails to import them because of dep-chain.

      import module_build_service.scm

@@ -477,7 +478,7 @@ 

          else:

              scm = module_build_service.scm.SCM(url, branch, conf.scmurls, allow_local_url)

          scm.checkout(td)

-         if not whitelist_url:

+         if not whitelist_url and mandatory_checks:

              scm.verify()

          cofn = scm.get_module_yaml()

          mmd = load_mmd(cofn, is_file=True)

@@ -495,6 +496,9 @@ 

              raise ValidationError(

                  'Module {}:{} is marked as EOL in PDC.'.format(scm.name, scm.branch))

  

+     if not mandatory_checks:

+         return mmd, scm

+ 

      # If the name was set in the modulemd, make sure it matches what the scmurl

      # says it should be

      if mmd.get_name() and mmd.get_name() != scm.name:

@@ -36,10 +36,14 @@ 

  

  

  def get_scm_url_re():

+     """

+     Returns a regular expression for SCM URL extraction and validation.

+     """

      schemes_re = '|'.join(map(re.escape, scm_url_schemes(terse=True)))

-     return re.compile(

-         r"(?P<giturl>(?:(?P<scheme>(" + schemes_re + r"))://(?P<host>[^/]+))?"

-         r"(?P<repopath>/[^\?]+))\?(?P<modpath>[^#]*)#(?P<revision>.+)")

+     regex = (

This code didn't change other than formatting so it shouldn't be part of this commit.

Please, check the code again. There are significant/functional changes in the regex.

I see the differences now. Could you please explain why you changed this regex?

Because it actually fixes what it supports compared to the intention before, what it should support, I mean the modpath portion of the regexp. If supplied before, it didn't pass the validation.

You're right, this is an improvement.

+         r"(?P<giturl>(?P<scheme>(?:" + schemes_re + r"))://(?P<host>[^/]+)?"

+         r"(?P<repopath>/[^\?]+))(?:\?(?P<modpath>[^#]+)?)?#(?P<revision>.+)")

+     return re.compile(regex)

  

  

  def pagination_metadata(p_query, api_version, request_args):

file modified
+56 -10

@@ -36,7 +36,8 @@ 

  from module_build_service.utils import (

      pagination_metadata, filter_module_builds, filter_component_builds,

      submit_module_build_from_scm, submit_module_build_from_yaml,

-     get_scm_url_re, cors_header, validate_api_version)

+     get_scm_url_re, cors_header, validate_api_version, import_mmd,

+     get_mmd_from_scm)

  from module_build_service.errors import (

      ValidationError, Forbidden, NotFound, ProgrammingError)

  from module_build_service.backports import jsonify

@@ -86,6 +87,12 @@ 

          'options': {

              'methods': ['GET']

          }

+     },

+     'import_module': {

+         'url': '/module-build-service/<int:api_version>/import-module/',

+         'options': {

+             'methods': ['POST'],

+         }

      }

  }

  

@@ -152,6 +159,12 @@ 

      query_filter = staticmethod(filter_module_builds)

      model = models.ModuleBuild

  

+     @staticmethod

+     def check_groups(username, groups, allowed_groups=conf.allowed_groups):

+         if allowed_groups and not (allowed_groups & groups):

+             raise Forbidden("%s is not in any of %r, only %r" % (

+                 username, allowed_groups, groups))

+ 

      # Additional POST and DELETE handlers for modules follow.

      @validate_api_version()

      def post(self, api_version):

@@ -163,9 +176,7 @@ 

          if conf.no_auth is True and handler.username == "anonymous" and "owner" in handler.data:

              handler.username = handler.data["owner"]

  

-         if conf.allowed_groups and not (conf.allowed_groups & handler.groups):

-             raise Forbidden("%s is not in any of  %r, only %r" % (

-                 handler.username, conf.allowed_groups, handler.groups))

+         self.check_groups(handler.username, handler.groups)

  

          handler.validate()

          modules = handler.post()

@@ -193,9 +204,7 @@ 

              elif username == "anonymous":

                  username = r["owner"]

  

-         if conf.allowed_groups and not (conf.allowed_groups & groups):

-             raise Forbidden("%s is not in any of  %r, only %r" % (

-                 username, conf.allowed_groups, groups))

+         self.check_groups(username, groups)

  

          module = models.ModuleBuild.query.filter_by(id=id).first()

          if not module:

@@ -268,6 +277,36 @@ 

          return jsonify({'items': items}), 200

  

  

+ class ImportModuleAPI(MethodView):

+ 

+     @validate_api_version()

+     def post(self, api_version):

+         # disable this API endpoint if no groups are defined

+         if not conf.allowed_groups_to_import_module:

+             log.error(

+                 "Import module API is disabled. Set 'ALLOWED_GROUPS_TO_IMPORT_MODULE'"

+                 " configuration value first.")

+             raise Forbidden(

+                 "Import module API is disabled.")

Optional: This should be on the previous line.

+ 

+         # auth checks

+         username, groups = module_build_service.auth.get_user(request)

+         ModuleBuildAPI.check_groups(username, groups,

+                                     allowed_groups=conf.allowed_groups_to_import_module)

+ 

+         # process request using SCM handler

+         handler = SCMHandler(request)

+         handler.validate(skip_branch=True, skip_optional_params=True)

+ 

+         mmd = get_mmd_from_scm(handler.data["scmurl"])

+         build, messages = import_mmd(db.session, mmd)

+         json_data = {"module": build.json(show_tasks=False),

+                      "messages": messages}

+ 

+         # return 201 Created if we reach this point

+         return jsonify(json_data), 201

+ 

+ 

  class BaseHandler(object):

      def __init__(self, request):

          self.username, self.groups = module_build_service.auth.get_user(request)

@@ -310,7 +349,7 @@ 

              log.error('Invalid JSON submitted')

              raise ValidationError('Invalid JSON submitted')

  

-     def validate(self):

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

          if "scmurl" not in self.data:

              log.error('Missing scmurl')

              raise ValidationError('Missing scmurl')

@@ -325,11 +364,12 @@ 

              log.error("The submitted scmurl %r is not valid" % url)

              raise Forbidden("The submitted scmurl %s is not valid" % url)

  

-         if "branch" not in self.data:

+         if not skip_branch and "branch" not in self.data:

              log.error('Missing branch')

              raise ValidationError('Missing branch')

  

-         self.validate_optional_params()

+         if not skip_optional_params:

+             self.validate_optional_params()

  

      def post(self):

          url = self.data["scmurl"]

@@ -369,6 +409,7 @@ 

      component_view = ComponentBuildAPI.as_view('component_builds')

      about_view = AboutAPI.as_view('about')

      rebuild_strategies_view = RebuildStrategies.as_view('rebuild_strategies')

+     import_module = ImportModuleAPI.as_view('import_module')

      for key, val in api_routes.items():

          if key.startswith('component_build'):

              app.add_url_rule(val['url'],

@@ -390,6 +431,11 @@ 

                               endpoint=key,

                               view_func=rebuild_strategies_view,

                               **val['options'])

+         elif key == 'import_module':

+             app.add_url_rule(val['url'],

+                              endpoint=key,

+                              view_func=import_module,

+                              **val['options'])

          else:

              raise NotImplementedError("Unhandled api key.")

  

@@ -0,0 +1,1 @@ 

+ ref: refs/heads/master

@@ -0,0 +1,4 @@ 

+ [core]

+ 	repositoryformatversion = 0

+ 	filemode = true

+ 	bare = true

@@ -0,0 +1,1 @@ 

+ Unnamed repository; edit this file 'description' to name the repository.

@@ -0,0 +1,6 @@ 

+ # git ls-files --others --exclude-from=.git/info/exclude

+ # Lines that start with '#' are comments.

+ # For a project mostly in C, the following would be a good set of

+ # exclude patterns (uncomment them if you want to use them):

+ # *.[oa]

+ # *~

@@ -0,0 +1,1 @@ 

+ 9ab5fdeba83eb3382413ee8bc06299344ef4477d	refs/heads/master

@@ -0,0 +1,2 @@ 

+ P pack-92fbbdbf4fa07dc9cab035120eb248da930e0bd6.pack

+ 

@@ -0,0 +1,2 @@ 

+ # pack-refs with: peeled fully-peeled 

+ 9ab5fdeba83eb3382413ee8bc06299344ef4477d refs/heads/master

empty or binary file added
empty or binary file added
file modified
+18 -17

@@ -30,7 +30,8 @@ 

  import module_build_service.scm

  from module_build_service.errors import ValidationError, UnprocessableEntity

  

- repo_path = 'file://' + os.path.dirname(__file__) + "/scm_data/testrepo"

+ base_dir = os.path.join(os.path.dirname(__file__), 'scm_data')

+ repo_url = 'file://' + base_dir + '/testrepo'

  

  

  class TestSCMModule:

@@ -45,14 +46,14 @@ 

  

      def test_simple_local_checkout(self):

          """ See if we can clone a local git repo. """

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          scm.checkout(self.tempdir)

          files = os.listdir(self.repodir)

          assert 'foo' in files, "foo not in %r" % files

  

      def test_local_get_latest_is_sane(self):

          """ See that a hash is returned by scm.get_latest. """

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          latest = scm.get_latest('master')

          target = '5481faa232d66589e660cc301179867fb00842c9'

          assert latest == target, "%r != %r" % (latest, target)

@@ -62,7 +63,7 @@ 

  

          https://pagure.io/fm-orchestrator/issue/329

          """

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          assert scm.scheme == 'git', scm.scheme

          fname = tempfile.mktemp(suffix='mbs-scm-test')

          try:

@@ -71,70 +72,70 @@ 

              assert not os.path.exists(fname), "%r exists!  Vulnerable." % fname

  

      def test_local_extract_name(self):

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          target = 'testrepo'

          assert scm.name == target, '%r != %r' % (scm.name, target)

  

      def test_local_extract_name_trailing_slash(self):

-         scm = module_build_service.scm.SCM(repo_path + '/')

+         scm = module_build_service.scm.SCM(repo_url + '/')

          target = 'testrepo'

          assert scm.name == target, '%r != %r' % (scm.name, target)

  

      def test_verify(self):

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          scm.checkout(self.tempdir)

          scm.verify()

  

      def test_verify_unknown_branch(self):

          with pytest.raises(UnprocessableEntity):

-             module_build_service.scm.SCM(repo_path, "unknown")

+             module_build_service.scm.SCM(repo_url, "unknown")

  

      def test_verify_commit_in_branch(self):

          target = '7035bd33614972ac66559ac1fdd019ff6027ad21'

-         scm = module_build_service.scm.SCM(repo_path + "?#" + target, "dev")

+         scm = module_build_service.scm.SCM(repo_url + "?#" + target, "dev")

          scm.checkout(self.tempdir)

          scm.verify()

  

      def test_verify_commit_not_in_branch(self):

          target = '7035bd33614972ac66559ac1fdd019ff6027ad21'

-         scm = module_build_service.scm.SCM(repo_path + "?#" + target, "master")

+         scm = module_build_service.scm.SCM(repo_url + "?#" + target, "master")

          scm.checkout(self.tempdir)

          with pytest.raises(ValidationError):

              scm.verify()

  

      def test_verify_unknown_hash(self):

          target = '7035bd33614972ac66559ac1fdd019ff6027ad22'

-         scm = module_build_service.scm.SCM(repo_path + "?#" + target, "master")

+         scm = module_build_service.scm.SCM(repo_url + "?#" + target, "master")

          with pytest.raises(UnprocessableEntity):

              scm.checkout(self.tempdir)

  

      def test_get_module_yaml(self):

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          scm.checkout(self.tempdir)

          scm.verify()

          with pytest.raises(UnprocessableEntity):

              scm.get_module_yaml()

  

      def test_get_latest_incorrect_component_branch(self):

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          with pytest.raises(UnprocessableEntity):

              scm.get_latest('foobar')

  

      def test_get_latest_component_branch(self):

          ref = "5481faa232d66589e660cc301179867fb00842c9"

          branch = "master"

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          commit = scm.get_latest(branch)

          assert commit == ref

  

      def test_get_latest_component_ref(self):

          ref = "5481faa232d66589e660cc301179867fb00842c9"

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          commit = scm.get_latest(ref)

          assert commit == ref

  

      def test_get_latest_incorrect_component_ref(self):

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          with pytest.raises(UnprocessableEntity):

              scm.get_latest('15481faa232d66589e660cc301179867fb00842c9')

  

@@ -146,6 +147,6 @@ 

  10a651f39911a07d85fe87fcfe91999545e44ae0\trefs/remotes/origin/master

  """

          mock_run.return_value = (0, output, '')

-         scm = module_build_service.scm.SCM(repo_path)

+         scm = module_build_service.scm.SCM(repo_url)

          commit = scm.get_latest(None)

          assert commit == '58379ef7887cbc91b215bacd32430628c92bc869'

@@ -33,6 +33,7 @@ 

  import pytest

  

  from tests import app, init_data, clean_database, reuse_component_init_data

+ from tests.test_scm import base_dir as scm_base_dir

  from module_build_service.errors import UnprocessableEntity

  from module_build_service.models import ModuleBuild

  from module_build_service import db, version, Modulemd

@@ -43,6 +44,7 @@ 

  user = ('Homer J. Simpson', set(['packager']))

  other_user = ('some_other_user', set(['packager']))

  anonymous_user = ('anonymous', set(['packager']))

+ import_module_user = ('Import M. King', set(['mbs-import-module']))

  base_dir = dirname(dirname(__file__))

  

  

@@ -1191,3 +1193,176 @@ 

      def test_cors_header_decorator(self):

          rv = self.client.get('/module-build-service/1/module-builds/')

          assert rv.headers['Access-Control-Allow-Origin'] == '*'

+ 

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

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

+     @patch.object(module_build_service.config.Config, 'allowed_groups_to_import_module',

+                   new_callable=PropertyMock, return_value=set())

+     def test_import_build_disabled(self, mocked_groups, mocked_get_user, api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(post_url)

+         data = json.loads(rv.data)

+ 

+         assert data['error'] == 'Forbidden'

+         assert data['message'] == (

+             'Import module API is disabled.')

+ 

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

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

+     def test_import_build_user_not_allowed(self, mocked_get_user, api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(post_url)

+         data = json.loads(rv.data)

+ 

+         assert data['error'] == 'Forbidden'

+         assert data['message'] == (

+             'Homer J. Simpson is not in any of '

+             'set([\'mbs-import-module\']), only set([\'packager\'])')

+ 

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

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

+     def test_import_build_scm_invalid_json(self, mocked_get_user, api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(post_url, data='')

+         data = json.loads(rv.data)

+ 

+         assert data['error'] == 'Bad Request'

+         assert data['message'] == 'Invalid JSON submitted'

+ 

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

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

+     def test_import_build_scm_url_not_allowed(self, mocked_get_user, api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(

+             post_url,

+             data=json.dumps({'scmurl': 'file://' + scm_base_dir + '/mariadb'}))

+         data = json.loads(rv.data)

+ 

+         assert data['error'] == 'Forbidden'

+         assert data['message'].startswith('The submitted scmurl ')

+         assert data['message'].endswith('/tests/scm_data/mariadb is not allowed')

+ 

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

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

+     @patch.object(module_build_service.config.Config, 'allow_custom_scmurls',

+                   new_callable=PropertyMock, return_value=True)

+     def test_import_build_scm_url_not_in_list(self, mocked_scmurls, mocked_get_user,

+                                               api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(

+             post_url,

+             data=json.dumps({'scmurl': 'file://' + scm_base_dir + (

+                 '/mariadb?#b17bea85de2d03558f24d506578abcfcf467e5bc')}))

+         data = json.loads(rv.data)

+ 

+         assert data['error'] == 'Forbidden'

+         assert data['message'].endswith(

+             '/tests/scm_data/mariadb?#b17bea85de2d03558f24d506578abcfcf467e5bc '

+             'is not in the list of allowed SCMs')

+ 

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

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

+     @patch.object(module_build_service.config.Config, 'scmurls',

+                   new_callable=PropertyMock, return_value=['file://'])

+     def test_import_build_scm(self, mocked_scmurls, mocked_get_user, api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(

+             post_url,

+             data=json.dumps({'scmurl': 'file://' + scm_base_dir + (

+                 '/mariadb?#7cf8fb26db8dbfea075eb5f898cc053139960250')}))

+         data = json.loads(rv.data)

+ 

+         assert 'Module mariadb:10.2:20180724000000:00000000 imported' in data['messages']

+         assert data['module']['name'] == 'mariadb'

+         assert data['module']['stream'] == '10.2'

+         assert data['module']['version'] == '20180724000000'

+         assert data['module']['context'] == '00000000'

+         assert data['module']['owner'] == 'mbs_import'

+         assert data['module']['state'] == 5

+         assert data['module']['state_reason'] is None

+         assert data['module']['state_name'] == 'ready'

+         assert data['module']['scmurl'] is None

+         assert data['module']['component_builds'] == []

+         assert data['module']['time_submitted'] == data['module']['time_modified'] == \

+             data['module']['time_completed']

+         assert data['module']['koji_tag'] == 'mariadb-10.2-20180724000000-00000000'

+         assert data['module']['siblings'] == []

+         assert data['module']['rebuild_strategy'] == 'all'

+ 

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

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

+     @patch.object(module_build_service.config.Config, 'scmurls',

+                   new_callable=PropertyMock, return_value=['file://'])

+     def test_import_build_scm_another_commit_hash(self, mocked_scmurls, mocked_get_user,

+                                                   api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(

+             post_url,

+             data=json.dumps({'scmurl': 'file://' + scm_base_dir + (

+                 '/mariadb?#1a43ea22cd32f235c2f119de1727a37902a49f20')}))

+         data = json.loads(rv.data)

+ 

+         assert 'Module mariadb:10.2:20180724065109:00000000 imported' in data['messages']

+         assert data['module']['name'] == 'mariadb'

+         assert data['module']['stream'] == '10.2'

+         assert data['module']['version'] == '20180724065109'

+         assert data['module']['context'] == '00000000'

+         assert data['module']['owner'] == 'mbs_import'

+         assert data['module']['state'] == 5

+         assert data['module']['state_reason'] is None

+         assert data['module']['state_name'] == 'ready'

+         assert data['module']['scmurl'] is None

+         assert data['module']['component_builds'] == []

+         assert data['module']['time_submitted'] == data['module']['time_modified'] == \

+             data['module']['time_completed']

+         assert data['module']['koji_tag'] == 'mariadb-10.2-20180724065109-00000000'

+         assert data['module']['siblings'] == []

+         assert data['module']['rebuild_strategy'] == 'all'

+ 

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

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

+     @patch.object(module_build_service.config.Config, 'scmurls',

+                   new_callable=PropertyMock, return_value=['file://'])

+     def test_import_build_scm_incomplete_nsvc(self, mocked_scmurls, mocked_get_user,

+                                               api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(

+             post_url,

+             data=json.dumps({'scmurl': 'file://' + scm_base_dir + (

+                 '/mariadb?#b17bea85de2d03558f24d506578abcfcf467e5bc')}))

+         data = json.loads(rv.data)

+ 

+         assert data['error'] == 'Unprocessable Entity'

+         assert data['message'] == 'Incomplete NSVC: None:None:0:00000000'

+ 

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

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

+     @patch.object(module_build_service.config.Config, 'scmurls',

+                   new_callable=PropertyMock, return_value=['file://'])

+     def test_import_build_scm_yaml_is_bad(self, mocked_scmurls, mocked_get_user,

+                                           api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(

+             post_url,

+             data=json.dumps({'scmurl': 'file://' + scm_base_dir + (

+                 '/mariadb?#cb7cf7069059141e0797ad2cf5a559fb673ef43d')}))

+         data = json.loads(rv.data)

+ 

+         assert data['error'] == 'Unprocessable Entity'

+         assert data['message'].startswith('The following invalid modulemd was encountered')

+ 

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

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

+     @patch.object(module_build_service.config.Config, 'scmurls',

+                   new_callable=PropertyMock, return_value=['file://'])

+     def test_import_build_scm_missing_koji_tag(self, mocked_scmurls, mocked_get_user,

+                                                api_version):

+         post_url = '/module-build-service/{0}/import-module/'.format(api_version)

+         rv = self.client.post(

+             post_url,

+             data=json.dumps({'scmurl': 'file://' + scm_base_dir + (

+                 '/mariadb?#9ab5fdeba83eb3382413ee8bc06299344ef4477d')}))

+         data = json.loads(rv.data)

+ 

+         assert data['error'] == 'Unprocessable Entity'

+         assert data['message'].startswith('\'koji_tag\' is not set in xmd[\'mbs\'] for module')

no initial comment

Could you default this to an empty set? If the MBS administrator wants to enable this, then they can configure their own instance and not rely on defaults.

1 new commit added

  • Add docs for import module API
9 months ago

1 new commit added

  • Empty set of groups allowed to import module(s)
9 months ago

3 new commits added

  • Empty set of groups allowed to import module(s)
  • Add docs for import module API
  • Import module API
9 months ago

3 new commits added

  • Empty set of groups allowed to import module(s)
  • Add docs for import module API
  • Import module API
9 months ago

3 new commits added

  • Empty set of groups allowed to import module(s)
  • Add docs for import module API
  • Import module API
9 months ago

1 new commit added

  • Handle failed module imports gracefully
9 months ago

4 new commits added

  • Handle failed module imports gracefully
  • Empty set of groups allowed to import module(s)
  • Add docs for import module API
  • Import module API
9 months ago

4 new commits added

  • Handle failed module imports gracefully
  • Empty set of groups allowed to import module(s)
  • Add docs for import module API
  • Import module API
9 months ago

4 new commits added

  • Handle failed module imports gracefully
  • Empty set of groups allowed to import module(s)
  • Add docs for import module API
  • Import module API
9 months ago

Could you document this repopath_only parameter?

Could you put this function in utils/general.py? I think it'd be a better place for it.

Should allowed_groups_to_import_module be capitalized because Flask configurations have to be all caps?

I'm going to finish reviewing it tomorrow.

1 new commit added

  • Review fixes
9 months ago

I'll wait for your comments on SCMHandler static methods as they're similar in the fashion, so I would move it either all or nothing.

Thank you.

5 new commits added

  • Review fixes
  • Handle failed module imports gracefully
  • Empty set of groups allowed to import module(s)
  • Add docs for import module API
  • Import module API
9 months ago

Could you remove the default for allowed_groups? I'd like for the developer to make a conscious decision as to what groups should be allowed.

Adding a docstring to this would also be nice.

Was it a requirement to support direct mmd submissions? I'm thinking we probably want to always enforce our pseudo modules to be stored in SCM since this gives us a history of changes we can audit.

I'll leave it up to you, but I'd prefer if this code was removed since it's unnecessary and adds unnecessary complexity to MBS.

What's the purpose of this variable? Why not use db.session directly?

It seems odd to me that the API supports partial changes and failures. As a user of this API, I'd expect nothing to change if one of my modules can't be imported.

Alternatively, you could stop allowing the submission of multiple SCM URLs. Although it is nice, I think it'd be more intuitive to have an API that just handles one at at time. That way the status code could actually indicate success or failure.

This should be a failure and instead this would end up returning a 200 status code.

@fivaldi could you set the scmurl property on the module in the DB so we can keep track of where the pseudo module came from?

rebased onto 84ba392a30e6b768e1078bc00c42a36c83400b91

9 months ago

From IRC - Filip says he's stuck with this one a bit. He's going to switch tasks for the moment and then cycle back to this one afterwards.

rebased onto f218ffc66b3045b750503aa8cfdc7fffe0f9f8ac

8 months ago

The PR has been updated with the discussed changes except for the 2 new discussed feature requests:
- recursive repository processing
- YAML file picking via SCM URL

Could you convert this to raise an exception if koji_tag isn't defined instead of a warning? If the koji_tag isn't set, this would cause a lot of problems.

I'd prefer if this was just a single message with a string. So if koji_tag wasn't set, the API would just return an error. If an existing build is being updated, you could just make the message "mariadb:10.2:20180724000000:00000000 was updated successfully". If this is a new build, then the message would be "mariadb:10.2:20180724000000:00000000 was imported successfully". Other details aren't really important to the user and the messages list feels more like a collection of log statements than a status. Not that MBS has a UI for submitting builds, but if you imagined it did, you would want a single string to display back to the user, not a list of strings.

Optional: This seems unnecessary since this is only called once in your code and it just wraps a single function.

This code didn't change other than formatting so it shouldn't be part of this commit.

It'd be best to do this first in your method before running any code.

"API" => "this API endpoint"

This seems like a useless variable that lessens readability. If you use db.session directly below, then I know what type of session the code is referring to, otherwise, I must go back to where the variable was defined.

This should raise an exception so the user clearly knows their repo is misconfigured.

This should not be a list since only a single modulemd and SCM URL is accepted. If we add batch imports in the future, it'd likely be a separate API endpoint.

@fivaldi I left a few comments. Please let me know once they've been addressed and I'll review again. Thanks for the PR!

Please, check the code again. There are significant/functional changes in the regex.

Once we have the batch imports (whether or not it's a separate API endpoint), the simplicity of a same output format (list) would be more ergonomic. What do you think?

rebased onto e23ac1ba8e9db6cc7f42da615274d1dcaad86a3e

8 months ago

@mprahl Please check my changes and comments.

I see the differences now. Could you please explain why you changed this regex?

I see your point but it's confusing if an API endpoint can only process a single module and it returns a list with always one entry. If we add batch support, the API endpoint would be something like import-modules which then would return a list.

Also, this is varying from the format of all other API endpoints. We use a generic items key for a list instead of something specific like imported..

rebased onto 3a0cc4a96a372eec745a340eb2cfe21f103550e0

8 months ago

So, if I use items?

rebased onto 76f28fe914b70fc48f707a8082f457d6f8a7d81d

8 months ago

rebased onto 8c6a91cc80f1e0e8fb8aee81795887f15464b3b8

8 months ago

rebased onto 0ab1bf2

8 months ago

This is an improvement but it still doesn't make sense to return a list when the API endpoint is singular and can only import one module.

@rbean could you please provide your opinion here?

+1, makes sense to me @mprahl.

What's going on with #1004? It says it is a duplicate of this one. Should this one be closed in favor of that one? Or vice versa?

rebased onto cdd5cab5f9882dc3f2e2d65c2b3ec0853a62cc7f

7 months ago

rebased onto c97a606

7 months ago

Fixed. Now it's just:

{
  "module": ...,
  "messages": ...
}

Because it actually fixes what it supports compared to the intention before, what it should support, I mean the modpath portion of the regexp. If supplied before, it didn't pass the validation.

This description needs updating after the API change to make it only return one module.

This should be removed because this error raises an exception now.

mandatory_checks doesn't seem like the best name because it insinuates it should never be turned off, but you have a valid reason to turn them off here.

rebased onto 4da4710b64f6f325afefcaed97cbd1a22eabbafa

7 months ago

rebased onto 8394cf983bae3fb76af117369f0bea426341b460

7 months ago

You're right, this is an improvement.

Could you log this as an error? The user of the MBS shouldn't be told about configurations not being set. They should only be told that the API is disabled.

Could you put this in the load_data method? It seems like this check should always run when the request is loaded in this handler. This would also reduce the complexity of knowing which methods you need to run to verify the passed-in data.

The name implies this method is checking the regex, but it's actually using regex to do the check. This should be called something like validate_scmurl.

A more descriptive name would be preferred here like validate_scmurl_prefix. This could also be condensed in the load_data method because you always want this check to run.

This if statement should be removed because get_mmd_from_scm calls _fetch_mmd, which calls get_module_yaml, which raises an UnprocessableEntity exception.

rebased onto b5df54080bb8a608ad0238ae9d199318e7d090dc

7 months ago

@fivaldi it looks good. I left some comments. Please let me know once they are addressed.

1 new commit added

  • Code refactoring w/o the use of static methods
7 months ago

2 new commits added

  • Code refactoring w/o the use of static methods
  • Import module API
7 months ago

rebased onto 598347e

7 months ago

Optional: This should be on the previous line.

Pull-Request has been merged by mprahl

7 months ago
Metadata