PR#1384: Add "scratch_build_only_branches" configuration options. - fm-orchestrator

fm-orchestrator

#1384 Add "scratch_build_only_branches" configuration options.

Merged 4 years ago by jkaluza. Opened 4 years ago by jkaluza.

jkaluza/fm-orchestrator private into master

Do not check Greenwave gating status for scratch builds.

Jan Kaluza • 4 years ago

80fca55

Add "scratch_build_only_branches" configuration options.

Jan Kaluza • 4 years ago

796a367

module_build_service/config.py

file modified

		`@@ -640,6 +640,12 @@`
		`"default": False,`
		`"desc": "Allow module scratch builds",`
		`},`
		`+ "scratch_build_only_branches": {`
		`+ "type": list,`
		`+ "default": [],`
		`+ "desc": "The list of regexes used to identify branches from which only the module "`
		`+ "scratch builds can be built",`
		`+ },`
		`"product_pages_url": {`
		`"type": str,`
		`"default": "",`

module_build_service/scheduler/producer.py

file modified

+1 -1

		`@@ -482,7 +482,7 @@`

		`module_builds = (`
		`db_session.query(models.ModuleBuild)`
		`- .filter_by(state=models.BUILD_STATES["done"]).all()`
		`+ .filter_by(state=models.BUILD_STATES["done"], scratch=False).all()`
		`)`

		`log.info("Checking Greenwave for %d builds", len(module_builds))`

module_build_service/utils/submit.py

file modified

+12

		`@@ -1003,6 +1003,18 @@`
		`# append incrementing counter to context`
		`context_suffix = "_" + str(len(scrmods) + 1)`
		`mmd.set_context(mmd.get_context() + context_suffix)`
		`+ else:`
		`+ # In case the branch is defined, check whether user is allowed to submit`
		`+ # non-scratch build from this branch. Note that the branch is always defined`
		`+ # for official builds from SCM, because it is requested in views.py.`
		`+ branch = params.get("branch")`
		`+ if branch:`
		`+ for regex in conf.scratch_build_only_branches:`
		`+ branch_search = re.search(regex, branch)`
		`+ if branch_search:`
		`+ raise ValidationError(`
		`+ "Only scratch module builds can be build from this branch."`
		`+ )`

		`log.debug("Creating new module build")`
		`module = models.ModuleBuild.create(`

tests/test_scheduler/test_poller.py

file modified

+14 -6

		`@@ -22,7 +22,7 @@`
		`import pytest`
		`from mock import patch`
		`from module_build_service import models, conf`
		`- from tests import clean_database`
		`+ from tests import clean_database, make_module_in_db`
		`import mock`
		`import koji`
		`from module_build_service.scheduler.producer import MBSProducer`
		`@@ -710,8 +710,12 @@`
		`module_build2 = models.ModuleBuild.get_by_id(db_session, 2)`
		`module_build2.state = models.BUILD_STATES["done"]`

		`- module_build2 = models.ModuleBuild.get_by_id(db_session, 3)`
		`- module_build2.state = models.BUILD_STATES["init"]`
		`+ module_build3 = models.ModuleBuild.get_by_id(db_session, 3)`
		`+ module_build3.state = models.BUILD_STATES["init"]`
		`+`
		`+ module_build4 = make_module_in_db("foo:1:1:1", {}, db_session=db_session)`
		`+ module_build4.state = models.BUILD_STATES["done"]`
		`+ module_build4.scratch = True`

		`db_session.commit()`

		`@@ -737,6 +741,10 @@`
		`assert len(modules) == 1`
		`assert modules[0].id == 1`
		`modules = models.ModuleBuild.by_state(db_session, "done")`
		`- assert len(modules) == 1`
		`- assert modules[0].id == 2`
		`- assert re.match("Gating failed.*", modules[0].state_reason)`
		`+ assert len(modules) == 2`
		`+ for module in modules:`
		`+ assert module.id in [2, 4]`
		`+ if module.id == 2:`
		`+ assert re.match("Gating failed.*", module.state_reason)`
		`+ else:`
		`+ assert module.state_reason is None`

tests/test_utils/test_utils.py

file modified

+26

		`@@ -38,6 +38,7 @@`
		`init_data,`
		`scheduler_init_data,`
		`make_module_in_db,`
		`+ make_module,`
		`read_staged_data, staged_data_filename)`
		`import mock`
		`import koji`
		`@@ -1051,6 +1052,31 @@`
		`assert builds[0].siblings(db_session) == [builds[1].id]`
		`assert builds[1].siblings(db_session) == [builds[0].id]`

		`+ @patch("module_build_service.utils.mse.generate_expanded_mmds")`
		`+ @patch(`
		`+ "module_build_service.config.Config.scratch_build_only_branches",`
		`+ new_callable=mock.PropertyMock,`
		`+ return_value=["^private-.*"],`
		`+ )`
		`+ def test_submit_build_scratch_build_only_branches(`
		`+ self, cfg, generate_expanded_mmds, db_session):`
		`+ """`
		`+ Tests the "scratch_build_only_branches" config option.`
		`+ """`
		`+ mmd = make_module("foo:stream:0:c1")`
		`+ generate_expanded_mmds.return_value = [mmd]`
		`+ # Create a copy of mmd1 without xmd.mbs, since that will cause validate_mmd to fail`
		`+ mmd_copy = mmd.copy()`
		`+ mmd_copy.set_xmd({})`
		`+`
		`+ with pytest.raises(ValidationError,`
		`+ match="Only scratch module builds can be build from this branch."):`
		`+ module_build_service.utils.submit_module_build(`
		`+ db_session, "foo", mmd_copy, {"branch": "private-foo"})`
		`+`
		`+ module_build_service.utils.submit_module_build(`
		`+ db_session, "foo", mmd_copy, {"branch": "otherbranch"})`
		`+`

		`class DummyModuleBuilder(GenericBuilder):`
		`"""`

jkaluza commented 4 years ago

The goal here is to define certain branches from which only scratch
module builds can be submitted. The main use case is for "private-*"
branches which can be created and maintained by anyone, but there
must not be production-ready module build created from them.

This commit adds new scratch_build_only_branches config option
to define the list of regexes to match such branches.

gnaponie commented on line 9 of module_build_service/utils/submit.py 4 years ago

Optional: you can avoid None. If the key is not in there, get will automatically return None.

gnaponie commented on line 11 of module_build_service/utils/submit.py 4 years ago

Are you sure that's correct?
If I understood correctly "conf.scratch_build_only_branches" is a dictionary. If you are going to iterate over that, regex will be "type, default, desc" (the keys of the dict) and I think what you want is the list of regex... Am I missing something here?

Edited 4 years ago by gnaponie

ignatenkobrain commented 4 years ago

Is this RHEL-only thing or you are aiming to set this in Fedora too? If former, I don't care.

lucarval commented 4 years ago

Are you sure that's correct?
If I understood correctly "conf.scratch_build_only_branches" is a dictionary. If you are going to iterate over that, regex will be "type, default, desc" (the keys of the dict) and I think what you want is the list of regex... Am I missing something here?

scratch_build_only_branches is a list. The definition in config.py is the schema which is itself a dict.

lucarval commented 4 years ago

:+1:

gnaponie commented 4 years ago