From b9bf7b51974de4176fc8d8628066becc8256d369 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Jan 09 2020 09:38:52 +0000 Subject: frontend, backend: fix multi-build delete Previously we mixed-up actions that deleted buils with different copr_dirs. While we are on it, let's minimize the action dictionary. Fixes: #1180 Merges: #1182 --- diff --git a/backend/backend/actions.py b/backend/backend/actions.py index 259f795..8a0de1a 100644 --- a/backend/backend/actions.py +++ b/backend/backend/actions.py @@ -264,26 +264,39 @@ class Action(object): except CreateRepoError: self.log.exception("Error making local repo: %s", chroot_path) - def delete_build(self, ownername, project_dirname, chroot_builddirs): + def delete_builddirs(self, ownername, project_dirname, chroot_builddirs): + """ + delete build (sub)directories from chroot directory, chroot_builddirs + parameter is in form [{str_chrootname_2: [str_builddir1, str_builddir2]}] + """ self.log.info("Going to delete: %s", chroot_builddirs) - for chroot, builddir in chroot_builddirs.items(): - if not builddir: - self.log.warning("Received empty builddir!") + for chroot, builddirs in chroot_builddirs.items(): + if not builddirs: + self.log.warning("No builddirs received!") continue - chroot_path = os.path.join(self.destdir, ownername, project_dirname, chroot) - builddir_path = os.path.join(chroot_path, builddir) - - if not os.path.isdir(builddir_path): - self.log.error("%s not found", builddir_path) - continue + chroot_path = os.path.join(self.destdir, ownername, + project_dirname, chroot) + for builddir in builddirs: + builddir_path = os.path.join(chroot_path, builddir) + if not os.path.isdir(builddir_path): + self.log.error("%s not found", builddir_path) + continue - self.log.debug("builddir to be deleted %s", builddir_path) - shutil.rmtree(builddir_path) + self.log.debug("builddir to be deleted %s", builddir_path) + shutil.rmtree(builddir_path) def handle_delete_build(self): self.log.info("Action delete build.") + + # == EXAMPLE DATA == + # ownername: @copr + # projectname: TEST1576047114845905086Project10Fork + # project_dirname: TEST1576047114845905086Project10Fork:pr:12 + # chroot_builddirs: + # srpm-builds: [00849545] + # fedora-30-x86_64: [00849545-example] ext_data = json.loads(self.data["data"]) ownername = ext_data["ownername"] @@ -292,25 +305,31 @@ class Action(object): chroot_builddirs = ext_data["chroot_builddirs"] chroots = list(chroot_builddirs.keys()) - self.delete_build(ownername, project_dirname, chroot_builddirs) + self.delete_builddirs(ownername, project_dirname, chroot_builddirs) self.run_createrepo(ownername, projectname, project_dirname, chroots) def handle_delete_multiple_builds(self): self.log.debug("Action delete multiple builds.") + + # == EXAMPLE DATA == + # ownername: @copr + # projectname: testproject + # project_dirnames: + # testproject:pr:10: + # srpm-builds: [00849545, 00849546] + # fedora-30-x86_64: [00849545-example, 00849545-foo] + # [...] ext_data = json.loads(self.data["data"]) ownername = ext_data["ownername"] projectname = ext_data["projectname"] - project_dirname = ext_data["project_dirname"] - - chroots = [] - for chroot_builddirs in ext_data["builds"]: - self.delete_build(ownername, project_dirname, chroot_builddirs) - for chroot in chroot_builddirs.keys(): - if chroot not in chroots: - chroots.append(chroot) + project_dirnames = ext_data["project_dirnames"] - self.run_createrepo(ownername, projectname, project_dirname, chroots) + for project_dirname, chroot_builddirs in project_dirnames.items(): + self.delete_builddirs(ownername, project_dirname, chroot_builddirs) + chroots = [ch for ch in chroot_builddirs] + self.run_createrepo(ownername, projectname, project_dirname, + chroots) def handle_delete_chroot(self): self.log.info("Action delete project chroot.") diff --git a/backend/tests/test_action.py b/backend/tests/test_action.py index 149887d..031c71d 100644 --- a/backend/tests/test_action.py +++ b/backend/tests/test_action.py @@ -606,11 +606,11 @@ class TestAction(object): ext_data = json.dumps({ "ownername": "foo", "projectname": "bar", - "project_dirname": "bar", - "builds": [ - {"fedora-20": "01-foo"}, - {"fedora-20": "02-foo"}, - ] + "project_dirnames": { + 'bar': { + "fedora-20": ["01-foo", "02-foo"], + } + }, }) self.opts.destdir = tmp_dir diff --git a/frontend/coprs_frontend/coprs/logic/actions_logic.py b/frontend/coprs_frontend/coprs/logic/actions_logic.py index ad60e1a..f1953ca 100644 --- a/frontend/coprs_frontend/coprs/logic/actions_logic.py +++ b/frontend/coprs_frontend/coprs/logic/actions_logic.py @@ -119,10 +119,10 @@ class ActionsLogic(object): Creates a dictionary of chroot builddirs for build delete action :type build: models.build """ - chroot_builddirs = {'srpm-builds': build.result_dir} + chroot_builddirs = {'srpm-builds': [build.result_dir]} for build_chroot in build.build_chroots: - chroot_builddirs[build_chroot.name] = build_chroot.result_dir + chroot_builddirs[build_chroot.name] = [build_chroot.result_dir] return chroot_builddirs @@ -132,32 +132,25 @@ class ActionsLogic(object): Creates data needed for build delete action :type build: models.build """ - data = { + return { "ownername": build.copr.owner_name, "projectname": build.copr_name, + "project_dirname": + build.copr_dirname if build.copr_dir else build.copr_name, + "chroot_builddirs": cls.get_chroot_builddirs(build) } - if build.copr_dir: - data["project_dirname"] = build.copr_dirname - else: - data["project_dirname"] = build.copr_name - - return data - @classmethod def send_delete_build(cls, build): """ Schedules build delete action :type build: models.Build """ - data = cls.get_build_delete_data(build) - data["chroot_builddirs"] = cls.get_chroot_builddirs(build) - action = models.Action( action_type=ActionTypeEnum("delete"), object_type="build", object_id=build.id, - data=json.dumps(data), + data=json.dumps(cls.get_build_delete_data(build)), created_on=int(time.time()) ) db.session.add(action) @@ -168,11 +161,31 @@ class ActionsLogic(object): Schedules builds delete action for builds belonging to the same project :type build: list of models.Build """ - data = cls.get_build_delete_data(builds[0]) - data["builds"] = [] + project_dirnames = {} + data = {'project_dirnames': project_dirnames} + for build in builds: - chroot_builddirs = cls.get_chroot_builddirs(build) - data["builds"].append(chroot_builddirs) + build_delete_data = cls.get_build_delete_data(build) + + # inherit some params from the first build + for param in ['ownername', 'projectname']: + new = build_delete_data[param] + if param in data and data[param] != new: + # this shouldn't happen + raise exceptions.BadRequest("Can not delete builds " + "from more projects") + data[param] = new + + dirname = build_delete_data['project_dirname'] + if not dirname in project_dirnames: + project_dirnames[dirname] = {} + + project_dirname = project_dirnames[dirname] + for chroot, subdirs in build_delete_data['chroot_builddirs'].items(): + if chroot not in project_dirname: + project_dirname[chroot] = subdirs + else: + project_dirname[chroot].extend(subdirs) action = models.Action( action_type=ActionTypeEnum("delete"), diff --git a/frontend/coprs_frontend/tests/coprs_test_case.py b/frontend/coprs_frontend/tests/coprs_test_case.py index e822cd5..19ae5ee 100644 --- a/frontend/coprs_frontend/tests/coprs_test_case.py +++ b/frontend/coprs_frontend/tests/coprs_test_case.py @@ -397,7 +397,8 @@ class CoprsTestCase(object): user=self.u1, submitted_on=50, pkgs="http://example.com/copr-keygen-1.58-1.fc20.src.rpm", - pkg_version="1.58" + pkg_version="1.58", + result_dir='00000FEW', ) self.db.session.add(self.b_few_chroots) @@ -543,6 +544,23 @@ class CoprsTestCase(object): source_type=0) @pytest.fixture + def f_pr_build(self, f_mock_chroots, f_builds, f_pr_dir): + self.b_pr = models.Build( + copr=self.c1, copr_dir=self.c4_dir, package=self.p1, + user=self.u1, submitted_on=50, srpm_url="http://somesrpm", + source_status=StatusEnum("succeeded"), result_dir='0000PR') + + self.bc_pr = models.BuildChroot( + build=self.b_pr, + mock_chroot=self.mc2, + status=StatusEnum("succeeded"), + git_hash="deadbeef", + result_dir='0000PR-pr-package', + ) + + self.db.session.add_all([self.b_pr, self.bc_pr]) + + @pytest.fixture def f_batches(self): self.batch1 = models.Batch() self.batch2 = models.Batch() diff --git a/frontend/coprs_frontend/tests/test_logic/test_builds_logic.py b/frontend/coprs_frontend/tests/test_logic/test_builds_logic.py index d4d738a..417350b 100644 --- a/frontend/coprs_frontend/tests/test_logic/test_builds_logic.py +++ b/frontend/coprs_frontend/tests/test_logic/test_builds_logic.py @@ -221,7 +221,7 @@ class TestBuildsLogic(CoprsTestCase): assert len(ActionsLogic.get_many().all()) == 1 action = ActionsLogic.get_many().one() delete_data = json.loads(action.data) - assert delete_data['chroot_builddirs'] == {'srpm-builds': 'bar', 'fedora-18-x86_64': 'bar'} + assert delete_data['chroot_builddirs'] == {'srpm-builds': ['bar'], 'fedora-18-x86_64': ['bar']} with pytest.raises(NoResultFound): BuildsLogic.get(self.b1.id).one() diff --git a/frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_builds.py b/frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_builds.py index 0b6d276..0658d83 100644 --- a/frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_builds.py +++ b/frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_builds.py @@ -144,9 +144,9 @@ class TestCoprDeleteBuild(CoprsTestCase): self.b_few_chroots.build_chroots[1].status= StatusEnum("canceled") self.db.session.commit() - expected_chroot_builddirs = {'srpm-builds': self.b_few_chroots.result_dir} + expected_chroot_builddirs = {'srpm-builds': [self.b_few_chroots.result_dir]} for chroot in self.b_few_chroots.build_chroots: - expected_chroot_builddirs[chroot.name] = chroot.result_dir + expected_chroot_builddirs[chroot.name] = [chroot.result_dir] expected_dir = self.b_few_chroots.result_dir r = self.test_client.post( @@ -214,8 +214,7 @@ class TestCoprDeleteBuild(CoprsTestCase): @TransactionDecorator("u1") def test_copr_delete_multiple_builds_sends_single_action(self, f_users, f_coprs, - f_mock_chroots, - f_builds): + f_pr_build): for bc in self.b2_bc: bc.status = StatusEnum("canceled") self.db.session.add_all(self.b2_bc) @@ -224,10 +223,11 @@ class TestCoprDeleteBuild(CoprsTestCase): b_id1 = self.b1.id b_id2 = self.b2.id + b_id3 = self.b_pr.id url = "/coprs/{0}/{1}/delete_builds/".format(self.u1.name, self.c1.name) r = self.test_client.post( - url, data={"build_ids[]": [b_id1, b_id2]}, follow_redirects=True) + url, data={"build_ids[]": [b_id1, b_id2, b_id3]}, follow_redirects=True) assert r.status_code == 200 b1 = ( @@ -249,8 +249,16 @@ class TestCoprDeleteBuild(CoprsTestCase): assert act.object_type == "builds" assert data.get('ownername') == "user1" assert data.get('projectname') == "foocopr" - assert data.get('builds') == 2 * [{'fedora-18-x86_64': 'bar', - 'srpm-builds': 'bar'}] + assert data.get('project_dirnames') == { + 'foocopr': { + # they'd usually look like ID-PKGNAME, not 'bar' + 'fedora-18-x86_64': ['bar', 'bar'], + 'srpm-builds': ['bar', 'bar']}, + 'foocopr:PR': { + 'fedora-17-x86_64': ['0000PR-pr-package'], + 'srpm-builds': ['0000PR'], + } + } @TransactionDecorator("u1") def test_copr_delete_package_sends_single_action(self, f_users,