#1079 frontend: fix forking correct chroots and forked projects
Closed 4 years ago by frostyx. Opened 4 years ago by dturecek.
copr/ dturecek/copr fix-forking  into  master

@@ -88,25 +88,32 @@ 

              raise exceptions.DuplicateException("Source project should not be same as destination")

  

          builds_map = {}

-         srpm_builds_src,srpm_builds_dst, chroots_src, chroots_dst = ([] for i in range(4))

+         srpm_builds_src = []

+         srpm_builds_dst = []

+ 

          for package in copr.main_dir.packages:

              fpackage = forking.fork_package(package, fcopr)

-             build = package.last_build(successful=True)

-             if not build:

+ 

+             builds = PackagesLogic.last_successful_build_chroots(package)

+             if not builds:

                  continue

  

-             fbuild = forking.fork_build(build, fcopr, fpackage)

+             for build, build_chroots in builds.items():

+                 fbuild = forking.fork_build(build, fcopr, fpackage, build_chroots)

+ 

+                 if build.result_dir:

+                     srpm_builds_src.append(build.result_dir)

+                     srpm_builds_dst.append(fbuild.result_dir)

  

-             if build.result_dir:

-                 srpm_builds_src.append(build.result_dir)

-                 srpm_builds_dst.append(fbuild.result_dir)

+                 for chroot, fchroot in zip(build_chroots, fbuild.build_chroots):

+                     if not chroot.result_dir:

+                         continue

+                     if chroot.name not in builds_map:

+                         builds_map[chroot.name] = {chroot.result_dir: fchroot.result_dir}

+                     else:

+                         builds_map[chroot.name][chroot.result_dir] = fchroot.result_dir

  

-             for chroot, fchroot in zip(build.build_chroots, fbuild.build_chroots):

-                 if chroot.result_dir:

-                     chroots_src.append(chroot.result_dir)

-                     chroots_dst.append(fchroot.result_dir)

-                 builds_map['srpm-builds'] = dict(zip(srpm_builds_src, srpm_builds_dst))

-                 builds_map[chroot.name] = dict(zip(chroots_src, chroots_dst))

+         builds_map['srpm-builds'] = dict(zip(srpm_builds_src, srpm_builds_dst))

  

          db.session.commit()

          ActionsLogic.send_fork_copr(copr, fcopr, builds_map)
@@ -294,7 +301,7 @@ 

              db.session.add(fpackage)

          return fpackage

  

-     def fork_build(self, build, fcopr, fpackage):

+     def fork_build(self, build, fcopr, fpackage, build_chroots):

          fbuild = self.create_object(models.Build, build, exclude=["id", "copr_id", "copr_dir_id", "package_id", "result_dir"])

          fbuild.copr = fcopr

          fbuild.package = fpackage
@@ -303,7 +310,7 @@ 

          db.session.flush()

  

          fbuild.result_dir = '{:08}'.format(fbuild.id)

-         fbuild.build_chroots = [self.create_object(models.BuildChroot, c, exclude=["id", "build_id", "result_dir"]) for c in build.build_chroots]

+         fbuild.build_chroots = [self.create_object(models.BuildChroot, c, exclude=["id", "build_id", "result_dir"]) for c in build_chroots]

          for chroot in fbuild.build_chroots:

              chroot.result_dir = '{:08}-{}'.format(fbuild.id, fpackage.name)

              chroot.status = StatusEnum("forked")

@@ -12,6 +12,7 @@ 

  

  from coprs.logic import users_logic

  from coprs.logic import builds_logic

+ from copr_common.enums import StatusEnum

  

  log = app.logger

  
@@ -314,3 +315,21 @@ 

  

          if counter > 0:

              db.session.commit()

+ 

+     @classmethod

+     def last_successful_build_chroots(cls, package):

+         builds = {}

+         for chroot in package.chroots:

+             for build in reversed(package.builds):

+                 try:

+                     build_chroot = build.chroots_dict_by_name[chroot.name]

+                 except KeyError:

+                     continue

+                 if build_chroot.status not in [StatusEnum("succeeded"), StatusEnum("forked")]:

+                     continue

+                 if build not in builds:

+                     builds[build] = [build_chroot]

+                 else:

+                     builds[build].append(build_chroot)

+                 break

+         return builds

@@ -328,6 +328,49 @@ 

                  self.db.session.add(buildchroot)

          self.db.session.add_all([self.b5, self.b6, self.b7, self.b8])

  

+         self.p5 = models.Package(

+             copr=self.c2, copr_dir=self.c2_dir, name="new-package", source_type=0)

+         self.b9 = models.Build(

+             copr=self.c2, copr_dir=self.c2_dir, package=self.p5,

+             user=self.u1, submitted_on=100, srpm_url="http://somesrpm",

+             source_status=StatusEnum("succeeded"), result_dir='00000009-new-package')

+         self.b10 = models.Build(

+             copr=self.c2, copr_dir=self.c2_dir, package=self.p5,

+             user=self.u1, submitted_on=100, srpm_url="http://somesrpm",

+             source_status=StatusEnum("failed"), result_dir='00000010-new-package')

+         self.b11 = models.Build(

+             copr=self.c2, copr_dir=self.c2_dir, package=self.p5,

+             user=self.u1, submitted_on=100, srpm_url="http://somesrpm",

+             source_status=StatusEnum("failed"), result_dir='00000011-new-package')

+ 

+         self.b9_bc = []

+         self.b10_bc = []

+         self.b11_bc = []

+         self.db.session.flush()

+ 

+         bc_status = {self.b9: {self.mc2: StatusEnum("succeeded"),

+                                self.mc3: StatusEnum("succeeded")},

+                      self.b10: {self.mc2: StatusEnum("forked"),

+                                 self.mc3: StatusEnum("failed")},

+                      self.b11: {self.mc2: StatusEnum("failed"),

+                                 self.mc3: StatusEnum("succeeded")}}

+ 

+         for build, build_chroots in zip(

+                 [self.b9, self.b10, self.b11],

+                 [self.b9_bc, self.b10_bc, self.b11_bc]):

+ 

+             for chroot in build.copr.active_chroots:

+                 buildchroot = models.BuildChroot(

+                     build=build,

+                     mock_chroot=chroot,

+                     status=bc_status[build][chroot],

+                     git_hash="12345",

+                     result_dir="{}-{}".format(build.id, build.package.name),

+                 )

+                 build_chroots.append(buildchroot)

+                 self.db.session.add(buildchroot)

+         self.db.session.add_all([self.b9, self.b10, self.b11])

+ 

      @pytest.fixture

      def f_hook_package(self, f_users, f_coprs, f_mock_chroots, f_builds):

          self.c1.webhook_secret = str(uuid.uuid4())

@@ -36,9 +36,15 @@ 

          data = json.loads(actions[0].data)

          assert data["user"] == self.u2.name

          assert data["copr"] == "dstname"

-         assert data["builds_map"] == {'srpm-builds': {'00000008-whatsupthere-world': '00000009', '00000005-hello-world': '00000010'},

-                    'fedora-17-x86_64': {'8-whatsupthere-world': '00000009-whatsupthere-world', '5-hello-world': '00000010-hello-world'},

-                    'fedora-17-i386': {'8-whatsupthere-world': '00000009-whatsupthere-world', '5-hello-world': '00000010-hello-world'}}

+         assert data["builds_map"] == {

+             'srpm-builds': {'00000008-whatsupthere-world': '00000012', '00000006-hello-world': '00000013',

+                             '00000010-new-package': '00000014', '00000011-new-package': '00000015'},

+             'fedora-17-x86_64': {'8-whatsupthere-world': '00000012-whatsupthere-world',

+                                  '6-hello-world': '00000013-hello-world',

+                                  '10-new-package': '00000014-new-package'},

+             'fedora-17-i386': {'8-whatsupthere-world': '00000012-whatsupthere-world',

+                                '6-hello-world': '00000013-hello-world',

+                                '11-new-package': '00000015-new-package'}}

  

      def test_delete_expired_coprs(self, f_users, f_mock_chroots, f_coprs, f_builds, f_db):

          query = self.db.session.query(models.Copr)
@@ -93,7 +99,7 @@ 

  

      def test_fork_build(self, f_users, f_coprs, f_mock_chroots, f_builds, f_db):

          forking = ProjectForking(self.u1)

-         fb1 = forking.fork_build(self.b1, self.c2, self.p2)

+         fb1 = forking.fork_build(self.b1, self.c2, self.p2, self.b1.build_chroots)

  

          assert fb1.id != self.b1.id

          assert fb1.state == 'forked'

@@ -0,0 +1,13 @@ 

+ from coprs.logic.packages_logic import PackagesLogic

+ 

+ from tests.coprs_test_case import CoprsTestCase

+ 

+ 

+ class TestPackagesLogic(CoprsTestCase):

+ 

+     def test_last_successful_build_chroots(self, f_users, f_fork_prepare, f_build_few_chroots):

+         builds_p4 = PackagesLogic.last_successful_build_chroots(self.p4)

+         builds_p5 = PackagesLogic.last_successful_build_chroots(self.p5)

+         assert builds_p4 == {self.b6: self.b6_bc}

+         assert builds_p5 == {self.b10: [self.b10_bc[0]],

+                              self.b11: [self.b11_bc[1]]}

This is unnecessarily complicated now, I wouldn't be afraid to go with simple

srpm_builds_src = []
srpm_builds_dst = []

rebased onto 3896bc2c2ae451579acff71625b73cfd6eaf0ebf

4 years ago

I would move this method to packages_logic.py

nitpick: don't get me wrong, I love map too, but I would go with simple [StatusEnum("succeeded"), StatusEnum("forked")].

I don't understand this. We go through for build in reversed(self.builds): and then unconditionally break after the first run through the loop? In that case, we don't need the for cycle and can go with reversed(self.builds)[0] just fine. Am I missing something?

The last_successful_build_chroots method is quite complex. It might deserve a unit test.

If the build_chroot that we are currently looking for isn't successful or not in the build in the current loop at all, we continue with the next build. So this way, if we reach the break, it means we've already found the build we want.

Ah, I missed the continue, it makes sense now. Thank you @dturecek

rebased onto 6206d14

4 years ago

I've addressed your points and made a unit test for the method.
However, I've encountered another problem and had to change the logic of the fork_copr method a bit. Please take another look.

Metadata Update from @frostyx:
- Pull-request tagged with: can-be-merged

4 years ago

Merged manually, closing the PR.

Pull-Request has been closed by frostyx

4 years ago