From 92dce63091feae55b77bd4cf89b8ab85def62bf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Tue, 21 Feb 2017 14:30:47 +0100 Subject: [PATCH 01/12] Allow submitting optional parameters such as copr_owner and copr_project --- ...697622859_add_optional_columns_for_copr.py | 24 +++++++++++++++++++ module_build_service/models.py | 9 +++++-- module_build_service/utils.py | 13 +++++----- module_build_service/views.py | 14 +++++++++-- 4 files changed, 50 insertions(+), 10 deletions(-) create mode 100644 module_build_service/migrations/versions/474697622859_add_optional_columns_for_copr.py diff --git a/module_build_service/migrations/versions/474697622859_add_optional_columns_for_copr.py b/module_build_service/migrations/versions/474697622859_add_optional_columns_for_copr.py new file mode 100644 index 00000000..19a0e4c9 --- /dev/null +++ b/module_build_service/migrations/versions/474697622859_add_optional_columns_for_copr.py @@ -0,0 +1,24 @@ +"""Add optional columns for copr + +Revision ID: 474697622859 +Revises: 0ef60c3ed440 +Create Date: 2017-02-21 11:18:22.304038 + +""" + +# revision identifiers, used by Alembic. +revision = '474697622859' +down_revision = '0ef60c3ed440' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('module_builds', sa.Column('copr_owner', sa.String(), nullable=True)) + op.add_column('module_builds', sa.Column('copr_project', sa.String(), nullable=True)) + + +def downgrade(): + op.drop_column('module_builds', 'copr_owner') + op.drop_column('module_builds', 'copr_project') diff --git a/module_build_service/models.py b/module_build_service/models.py index d567a45c..cffc9a8d 100644 --- a/module_build_service/models.py +++ b/module_build_service/models.py @@ -101,6 +101,8 @@ class ModuleBuild(RidaBase): state_reason = db.Column(db.String) modulemd = db.Column(db.String, nullable=False) koji_tag = db.Column(db.String) # This gets set after 'wait' + copr_owner = db.Column(db.String) + copr_project = db.Column(db.String) scmurl = db.Column(db.String) owner = db.Column(db.String, nullable=False) time_submitted = db.Column(db.DateTime, nullable=False) @@ -155,7 +157,8 @@ class ModuleBuild(RidaBase): % type(event).__name__) @classmethod - def create(cls, session, conf, name, stream, version, modulemd, scmurl, username): + def create(cls, session, conf, name, stream, version, modulemd, scmurl, username, + copr_owner=None, copr_project=None): now = datetime.utcnow() module = cls( name=name, @@ -165,7 +168,9 @@ class ModuleBuild(RidaBase): modulemd=modulemd, scmurl=scmurl, owner=username, - time_submitted=now + time_submitted=now, + copr_owner=copr_owner, + copr_project=copr_project, ) session.add(module) session.commit() diff --git a/module_build_service/utils.py b/module_build_service/utils.py index 5fc1ffc2..3bf8330a 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -526,17 +526,17 @@ def record_component_builds(scm, mmd, module, initial_batch = 1): return batch -def submit_module_build_from_yaml(username, yaml): +def submit_module_build_from_yaml(username, yaml, optional_params={}): mmd = load_mmd(yaml) - return submit_module_build(username, None, mmd, None, yaml) + return submit_module_build(username, None, mmd, None, yaml, optional_params) -def submit_module_build_from_scm(username, url, allow_local_url=False): +def submit_module_build_from_scm(username, url, allow_local_url=False, optional_params={}): mmd, scm, yaml = _fetch_mmd(url, allow_local_url) - return submit_module_build(username, url, mmd, scm, yaml) + return submit_module_build(username, url, mmd, scm, yaml, optional_params) -def submit_module_build(username, url, mmd, scm, yaml): +def submit_module_build(username, url, mmd, scm, yaml, optional_params={}): # Import it here, because SCM uses utils methods # and fails to import them because of dep-chain. import module_build_service.scm @@ -569,7 +569,8 @@ def submit_module_build(username, url, mmd, scm, yaml): version=str(mmd.version), modulemd=yaml, scmurl=url, - username=username + username=username, + **optional_params ) record_component_builds(scm, mmd, module) diff --git a/module_build_service/views.py b/module_build_service/views.py index 65fc3187..3e38f53d 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -124,18 +124,28 @@ class ModuleBuildAPI(MethodView): log.error("The submitted scmurl %r is not valid" % url) raise Unauthorized("The submitted scmurl %s is not valid" % url) - return submit_module_build_from_scm(username, url, allow_local_url=False) + forbidden_params = [k for k in r if not hasattr(models.ModuleBuild, k)] + if forbidden_params: + raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) + + optional_params = {k: v for k, v in r.items() if k != "scmurl"} + return submit_module_build_from_scm(username, url, allow_local_url=False, optional_params=optional_params) def post_file(self, username): if not conf.yaml_submit_allowed: raise Unauthorized("YAML submission is not enabled") + + forbidden_params = [k for k in request.form if not hasattr(models.ModuleBuild, k)] + if forbidden_params: + raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) + try: r = request.files["yaml"] except: log.error('Invalid file submitted') raise ValidationError('Invalid file submitted') - return submit_module_build_from_yaml(username, r.read()) + return submit_module_build_from_yaml(username, r.read(), optional_params=dict(request.form.items())) def patch(self, id): username, groups = module_build_service.auth.get_user(request) From 6bd9e7026fb514e25d08e8c0174a1042f77c3b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Tue, 21 Feb 2017 15:30:44 +0100 Subject: [PATCH 02/12] Use optional module build params in CoprModuleBuilder --- module_build_service/builder.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/module_build_service/builder.py b/module_build_service/builder.py index fce78234..79beedbb 100644 --- a/module_build_service/builder.py +++ b/module_build_service/builder.py @@ -927,7 +927,15 @@ class CoprModuleBuilder(GenericBuilder): def _get_copr_safe(self): from copr.exceptions import CoprRequestException - kwargs = {"ownername": self.owner, "projectname": CoprModuleBuilder._tag_to_copr_name(self.tag_name)} + + # @TODO it would be nice if the module build object was passed to Builder __init__ + module = ModuleBuild.query.filter(ModuleBuild.name == self.module_str).first() + + kwargs = { + "ownername": module.copr_owner or self.owner, + "projectname": module.copr_project or CoprModuleBuilder._tag_to_copr_name(self.tag_name) + } + try: return self._get_copr(**kwargs) except CoprRequestException: From 38bdc9224870b828be41606af7922cc0d07115b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Thu, 23 Feb 2017 19:21:16 +0100 Subject: [PATCH 03/12] Have immutable default value for optional_params --- module_build_service/utils.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/module_build_service/utils.py b/module_build_service/utils.py index 3bf8330a..2f3e2f26 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -526,17 +526,17 @@ def record_component_builds(scm, mmd, module, initial_batch = 1): return batch -def submit_module_build_from_yaml(username, yaml, optional_params={}): +def submit_module_build_from_yaml(username, yaml, optional_params=None): mmd = load_mmd(yaml) return submit_module_build(username, None, mmd, None, yaml, optional_params) -def submit_module_build_from_scm(username, url, allow_local_url=False, optional_params={}): +def submit_module_build_from_scm(username, url, allow_local_url=False, optional_params=None): mmd, scm, yaml = _fetch_mmd(url, allow_local_url) return submit_module_build(username, url, mmd, scm, yaml, optional_params) -def submit_module_build(username, url, mmd, scm, yaml, optional_params={}): +def submit_module_build(username, url, mmd, scm, yaml, optional_params=None): # Import it here, because SCM uses utils methods # and fails to import them because of dep-chain. import module_build_service.scm @@ -570,7 +570,7 @@ def submit_module_build(username, url, mmd, scm, yaml, optional_params={}): modulemd=yaml, scmurl=url, username=username, - **optional_params + **(optional_params or {}) ) record_component_builds(scm, mmd, module) From 5a65f50b524a64487fa84b56a19401d123bae01c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Thu, 23 Feb 2017 20:00:38 +0100 Subject: [PATCH 04/12] Use built-in to_dict() function --- module_build_service/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module_build_service/views.py b/module_build_service/views.py index 3e38f53d..c010295e 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -145,7 +145,7 @@ class ModuleBuildAPI(MethodView): log.error('Invalid file submitted') raise ValidationError('Invalid file submitted') - return submit_module_build_from_yaml(username, r.read(), optional_params=dict(request.form.items())) + return submit_module_build_from_yaml(username, r.read(), optional_params=request.form.to_dict()) def patch(self, id): username, groups = module_build_service.auth.get_user(request) From 62e26ba652045eddee582b3481baf42b98c64e3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Thu, 23 Feb 2017 20:17:33 +0100 Subject: [PATCH 05/12] Use better way of checking for forbidden columns --- module_build_service/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module_build_service/views.py b/module_build_service/views.py index c010295e..f5145746 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -124,7 +124,7 @@ class ModuleBuildAPI(MethodView): log.error("The submitted scmurl %r is not valid" % url) raise Unauthorized("The submitted scmurl %s is not valid" % url) - forbidden_params = [k for k in r if not hasattr(models.ModuleBuild, k)] + forbidden_params = [k for k in r if k not in models.ModuleBuild.__table__.columns] if forbidden_params: raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) @@ -135,7 +135,7 @@ class ModuleBuildAPI(MethodView): if not conf.yaml_submit_allowed: raise Unauthorized("YAML submission is not enabled") - forbidden_params = [k for k in request.form if not hasattr(models.ModuleBuild, k)] + forbidden_params = [k for k in request.form if k not in models.ModuleBuild.__table__.columns] if forbidden_params: raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) From 7799515994f410e7ed210e9eb6eb2e19de3bde8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Thu, 23 Feb 2017 20:48:45 +0100 Subject: [PATCH 06/12] Use function for validating optional params to reduce code duplicity --- module_build_service/views.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/module_build_service/views.py b/module_build_service/views.py index f5145746..9cf93afa 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -97,14 +97,18 @@ class ModuleBuildAPI(MethodView): raise Unauthorized("%s is not in any of %r, only %r" % ( username, conf.allowed_groups, groups)) - if "multipart/form-data" in request.headers.get("Content-Type"): - module = self.post_file(username) - else: - module = self.post_scm(username) + def validate_optional_params(params): + forbidden_params = [k for k in params if k not in models.ModuleBuild.__table__.columns] + if forbidden_params: + raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) + + kwargs = {"username": username, "validate_optional_params": validate_optional_params} + module = (self.post_file(**kwargs) if "multipart/form-data" in request.headers.get("Content-Type") else + self.post_scm(**kwargs)) return jsonify(module.json()), 201 - def post_scm(self, username): + def post_scm(self, username, validate_optional_params): try: r = json.loads(request.get_data().decode("utf-8")) except: @@ -124,20 +128,14 @@ class ModuleBuildAPI(MethodView): log.error("The submitted scmurl %r is not valid" % url) raise Unauthorized("The submitted scmurl %s is not valid" % url) - forbidden_params = [k for k in r if k not in models.ModuleBuild.__table__.columns] - if forbidden_params: - raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) - + validate_optional_params(r) optional_params = {k: v for k, v in r.items() if k != "scmurl"} return submit_module_build_from_scm(username, url, allow_local_url=False, optional_params=optional_params) - def post_file(self, username): + def post_file(self, username, validate_optional_params): if not conf.yaml_submit_allowed: raise Unauthorized("YAML submission is not enabled") - - forbidden_params = [k for k in request.form if k not in models.ModuleBuild.__table__.columns] - if forbidden_params: - raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) + validate_optional_params(request.form) try: r = request.files["yaml"] From 5c52fce586e16e70263c139bfabd367d93967256 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Thu, 23 Feb 2017 20:55:51 +0100 Subject: [PATCH 07/12] Update to the accurate down_revision --- .../versions/474697622859_add_optional_columns_for_copr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module_build_service/migrations/versions/474697622859_add_optional_columns_for_copr.py b/module_build_service/migrations/versions/474697622859_add_optional_columns_for_copr.py index 19a0e4c9..c3d6b5b4 100644 --- a/module_build_service/migrations/versions/474697622859_add_optional_columns_for_copr.py +++ b/module_build_service/migrations/versions/474697622859_add_optional_columns_for_copr.py @@ -8,7 +8,7 @@ Create Date: 2017-02-21 11:18:22.304038 # revision identifiers, used by Alembic. revision = '474697622859' -down_revision = '0ef60c3ed440' +down_revision = 'a1fc0736bca8' from alembic import op import sqlalchemy as sa From 48f27cd7ea9862f70425c0850d9ed689ea03f201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Sun, 26 Feb 2017 22:19:31 +0100 Subject: [PATCH 08/12] Get one() result instead of first() --- module_build_service/builder.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module_build_service/builder.py b/module_build_service/builder.py index 79beedbb..765992f4 100644 --- a/module_build_service/builder.py +++ b/module_build_service/builder.py @@ -929,7 +929,7 @@ class CoprModuleBuilder(GenericBuilder): from copr.exceptions import CoprRequestException # @TODO it would be nice if the module build object was passed to Builder __init__ - module = ModuleBuild.query.filter(ModuleBuild.name == self.module_str).first() + module = ModuleBuild.query.filter(ModuleBuild.name == self.module_str).one() kwargs = { "ownername": module.copr_owner or self.owner, @@ -1056,7 +1056,7 @@ class CoprModuleBuilder(GenericBuilder): def finalize(self): modulemd = tempfile.mktemp() - m1 = ModuleBuild.query.filter(ModuleBuild.name == self.module_str).first() + m1 = ModuleBuild.query.filter(ModuleBuild.name == self.module_str).one() m1.mmd().dump(modulemd) # Wait until all builds are finished From 34ea4a8efec54b0060a5c9c3272f49a94ae9c32b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Sun, 26 Feb 2017 22:23:50 +0100 Subject: [PATCH 09/12] Move validate_optional_params function to utils.py --- module_build_service/utils.py | 6 ++++++ module_build_service/views.py | 13 ++++--------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/module_build_service/utils.py b/module_build_service/utils.py index 2f3e2f26..ef9351c2 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -583,6 +583,12 @@ def submit_module_build(username, url, mmd, scm, yaml, optional_params=None): mmd.name, mmd.stream, mmd.version) return module + +def validate_optional_params(params): + forbidden_params = [k for k in params if k not in models.ModuleBuild.__table__.columns] + if forbidden_params: + raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) + def scm_url_schemes(terse=False): """ Definition of URL schemes supported by both frontend and scheduler. diff --git a/module_build_service/views.py b/module_build_service/views.py index 9cf93afa..097f4d9e 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -35,7 +35,7 @@ from module_build_service import app, conf, log from module_build_service import models, db from module_build_service.utils import ( pagination_metadata, filter_module_builds, submit_module_build_from_scm, - submit_module_build_from_yaml, scm_url_schemes, get_scm_url_re) + submit_module_build_from_yaml, scm_url_schemes, get_scm_url_re, validate_optional_params) from module_build_service.errors import ( ValidationError, Unauthorized, NotFound) @@ -97,18 +97,13 @@ class ModuleBuildAPI(MethodView): raise Unauthorized("%s is not in any of %r, only %r" % ( username, conf.allowed_groups, groups)) - def validate_optional_params(params): - forbidden_params = [k for k in params if k not in models.ModuleBuild.__table__.columns] - if forbidden_params: - raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) - - kwargs = {"username": username, "validate_optional_params": validate_optional_params} + kwargs = {"username": username} module = (self.post_file(**kwargs) if "multipart/form-data" in request.headers.get("Content-Type") else self.post_scm(**kwargs)) return jsonify(module.json()), 201 - def post_scm(self, username, validate_optional_params): + def post_scm(self, username): try: r = json.loads(request.get_data().decode("utf-8")) except: @@ -132,7 +127,7 @@ class ModuleBuildAPI(MethodView): optional_params = {k: v for k, v in r.items() if k != "scmurl"} return submit_module_build_from_scm(username, url, allow_local_url=False, optional_params=optional_params) - def post_file(self, username, validate_optional_params): + def post_file(self, username): if not conf.yaml_submit_allowed: raise Unauthorized("YAML submission is not enabled") validate_optional_params(request.form) From cef690f260fac0d5c62a863a7bdc51b3d0c3d079 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Mon, 27 Feb 2017 15:53:39 +0100 Subject: [PATCH 10/12] Not accept copr parameters when different builder is used --- module_build_service/utils.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/module_build_service/utils.py b/module_build_service/utils.py index ef9351c2..a90baa83 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -589,6 +589,12 @@ def validate_optional_params(params): if forbidden_params: raise ValidationError('The request contains unspecified parameters: {}'.format(", ".join(forbidden_params))) + forbidden_params = [k for k in params if k.startswith("copr_")] + if conf.system != "copr" and forbidden_params: + raise ValidationError('The request contains parameters specific to Copr builder: {} even though {} is used' + .format(", ".join(forbidden_params), conf.system)) + + def scm_url_schemes(terse=False): """ Definition of URL schemes supported by both frontend and scheduler. From 9429114836b89fdb09587a24f8ca835eef1f3ec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Mon, 27 Feb 2017 16:20:01 +0100 Subject: [PATCH 11/12] Get empty string if Content-Type is not in headers --- module_build_service/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module_build_service/views.py b/module_build_service/views.py index 097f4d9e..cf414deb 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -98,7 +98,7 @@ class ModuleBuildAPI(MethodView): username, conf.allowed_groups, groups)) kwargs = {"username": username} - module = (self.post_file(**kwargs) if "multipart/form-data" in request.headers.get("Content-Type") else + module = (self.post_file(**kwargs) if "multipart/form-data" in request.headers.get("Content-Type", "") else self.post_scm(**kwargs)) return jsonify(module.json()), 201 From 70103c41e2f8f31c3e153e7b0db4d16166fb38f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kadl=C4=8D=C3=ADk?= Date: Mon, 27 Feb 2017 17:23:05 +0100 Subject: [PATCH 12/12] Test submitting optional parameters --- tests/test_build/test_build.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index 1ddb9078..4ab772fc 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -312,6 +312,24 @@ class TestBuild(unittest.TestCase): self.assertEqual(data['status'], 401) self.assertEqual(data['message'], 'YAML submission is not enabled') + @timed(30) + @patch('module_build_service.auth.get_user', return_value=user) + def test_submit_build_with_optional_params(self, mocked_get_user): + params = {'scmurl': 'git://pkgs.stg.fedoraproject.org/modules/' + 'testmodule.git?#68932c90de214d9d13feefbd35246a81b6cb8d49'} + + def submit(data): + rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(data)) + return json.loads(rv.data) + + data = submit(dict(params.items() + {"not_existing_param": "foo"}.items())) + self.assertIn("The request contains unspecified parameters:", data["message"]) + self.assertIn("not_existing_param", data["message"]) + self.assertEqual(data["status"], 400) + + data = submit(dict(params.items() + {"copr_owner": "foo"}.items())) + self.assertIn("The request contains parameters specific to Copr builder", data["message"]) + @timed(30) @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM')