PR#1580: New 'module_stream' optional parameter - fm-orchestrator

fm-orchestrator

#1580 New 'module_stream' optional parameter

Merged 4 years ago by mprahl. Opened 4 years ago by merlinm.

Unknown source stream-name-param into master

New 'module_stream' optional parameter

Merlin Mathesius • 4 years ago

da51beb

README.rst

file modified

		`@@ -124,6 +124,8 @@`
		``scratch`` is ``True`` or if the MBS setting ``YAML_SUBMIT_ALLOWED`` is ``True``.
		- ``module_name`` - a string to use as the module name if a scratch build is requested and the
		YAML modulemd is submitted using the ``modulemd`` parameter.
		+ - ``module_stream`` - a string to use as the stream name if a scratch build is requested and the
		+ YAML modulemd is submitted using the ``modulemd`` parameter.
		- ``rebuild_strategy`` - a string of the desired rebuild strategy (affects what components get
		`rebuilt). For the available options, please look at the "Rebuild Strategies" section below.`
		- ``require_overrides`` - the requires to override the modulemd with. The overrides must be to

module_build_service/web/views.py

file modified

+16 -3

		`@@ -346,6 +346,7 @@`
		`"buildrequire_overrides",`
		`"modulemd",`
		`"module_name",`
		`+ "module_stream",`
		`"owner",`
		`"rebuild_strategy",`
		`"reuse_components_from",`
		`@@ -481,6 +482,17 @@`
		`log.error("Missing branch")`
		`raise ValidationError("Missing branch")`

		`+ if "module_name" in self.data:`
		`+ log.error("Module name override is only allowed when a YAML file is submitted")`
		`+ raise ValidationError(`
		`+ "Module name override is only allowed when a YAML file is submitted"`
		`+ )`
		`+ if "module_stream" in self.data:`
		`+ log.error("Stream name override is only allowed when a YAML file is submitted")`
		`+ raise ValidationError(`
		`+ "Stream name override is only allowed when a YAML file is submitted"`
		`+ )`
		`+`
		`if not skip_optional_params:`
		`self.validate_optional_params()`

		`@@ -507,12 +519,13 @@`
		`def post(self):`
		`if "modulemd" in self.data:`
		`handle = BytesIO(self.data["modulemd"].encode("utf-8"))`
		`- if self.data.get("module_name"):`
		`- handle.filename = self.data["module_name"]`
		`else:`
		`handle = request.files["yaml"]`
		`+ if self.data.get("module_name"):`
		`+ handle.filename = self.data["module_name"]`
		`+ stream_name = self.data.get("module_stream", None)`
		`return submit_module_build_from_yaml(`
		`- db.session, self.username, handle, self.data)`
		`+ db.session, self.username, handle, self.data, stream=stream_name)`


		`def _dict_from_request(request):`

tests/test_web/test_views.py

file modified

+100 -2

		`@@ -2078,6 +2078,60 @@`
		`@pytest.mark.parametrize("api_version", [1, 2])`
		`@patch("module_build_service.web.auth.get_user", return_value=user)`
		`@patch("module_build_service.common.scm.SCM")`
		`+ def test_submit_build_module_name_override_not_allowed(`
		`+ self, mocked_scm, mocked_get_user, api_version`
		`+ ):`
		`+ FakeSCM(`
		`+ mocked_scm, "testmodule", "testmodule.yaml", "620ec77321b2ea7b0d67d82992dda3e1d67055b4")`
		`+`
		`+ post_url = "/module-build-service/{0}/module-builds/".format(api_version)`
		`+ rv = self.client.post(`
		`+ post_url,`
		`+ data=json.dumps({`
		`+ "branch": "master",`
		`+ "scmurl": "https://src.stg.fedoraproject.org/modules/"`
		`+ "testmodule.git?#68931c90de214d9d13feefbd35246a81b6cb8d49",`
		`+ "module_name": "altname",`
		`+ }),`
		`+ )`
		`+ # module name is allowed only when a modulemd file is submitted`
		`+ assert rv.status_code == 400`
		`+ result = json.loads(rv.data)`
		`+ assert result["error"] == "Bad Request"`
		`+ assert result["message"] == (`
		`+ "Module name override is only allowed when a YAML file is submitted"`
		`+ )`
		`+`
		`+ @pytest.mark.parametrize("api_version", [1, 2])`
		`+ @patch("module_build_service.web.auth.get_user", return_value=user)`
		`+ @patch("module_build_service.common.scm.SCM")`
		`+ def test_submit_build_stream_name_override_not_allowed(`
		`+ self, mocked_scm, mocked_get_user, api_version`
		`+ ):`
		`+ FakeSCM(`
		`+ mocked_scm, "testmodule", "testmodule.yaml", "620ec77321b2ea7b0d67d82992dda3e1d67055b4")`
		`+`
		`+ post_url = "/module-build-service/{0}/module-builds/".format(api_version)`
		`+ rv = self.client.post(`
		`+ post_url,`
		`+ data=json.dumps({`
		`+ "branch": "master",`
		`+ "scmurl": "https://src.stg.fedoraproject.org/modules/"`
		`+ "testmodule.git?#68931c90de214d9d13feefbd35246a81b6cb8d49",`
		`+ "module_stream": "altstream",`
		`+ }),`
		`+ )`
		`+ # stream name override is allowed only when a modulemd file is submitted`
		`+ assert rv.status_code == 400`
		`+ result = json.loads(rv.data)`
		`+ assert result["error"] == "Bad Request"`
		`+ assert result["message"] == (`
		`+ "Stream name override is only allowed when a YAML file is submitted"`
		`+ )`
		`+`
		`+ @pytest.mark.parametrize("api_version", [1, 2])`
		`+ @patch("module_build_service.web.auth.get_user", return_value=user)`
		`+ @patch("module_build_service.common.scm.SCM")`
		`@patch(`
		`"module_build_service.common.config.Config.modules_allow_scratch",`
		`new_callable=PropertyMock,`
		`@@ -2169,6 +2223,45 @@`
		`assert rv.status_code == expected_error["status"]`

		`@pytest.mark.parametrize("api_version", [1, 2])`
		`+ @pytest.mark.parametrize("mod_stream", [None, "alternate"])`
		`+ @patch("module_build_service.web.auth.get_user", return_value=user)`
		`+ @patch(`
		`+ "module_build_service.common.config.Config.yaml_submit_allowed",`
		`+ new_callable=PropertyMock,`
		`+ return_value=True,`
		`+ )`
		`+ def test_submit_build_with_mmd(`
		`+ self, mocked_allow_yaml, mocked_get_user, mod_stream, api_version`
		`+ ):`
		`+ modulemd = read_staged_data("testmodule")`
		`+`
		`+ post_data = {`
		`+ "branch": "master",`
		`+ "modulemd": modulemd,`
		`+ "module_name": str(splitext(basename(staged_data_filename("testmodule")))[0]),`
		`+ }`
		`+ if mod_stream:`
		`+ post_data["module_stream"] = mod_stream`
		`+ expected_stream = mod_stream`
		`+ else:`
		`+ expected_stream = "master"`
		`+ post_url = "/module-build-service/{0}/module-builds/".format(api_version)`
		`+ rv = self.client.post(post_url, data=json.dumps(post_data))`
		`+ data = json.loads(rv.data)`
		`+`
		`+ if api_version >= 2:`
		`+ assert isinstance(data, list)`
		`+ assert len(data) == 1`
		`+ data = data[0]`
		`+`
		`+ assert data["name"] == "testmodule"`
		`+ assert data["scratch"] is False`
		`+ assert data["stream"] == expected_stream`
		`+ # Assertions for other "testmodule" attributes are done in`
		`+ # test_submit_scratch_build_with_mmd()`
		`+`
		`+ @pytest.mark.parametrize("api_version", [1, 2])`
		`+ @pytest.mark.parametrize("mod_stream", [None, "alternate"])`
		`@patch("module_build_service.web.auth.get_user", return_value=user)`
		`@patch(`
		`"module_build_service.common.config.Config.modules_allow_scratch",`
		`@@ -2181,7 +2274,7 @@`
		`return_value=True,`
		`)`
		`def test_submit_scratch_build_with_mmd(`
		`- self, mocked_allow_yaml, mocked_allow_scratch, mocked_get_user, api_version`
		`+ self, mocked_allow_yaml, mocked_allow_scratch, mocked_get_user, mod_stream, api_version`
		`):`
		`modulemd = read_staged_data("testmodule")`

		`@@ -2191,6 +2284,11 @@`
		`"modulemd": modulemd,`
		`"module_name": str(splitext(basename(staged_data_filename("testmodule")))[0]),`
		`}`
		`+ if mod_stream:`
		`+ post_data["module_stream"] = mod_stream`
		`+ expected_stream = mod_stream`
		`+ else:`
		`+ expected_stream = "master"`
		`post_url = "/module-build-service/{0}/module-builds/".format(api_version)`
		`rv = self.client.post(post_url, data=json.dumps(post_data))`
		`data = json.loads(rv.data)`
		`@@ -2212,7 +2310,7 @@`
		`assert data["time_submitted"] is not None`
		`assert data["time_modified"] is not None`
		`assert data["time_completed"] is None`
		`- assert data["stream"] == "master"`
		`+ assert data["stream"] == expected_stream`
		`assert data["owner"] == "Homer J. Simpson"`
		`assert data["id"] == 8`
		`assert data["rebuild_strategy"] == "changed-and-after"`

merlinm commented 4 years ago

Implement new optional parameter module_stream to allow a scratch module build's stream name to be set from the command line when also submitting a YAML modulemd file.
Validate that module_name and module_stream parameters can only be specified along with a YAML modulemd file.
Add tests to verify that module_stream sets the stream name correctly.
Add tests to verify that module_name and module_stream are only allowed along with a YAML modulemd file.

mprahl commented 4 years ago

@breilly and @mikem, could you please review this?

breilly commented 4 years ago

LGTM :thumbsup:

mprahl commented on line 16 of module_build_service/views.py 4 years ago

Could you please use double quotes to conform to the coding standards?

mprahl commented on line 21 of module_build_service/views.py 4 years ago

Could you please use double quotes to conform to the coding standards?

mprahl commented on line 97 of tests/test_views/test_views.py 4 years ago

Most of these assertions are already done elsewhere. Could you please slim this down and only verify things that are specific to this test (e.g. the the module's name and stream)?

rebased onto 1fc39dc810968417e422b153b947084c7cbe09b8

4 years ago

merlinm commented 4 years ago

@mprahl Thank you for the feedback! I rebased my PR to the latest master and addressed your comments.

Strangely, however, git or pagure picked up and include an additional commit with my change (1fc39dc810968417e422b153b947084c7cbe09b8, Pin the version of pip in the CentOS tests, mprahl) that's not actually in the master branch. I'll rebase again and see if I can get that to go away.

rebased onto 48eeff6f8dd91a9af6220fe95d9d09b2c01d753c

4 years ago