PR#1774: frontend: clone all CoprChroot attributes when forking - copr/copr

copr / copr

#1774 frontend: clone all CoprChroot attributes when forking

Merged 3 years ago by praiskup. Opened 3 years ago by praiskup.

Unknown source all-copr-chroot-attributes-forked into main

frontend: better test the branch-fedora command

Pavel Raiskup • 3 years ago

065d85d

frontend: print forking stats for rawhide-to-release

Pavel Raiskup • 3 years ago

f05bb90

frontend: rawhide-to-release fix for deactivated chroots

Pavel Raiskup • 3 years ago

6fc5dec

frontend: clone all CoprChroot attributes when forking

Pavel Raiskup • 3 years ago

eb901d2

frontend/coprs_frontend/commands/rawhide_to_release.py

file modified

+11 -8

		`@@ -95,6 +95,7 @@`
		`if not len(fork_builds):`
		`continue`

		`+ new_build_chroots = 0`
		`for build in fork_builds:`
		`chroot_exists = mock_chroot in build.chroots`

		`@@ -111,6 +112,7 @@`
		`if not chroot_exists:`
		`# forked chroot may already exists, e.g. from prevoius`
		`# 'rawhide-to-release-run'`
		`+ new_build_chroots += 1`
		`dest_build_chroot = builds_logic.BuildChrootsLogic.new(`
		`build=rbc.build,`
		`mock_chroot=mock_chroot,`
		`@@ -124,6 +126,12 @@`
		`if rbc.result_dir:`
		`data['builds'].append(rbc.result_dir)`

		`+ if data["builds"] or new_build_chroots:`
		`+ print(" Fresh new build chroots: {}, regenerate {}".format(`
		`+ new_build_chroots,`
		`+ len(data["builds"]) - new_build_chroots,`
		`+ ))`
		`+`
		`if len(data["builds"]):`
		`actions_logic.ActionsLogic.send_rawhide_to_release(data)`

		`@@ -131,16 +139,11 @@`

		`def turn_on_the_chroot_for_copr(copr, rawhide_name, mock_chroot):`
		`rawhide_chroot = None`
		`- for chroot in copr.active_copr_chroots:`
		`+ for chroot in copr.copr_chroots:`
		`if chroot.name == rawhide_name:`
		`rawhide_chroot = chroot`
		`if chroot.name == mock_chroot.name:`
		`# already created`
		`return`
		`-`
		`- create_kwargs = {`
		`- "buildroot_pkgs": rawhide_chroot.buildroot_pkgs,`
		`- "comps": rawhide_chroot.comps,`
		`- "comps_name": rawhide_chroot.comps_name,`
		`- }`
		`- coprs_logic.CoprChrootsLogic.create_chroot(copr.user, copr, mock_chroot, **create_kwargs)`
		`+ coprs_logic.CoprChrootsLogic.create_chroot_from(rawhide_chroot,`
		`+ mock_chroot=mock_chroot)`

frontend/coprs_frontend/config/copr_unit_test.conf

file modified

		`@@ -4,6 +4,9 @@`
		`import time`
		`import os`

		`+ # We need to set this so copr_url() can be used in unit tests`
		`+ SERVER_NAME = "localhost"`
		`+`
		`PROJECT_ROOT = os.path.abspath(os.path.join(os.path.dirname( __file__ ), '..', '..', '..'))`
		`LOCAL_TMP_DIR = os.path.join(PROJECT_ROOT, '_tmp', str(int(time.time())) )`

frontend/coprs_frontend/coprs/helpers.py

file modified

+34

		`@@ -659,3 +659,37 @@`
		`items[0],`
		`" is" if be_suffix else ""`
		`)`
		`+`
		`+ def clone_sqlalchemy_instance(instance, ignored=None):`
		`+ """`
		`+ Clone an object, but skip the primary key.`
		`+ """`
		`+ new_instance = type(instance)()`
		`+ if not ignored:`
		`+ ignored = []`
		`+`
		`+ # Copy the normal table columns.`
		`+ for col in instance.__table__.columns:`
		`+ column = col.name`
		`+ if column in ignored:`
		`+ continue`
		`+ if not hasattr(instance, column):`
		`+ # stuff like user_id in _UserPrivate`
		`+ continue`
		`+ if col.primary_key:`
		`+ # the new object needs to have it's own unique primary key`
		`+ continue`
		`+ if col.foreign_keys:`
		`+ # we copy the references instead below`
		`+ continue`
		`+ setattr(new_instance, column, getattr(instance, column))`
		`+`
		`+ # Copy the references. It is better to copy 'new.parent = old.parent'`
		`+ # than just 'old.parent_id' because the 'new' object wouldn't have the`
		`+ # 'new.parent' object loaded.`
		`+ for attr, rel in instance.__mapper__.relationships.items():`
		`+ if rel.uselist:`
		`+ # TODO: support also 1:N, not only N:1`
		`+ continue`
		`+ setattr(new_instance, attr, getattr(instance, attr))`
		`+ return new_instance`

frontend/coprs_frontend/coprs/logic/complex_logic.py

file modified

+4 -3

		`@@ -330,9 +330,10 @@`
		`fcopr_dir = models.CoprDir(name=fcopr.name, copr=fcopr, main=True)`

		`for chroot in list(copr.active_copr_chroots):`
		`- CoprChrootsLogic.create_chroot(self.user, fcopr, chroot.mock_chroot, chroot.buildroot_pkgs,`
		`- chroot.repos, comps=chroot.comps, comps_name=chroot.comps_name,`
		`- with_opts=chroot.with_opts, without_opts=chroot.without_opts)`
		`+ CoprChrootsLogic.create_chroot_from(chroot,`
		`+ mock_chroot=chroot.mock_chroot,`
		`+ copr=fcopr)`
		`+`
		`db.session.add(fcopr)`
		`db.session.add(fcopr_dir)`

frontend/coprs_frontend/coprs/logic/coprs_logic.py

file modified

+18 -1

		`@@ -20,7 +20,7 @@`
		`from coprs.exceptions import MalformedArgumentException, BadRequest`
		`from coprs.logic import users_logic`
		`from coprs.whoosheers import CoprWhoosheer`
		`- from coprs.helpers import fix_protocol_for_backend`
		`+ from coprs.helpers import fix_protocol_for_backend, clone_sqlalchemy_instance`

		`from coprs.logic.actions_logic import ActionsLogic`
		`from coprs.logic.users_logic import UsersLogic`
		`@@ -681,6 +681,23 @@`
		`return chroot`

		`@classmethod`
		`+ def create_chroot_from(cls, from_copr_chroot, copr=None, mock_chroot=None):`
		`+ """`
		`+ Create a new CoprChroot object for USER, COPR and MOCK_CHROOT,`
		`+ inheriting the configuration from FROM_COPR_CHROOT.`
		`+ """`
		`+ assert copr or mock_chroot`
		`+ copr_chroot = clone_sqlalchemy_instance(from_copr_chroot, ["build_chroots"])`
		`+ if mock_chroot is not None:`
		`+ copr_chroot.mock_chroot = mock_chroot`
		`+ if copr is not None:`
		`+ copr_chroot.copr = copr`
		`+ db.session.add(copr_chroot)`
		`+ if copr_chroot.comps_name is not None:`
		`+ ActionsLogic.send_update_comps(copr_chroot)`
		`+ return copr_chroot`
		`+`
		`+ @classmethod`
		`def update_chroot(cls, user, copr_chroot, buildroot_pkgs=None, repos=None, comps=None, comps_name=None,`
		`with_opts="", without_opts="", delete_after=None, delete_notify=None, module_toggle="",`
		`bootstrap=None, bootstrap_image=None, isolation=None):`

frontend/coprs_frontend/tests/test_commands/test_rawhide_to_release.py

file modified

+38 -4

		`@@ -6,16 +6,21 @@`

		`from coprs import db, models`
		`from coprs.logic import coprs_logic`
		`- from tests.coprs_test_case import CoprsTestCase, new_app_context`
		`- from copr_common.enums import StatusEnum`
		`+ from copr_common.enums import StatusEnum, ActionTypeEnum`
		`# pylint: disable=wrong-import-order`
		`from commands.branch_fedora import branch_fedora_function`

		`+ from coprs_frontend.tests.coprs_test_case import CoprsTestCase, new_app_context`

		`@pytest.mark.usefixtures("f_copr_chroots_assigned_finished")`
		`class TestBranchFedora(CoprsTestCase):`
		`+`
		`+ def _get_actions(self):`
		`+ actions = self.models.Action.query.all()`
		`+ return [ActionTypeEnum(a.action_type) for a in actions]`
		`+`
		`@new_app_context`
		`- def test_branch_fedora(self):`
		`+ def test_branch_fedora(self, capsys):`
		`""" Test rawhide-to-release through branch-fedora command """`

		`# Create one build which is also built in rawhide-i386 chroot`
		`@@ -26,7 +31,6 @@`
		`db.session.add(b_rawhide)`

		`for cch in self.c3.copr_chroots:`
		`- print("adding {} buildchroot into {}".format(cch.name, b_rawhide))`
		`bch_rawhide = models.BuildChroot(`
		`build=b_rawhide,`
		`mock_chroot=cch.mock_chroot,`
		`@@ -53,6 +57,8 @@`
		`db.session.add(cc_rawhide_x86_64)`
		`db.session.commit()`

		`+ assert self._get_actions() == []`
		`+`
		`assert len(self.c1.copr_chroots) == 2`
		`assert len(self.c3.copr_chroots) == 2`

		`@@ -81,3 +87,31 @@`

		`assert bch.status == StatusEnum("forked")`
		`assert bch.build.package == self.p3`
		`+`
		`+ assert self._get_actions() == ["rawhide_to_release"]`
		`+`
		`+ # re-run the command, this is no-op`
		`+ branch_fedora_function(19, False, 'f19')`
		`+ assert self._get_actions() == ["rawhide_to_release"]`
		`+`
		`+ # re-run, and re-fork all the builds, generates new action`
		`+ branch_fedora_function(19, True, 'f19')`
		`+ assert self._get_actions() == ["rawhide_to_release",`
		`+ "rawhide_to_release"]`
		`+`
		`+ stdout, _ = capsys.readouterr()`
		`+ assert stdout == "\n".join([`
		`+ "Handling builds in copr 'user2/barcopr', chroot 'fedora-rawhide-i386'",`
		`+ " Fresh new build chroots: 1, regenerate 0",`
		`+ "Handling builds in copr 'user1/foocopr', chroot 'fedora-rawhide-x86_64'",`
		`+ "fedora-19-i386 - already exists.",`
		`+ "fedora-19-x86_64 - already exists.",`
		`+ "Handling builds in copr 'user2/barcopr', chroot 'fedora-rawhide-i386'",`
		`+ "Handling builds in copr 'user1/foocopr', chroot 'fedora-rawhide-x86_64'",`
		`+ "fedora-19-i386 - already exists.",`
		`+ "fedora-19-x86_64 - already exists.",`
		`+ "Handling builds in copr 'user2/barcopr', chroot 'fedora-rawhide-i386'",`
		`+ " Fresh new build chroots: 0, regenerate 1",`
		`+ "Handling builds in copr 'user1/foocopr', chroot 'fedora-rawhide-x86_64'",`
		`+ ""`
		`+ ])`

frontend/coprs_frontend/tests/test_helpers.py

file modified

+34 -1

		`@@ -9,7 +9,7 @@`
		`from coprs.helpers import parse_package_name, generate_repo_url, \`
		`fix_protocol_for_frontend, fix_protocol_for_backend, pre_process_repo_url, \`
		`parse_repo_params, pagure_html_diff_changed, SubdirMatch, \`
		`- raw_commit_changes, WorkList, pluralize`
		`+ raw_commit_changes, WorkList, pluralize, clone_sqlalchemy_instance`

		`from tests.coprs_test_case import CoprsTestCase`

		`@@ -228,6 +228,39 @@`
		`assert pagure_html_diff_changed('some-dust-ajablůňka>isdf/#~<--') == set([])`
		`assert pagure_html_diff_changed(b'091213114151deadbeaf') == set([])`

		`+ @pytest.mark.usefixtures("f_users", "f_coprs", "f_mock_chroots", "f_db")`
		`+ def test_orm_object_clone(self):`
		`+ """`
		`+ Test here that we are able to clone several types of ORM objects we have`
		`+ in Copr database.`
		`+ """`
		`+ # "fedora-17-x86_64" from "user2/foocopr"`
		`+ original = self.models.CoprChroot.query.get(2)`
		`+ # "user1/foocopr"`
		`+ target_copr = self.models.Copr.query.get(1)`
		`+`
		`+ assert "fedora-17-x86_64" not in \`
		`+ [m.name for m in target_copr.mock_chroots]`
		`+`
		`+ # Check this is copied, even though it causes duplicity - caller needs`
		`+ # to take care of unique keys for now.`
		`+ copy = clone_sqlalchemy_instance(original)`
		`+ assert copy.copr == original.copr`
		`+ assert copy.mock_chroot == original.mock_chroot`
		`+ # These are not copied.`
		`+ assert copy.copr_id is None`
		`+ assert copy.mock_chroot_id is None`
		`+ # Array fields are not copied.`
		`+ assert copy.build_chroots == []`
		`+`
		`+ # remove the duplicity, and commit (no traceback)`
		`+ copy.copr = target_copr`
		`+ self.db.session.commit()`
		`+`
		`+ target_copr = self.models.Copr.query.get(1)`
		`+ assert "fedora-17-x86_64" in \`
		`+ [m.name for m in target_copr.mock_chroots]`
		`+`

		`def test_worklist_class():`
		`""" test that all tasks are processed only once """`

praiskup commented 3 years ago

This brings in a little bit of experiment, a new function
clone_sqlalchemy_instance() that should be able to clone SQLAlchemy ORM
objects. This first implementation isn't able to clone list (1=>N)
relations.

Fixes: #1760

zuul commented 3 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 47s

frostyx commented on line 26 of frontend/coprs_frontend/coprs/helpers.py 3 years ago

I think this has something to do with

This first implementation isn't able to clone list (1=>N) relations.

Can you please add some comment or TODO here into the code?

frostyx commented 3 years ago

I guess this is transitively tested by test_rawhide_to_release.py but I think it would be worth it to add some simple tests directly for this new clone_sqlalchemy_instance helper function. It would help us catch potential issues without investigating what is wrong with rawhide_to_release command.

Otherwise, it looks good to me,
thank you.

rebased onto b2347a8102f0fd2151ee98b61e6a723ab379a659

3 years ago

praiskup commented 3 years ago