PR#973: Import module API - fm-orchestrator

fm-orchestrator

#973 Import module API

Merged 5 years ago by mprahl. Opened 5 years ago by fivaldi.

fivaldi/fm-orchestrator master into master

Filip Valder • 5 years ago

598347e

README.rst

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`
		`-------------`

conf/config.py

file modified

		`@@ -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`

module_build_service/config.py

file modified

+5 -1

		`@@ -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,`

module_build_service/utils/general.py

file modified

+43 -9

		`@@ -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`

module_build_service/utils/submit.py

file modified

+6 -2

		`@@ -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:`

module_build_service/utils/views.py

file modified

+7 -3

		`@@ -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 = (`
mprahl commented 5 years ago This code didn't change other than formatting so it shouldn't be part of this commit.
fivaldi commented 5 years ago Please, check the code again. There are significant/functional changes in the regex.
mprahl commented 5 years ago I see the differences now. Could you please explain why you changed this regex?
fivaldi commented 5 years ago 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.
mprahl commented 5 years ago 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):`

module_build_service/views.py

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.")`
mprahl commented 5 years ago 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.")`

tests/scm_data/mariadb/HEAD

file added

		`@@ -0,0 +1,1 @@`
		`+ ref: refs/heads/master`

tests/scm_data/mariadb/config

file added

		`@@ -0,0 +1,4 @@`
		`+ [core]`
		`+ repositoryformatversion = 0`
		`+ filemode = true`
		`+ bare = true`

tests/scm_data/mariadb/description

file added

		`@@ -0,0 +1,1 @@`
		`+ Unnamed repository; edit this file 'description' to name the repository.`

tests/scm_data/mariadb/info/exclude

file added

		`@@ -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]`
		`+ # *~`

tests/scm_data/mariadb/info/refs

file added

		`@@ -0,0 +1,1 @@`
		`+ 9ab5fdeba83eb3382413ee8bc06299344ef4477d refs/heads/master`

tests/scm_data/mariadb/objects/info/packs

file added

		`@@ -0,0 +1,2 @@`
		`+ P pack-92fbbdbf4fa07dc9cab035120eb248da930e0bd6.pack`
		`+`

tests/scm_data/mariadb/objects/pack/pack-92fbbdbf4fa07dc9cab035120eb248da930e0bd6.idx

file added

empty or binary file added

tests/scm_data/mariadb/objects/pack/pack-92fbbdbf4fa07dc9cab035120eb248da930e0bd6.pack

file added

empty or binary file added

tests/scm_data/mariadb/packed-refs

file added

		`@@ -0,0 +1,2 @@`
		`+ # pack-refs with: peeled fully-peeled`
		`+ 9ab5fdeba83eb3382413ee8bc06299344ef4477d refs/heads/master`

tests/scm_data/mariadb/refs/heads/.placeholder

file added

empty or binary file added

tests/scm_data/mariadb/refs/tags/.placeholder

file added

empty or binary file added

tests/test_scm.py

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'`

tests/test_views/test_views.py

file modified

+175

		`@@ -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

mprahl commented on line 13 of conf/config.py 5 years ago

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.

Edited 5 years ago by mprahl

1 new commit added

Add docs for import module API

5 years ago

1 new commit added

Empty set of groups allowed to import module(s)

5 years ago

3 new commits added

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

5 years ago

3 new commits added

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

5 years ago

3 new commits added

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

5 years ago

1 new commit added

Handle failed module imports gracefully

5 years 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

5 years 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

5 years 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

5 years ago

mprahl commented on line 6 of module_build_service/utils/views.py 5 years ago

Could you document this repopath_only parameter?

mprahl commented on line 29 of module_build_service/views.py 5 years ago

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

mprahl commented on line 74 of module_build_service/views.py 5 years ago

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

mprahl commented 5 years ago

I'm going to finish reviewing it tomorrow.

fivaldi commented on line 6 of module_build_service/utils/views.py 5 years ago

Added to a docstring, Thanks.

1 new commit added

Review fixes

5 years ago

fivaldi commented on line 29 of module_build_service/views.py 5 years 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

5 years ago

mprahl commented on line 29 of module_build_service/views.py 5 years 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.

mprahl commented on line 29 of module_build_service/views.py 5 years ago

Adding a docstring to this would also be nice.

mprahl commented on line 84 of module_build_service/views.py 5 years ago

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.

mprahl commented on line 68 of module_build_service/views.py 5 years ago

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

mprahl commented on line 104 of module_build_service/views.py 5 years ago

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.

mprahl commented on line 118 of module_build_service/views.py 5 years ago

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

mprahl commented 5 years ago

@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

5 years ago

ralph commented 5 years 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

5 years ago

fivaldi commented 5 years 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

mprahl commented on line 40 of module_build_service/utils/general.py 5 years ago

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.

mprahl commented on line 55 of README.rst 5 years ago

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.

mprahl commented on line 72 of module_build_service/utils/general.py 5 years ago

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

mprahl commented on line 12 of module_build_service/utils/views.py 5 years ago

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

mprahl commented on line 71 of module_build_service/views.py 5 years ago

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

mprahl commented on line 70 of module_build_service/views.py 5 years ago

"API" => "this API endpoint"

mprahl commented on line 68 of module_build_service/views.py 5 years ago

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.

mprahl commented on line 98 of module_build_service/views.py 5 years ago

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

mprahl commented on line 34 of README.rst 5 years ago

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.

mprahl commented 5 years ago

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

fivaldi commented on line 12 of module_build_service/utils/views.py 5 years ago

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

fivaldi commented on line 34 of README.rst 5 years ago

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

5 years ago

fivaldi commented 5 years ago

@mprahl Please check my changes and comments.

mprahl commented on line 12 of module_build_service/utils/views.py 5 years ago

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

mprahl commented on line 34 of README.rst 5 years ago

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

5 years ago

fivaldi commented on line 34 of README.rst 5 years ago

So, if I use items?

rebased onto 76f28fe914b70fc48f707a8082f457d6f8a7d81d

5 years ago

rebased onto 8c6a91cc80f1e0e8fb8aee81795887f15464b3b8

5 years ago

rebased onto 0ab1bf2

5 years ago

mprahl commented on line 34 of README.rst 5 years 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?

ralph commented on line 34 of README.rst 5 years ago

+1, makes sense to me @mprahl.

ralph commented 5 years ago

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?

Edited 5 years ago by ralph

fivaldi commented 5 years ago

PR#1004 closed without merging.

rebased onto cdd5cab5f9882dc3f2e2d65c2b3ec0853a62cc7f

5 years ago

rebased onto c97a606

5 years ago

fivaldi commented on line 34 of README.rst 5 years ago

Fixed. Now it's just:

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

fivaldi commented on line 12 of module_build_service/utils/views.py 5 years ago

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.

mprahl commented on line 23 of README.rst 5 years ago

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

mprahl commented on line 54 of README.rst 5 years ago

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

mprahl commented on line 15 of module_build_service/utils/submit.py 5 years ago

You need to rebase because this changed in:
https://pagure.io/fm-orchestrator/pull-request/1008

mprahl commented on line 7 of module_build_service/utils/submit.py 5 years ago

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

5 years ago

rebased onto 8394cf983bae3fb76af117369f0bea426341b460

5 years ago

mprahl commented on line 12 of module_build_service/utils/views.py 5 years ago

You're right, this is an improvement.

mprahl commented on line 73 of module_build_service/views.py 5 years ago

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.

mprahl commented on line 124 of module_build_service/views.py 5 years ago

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.

mprahl commented on line 138 of module_build_service/views.py 5 years ago

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.

mprahl commented on line 131 of module_build_service/views.py 5 years ago

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.

mprahl commented on line 92 of module_build_service/views.py 5 years ago

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