From 663b7fc4d06dfe2aa970d470193caf4a4e1f09a1 Mon Sep 17 00:00:00 2001 From: Merlin Mathesius Date: Apr 03 2019 18:15:59 +0000 Subject: [PATCH 1/3] Allow resubmitting the same NSV for scratch module builds. Signed-off-by: Merlin Mathesius --- diff --git a/module_build_service/models.py b/module_build_service/models.py index a4bc05f..7ea03c5 100644 --- a/module_build_service/models.py +++ b/module_build_service/models.py @@ -354,6 +354,16 @@ class ModuleBuild(MBSBase): name=name, stream=stream, version=str(version), context=context, **kwargs).first() @staticmethod + def get_scratch_builds_from_nsvc(session, name, stream, version, context, **kwargs): + """ + Returns all scratch builds defined by NSVC. This is done by using the supplied `context` + as a match prefix. Optional kwargs are passed to SQLAlchemy filter_by method. + """ + return session.query(ModuleBuild).filter_by( + name=name, stream=stream, version=str(version), scratch=True, **kwargs)\ + .filter(ModuleBuild.context.like(context + '%')) + + @staticmethod def get_last_builds_in_stream_version_lte(session, name, stream_version): """ Returns the latest builds in "ready" state for given name:stream limited by @@ -831,10 +841,10 @@ class ModuleBuild(MBSBase): return rv def __repr__(self): - return (("") - % (self.name, self.id, self.stream, self.version, INVERSE_BUILD_STATES[self.state], - self.batch, self.state_reason)) + return (("") + % (self.name, self.id, self.stream, self.version, self.scratch, + INVERSE_BUILD_STATES[self.state], self.batch, self.state_reason)) class ModuleBuildTrace(MBSBase): diff --git a/module_build_service/utils/submit.py b/module_build_service/utils/submit.py index 37b970c..4a75933 100644 --- a/module_build_service/utils/submit.py +++ b/module_build_service/utils/submit.py @@ -602,6 +602,10 @@ def submit_module_build(username, mmd, params): """ import koji # Placed here to avoid py2/py3 conflicts... + log.debug('Submitted %s module build for %s:%s:%s', + ("scratch" if params.get('scratch', False) else "normal"), + mmd.get_name(), mmd.get_stream(), mmd.get_version()) + if mmd.get_name() in conf.base_module_names: raise ValidationError( 'You cannot build a module named "{}" since it is a base module'.format(mmd.get_name())) @@ -639,7 +643,7 @@ def submit_module_build(username, mmd, params): log.debug('Checking whether module build already exists: %s.', nsvc) module = models.ModuleBuild.get_build_from_nsvc( db.session, mmd.get_name(), mmd.get_stream(), version_str, mmd.get_context()) - if module: + if module and not params.get('scratch', False): if module.state != models.BUILD_STATES['failed']: log.info("Skipping rebuild of %s, only rebuild of modules in failed state " "is allowed.", nsvc) @@ -669,6 +673,18 @@ def submit_module_build(username, mmd, params): module.transition(conf, transition_to, "Resubmitted by %s" % username) log.info("Resumed existing module build in previous state %s" % module.state) else: + # make NSVC unique for every scratch build + context_suffix = '' + if params.get('scratch', False): + log.debug('Checking for existing scratch module builds by NSVC') + scrmods = models.ModuleBuild.get_scratch_builds_from_nsvc( + db.session, mmd.get_name(), mmd.get_stream(), version_str, mmd.get_context()) + scrmod_contexts = [scrmod.context for scrmod in scrmods] + log.debug('Found %d previous scratch module build context(s): %s', + scrmods.count(), ",".join(scrmod_contexts)) + # append incrementing counter to context + context_suffix = '.' + str(scrmods.count() + 1) + mmd.set_context(mmd.get_context() + context_suffix) log.debug('Creating new module build') module = models.ModuleBuild.create( db.session, @@ -685,6 +701,7 @@ def submit_module_build(username, mmd, params): ) (module.ref_build_context, module.build_context, module.runtime_context, module.context) = module.contexts_from_mmd(module.modulemd) + module.context += context_suffix all_modules_skipped = False db.session.add(module) diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index e2f46db..ce497e3 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -1200,6 +1200,105 @@ class TestBuild: @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') + @patch('module_build_service.config.Config.modules_allow_scratch', + new_callable=PropertyMock, return_value=True) + def test_submit_scratch_vs_normal( + self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc): + """ + Tests that submitting a scratch build with the same NSV as a completed + normal build succeeds + """ + FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml', + '620ec77321b2ea7b0d67d82992dda3e1d67055b4') + # Post so a module is in the init phase + post_url = '/module-build-service/1/module-builds/' + post_data = { + 'branch': 'master', + 'scmurl': 'https://src.stg.fedoraproject.org/modules/' + 'testmodule.git?#620ec77321b2ea7b0d67d82992dda3e1d67055b4', + 'scratch': False, + } + rv = self.client.post(post_url, data=json.dumps(post_data)) + assert rv.status_code == 201 + data = json.loads(rv.data) + module_build_id = data['id'] + module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() + # make sure normal build has expected context + assert module_build.context == '9c690d0e' + # Run the backend + stop = module_build_service.scheduler.make_simple_stop_condition(db.session) + module_build_service.scheduler.main([], stop) + # Post again as a scratch build and make sure it succeeds + post_data['scratch'] = True + rv2 = self.client.post(post_url, data=json.dumps(post_data)) + assert rv2.status_code == 201 + data = json.loads(rv2.data) + module_build_id = data['id'] + module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() + # make sure scratch build has context with unique suffix + assert module_build.context == '9c690d0e.1' + + @patch('module_build_service.auth.get_user', return_value=user) + @patch('module_build_service.scm.SCM') + @patch('module_build_service.config.Config.modules_allow_scratch', + new_callable=PropertyMock, return_value=True) + def test_submit_normal_vs_scratch( + self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc): + """ + Tests that submitting a normal build with the same NSV as a completed + scratch build succeeds + """ + FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml', + '620ec77321b2ea7b0d67d82992dda3e1d67055b4') + # Post so a scratch module build is in the init phase + post_url = '/module-build-service/1/module-builds/' + post_data = { + 'branch': 'master', + 'scmurl': 'https://src.stg.fedoraproject.org/modules/' + 'testmodule.git?#620ec77321b2ea7b0d67d82992dda3e1d67055b4', + 'scratch': True, + } + rv = self.client.post(post_url, data=json.dumps(post_data)) + assert rv.status_code == 201 + # Run the backend + stop = module_build_service.scheduler.make_simple_stop_condition(db.session) + module_build_service.scheduler.main([], stop) + # Post again as a non-scratch build and make sure it succeeds + post_data['scratch'] = False + rv2 = self.client.post(post_url, data=json.dumps(post_data)) + assert rv2.status_code == 201 + + @patch('module_build_service.auth.get_user', return_value=user) + @patch('module_build_service.scm.SCM') + @patch('module_build_service.config.Config.modules_allow_scratch', + new_callable=PropertyMock, return_value=True) + def test_submit_scratch_vs_scratch( + self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc): + """ + Tests that submitting a scratch build with the same NSV as a completed + scratch build succeeds + """ + FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml', + '620ec77321b2ea7b0d67d82992dda3e1d67055b4') + # Post so a scratch module build is in the init phase + post_url = '/module-build-service/1/module-builds/' + post_data = { + 'branch': 'master', + 'scmurl': 'https://src.stg.fedoraproject.org/modules/' + 'testmodule.git?#620ec77321b2ea7b0d67d82992dda3e1d67055b4', + 'scratch': True, + } + rv = self.client.post(post_url, data=json.dumps(post_data)) + assert rv.status_code == 201 + # Run the backend + stop = module_build_service.scheduler.make_simple_stop_condition(db.session) + module_build_service.scheduler.main([], stop) + # Post scratch build again and make sure it succeeds + rv2 = self.client.post(post_url, data=json.dumps(post_data)) + assert rv2.status_code == 201 + + @patch('module_build_service.auth.get_user', return_value=user) + @patch('module_build_service.scm.SCM') def test_submit_build_repo_regen_not_started_batch(self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc): """ From cd76a1dca5287ddd68ae60c5dbf4454c6fabdcb5 Mon Sep 17 00:00:00 2001 From: Merlin Mathesius Date: Apr 03 2019 18:15:59 +0000 Subject: [PATCH 2/3] Update scratch context uniqueness suffix character from '.' to '_'. Signed-off-by: Merlin Mathesius --- diff --git a/module_build_service/utils/submit.py b/module_build_service/utils/submit.py index 4a75933..761868e 100644 --- a/module_build_service/utils/submit.py +++ b/module_build_service/utils/submit.py @@ -683,7 +683,7 @@ def submit_module_build(username, mmd, params): log.debug('Found %d previous scratch module build context(s): %s', scrmods.count(), ",".join(scrmod_contexts)) # append incrementing counter to context - context_suffix = '.' + str(scrmods.count() + 1) + context_suffix = '_' + str(scrmods.count() + 1) mmd.set_context(mmd.get_context() + context_suffix) log.debug('Creating new module build') module = models.ModuleBuild.create( diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index ce497e3..35124f6 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -1236,7 +1236,7 @@ class TestBuild: module_build_id = data['id'] module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() # make sure scratch build has context with unique suffix - assert module_build.context == '9c690d0e.1' + assert module_build.context == '9c690d0e_1' @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') From 06e903c3c130f16ab41259a66a63095440457715 Mon Sep 17 00:00:00 2001 From: Merlin Mathesius Date: Apr 03 2019 18:15:59 +0000 Subject: [PATCH 3/3] Fixes and test improvements based on review feedback. Signed-off-by: Merlin Mathesius --- diff --git a/module_build_service/models.py b/module_build_service/models.py index 7ea03c5..a206b16 100644 --- a/module_build_service/models.py +++ b/module_build_service/models.py @@ -361,7 +361,7 @@ class ModuleBuild(MBSBase): """ return session.query(ModuleBuild).filter_by( name=name, stream=stream, version=str(version), scratch=True, **kwargs)\ - .filter(ModuleBuild.context.like(context + '%')) + .filter(ModuleBuild.context.like(context + '%')).all() @staticmethod def get_last_builds_in_stream_version_lte(session, name, stream_version): @@ -517,7 +517,7 @@ class ModuleBuild(MBSBase): @property def siblings(self): query = self.query.filter_by( - name=self.name, stream=self.stream, version=self.version).options( + name=self.name, stream=self.stream, version=self.version, scratch=self.scratch).options( load_only('id')).filter(ModuleBuild.id != self.id) return [build.id for build in query.all()] diff --git a/module_build_service/utils/submit.py b/module_build_service/utils/submit.py index 761868e..ef5c962 100644 --- a/module_build_service/utils/submit.py +++ b/module_build_service/utils/submit.py @@ -681,9 +681,9 @@ def submit_module_build(username, mmd, params): db.session, mmd.get_name(), mmd.get_stream(), version_str, mmd.get_context()) scrmod_contexts = [scrmod.context for scrmod in scrmods] log.debug('Found %d previous scratch module build context(s): %s', - scrmods.count(), ",".join(scrmod_contexts)) + len(scrmods), ",".join(scrmod_contexts)) # append incrementing counter to context - context_suffix = '_' + str(scrmods.count() + 1) + context_suffix = '_' + str(len(scrmods) + 1) mmd.set_context(mmd.get_context() + context_suffix) log.debug('Creating new module build') module = models.ModuleBuild.create( diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index 35124f6..41cffc0 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -1205,8 +1205,8 @@ class TestBuild: def test_submit_scratch_vs_normal( self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc): """ - Tests that submitting a scratch build with the same NSV as a completed - normal build succeeds + Tests that submitting a scratch build with the same NSV as a previously + completed normal build succeeds and both have expected contexts """ FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml', '620ec77321b2ea7b0d67d82992dda3e1d67055b4') @@ -1223,7 +1223,7 @@ class TestBuild: data = json.loads(rv.data) module_build_id = data['id'] module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() - # make sure normal build has expected context + # make sure normal build has expected context without a suffix assert module_build.context == '9c690d0e' # Run the backend stop = module_build_service.scheduler.make_simple_stop_condition(db.session) @@ -1235,7 +1235,7 @@ class TestBuild: data = json.loads(rv2.data) module_build_id = data['id'] module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() - # make sure scratch build has context with unique suffix + # make sure scratch build has expected context with unique suffix assert module_build.context == '9c690d0e_1' @patch('module_build_service.auth.get_user', return_value=user) @@ -1245,8 +1245,8 @@ class TestBuild: def test_submit_normal_vs_scratch( self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc): """ - Tests that submitting a normal build with the same NSV as a completed - scratch build succeeds + Tests that submitting a normal build with the same NSV as a previously + completed scratch build succeeds and both have expected contexts """ FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml', '620ec77321b2ea7b0d67d82992dda3e1d67055b4') @@ -1260,6 +1260,11 @@ class TestBuild: } rv = self.client.post(post_url, data=json.dumps(post_data)) assert rv.status_code == 201 + data = json.loads(rv.data) + module_build_id = data['id'] + module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() + # make sure scratch build has expected context with unique suffix + assert module_build.context == '9c690d0e_1' # Run the backend stop = module_build_service.scheduler.make_simple_stop_condition(db.session) module_build_service.scheduler.main([], stop) @@ -1267,6 +1272,11 @@ class TestBuild: post_data['scratch'] = False rv2 = self.client.post(post_url, data=json.dumps(post_data)) assert rv2.status_code == 201 + data = json.loads(rv2.data) + module_build_id = data['id'] + module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() + # make sure normal build has expected context without suffix + assert module_build.context == '9c690d0e' @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') @@ -1275,8 +1285,8 @@ class TestBuild: def test_submit_scratch_vs_scratch( self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc): """ - Tests that submitting a scratch build with the same NSV as a completed - scratch build succeeds + Tests that submitting a scratch build with the same NSV as a previously + completed scratch build succeeds and both have expected contexts """ FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml', '620ec77321b2ea7b0d67d82992dda3e1d67055b4') @@ -1290,12 +1300,22 @@ class TestBuild: } rv = self.client.post(post_url, data=json.dumps(post_data)) assert rv.status_code == 201 + data = json.loads(rv.data) + module_build_id = data['id'] + module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() + # make sure first scratch build has expected context with unique suffix + assert module_build.context == '9c690d0e_1' # Run the backend stop = module_build_service.scheduler.make_simple_stop_condition(db.session) module_build_service.scheduler.main([], stop) # Post scratch build again and make sure it succeeds rv2 = self.client.post(post_url, data=json.dumps(post_data)) assert rv2.status_code == 201 + data = json.loads(rv2.data) + module_build_id = data['id'] + module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one() + # make sure second scratch build has expected context with unique suffix + assert module_build.context == '9c690d0e_2' @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM')