copr / copr

Clone

#943 frontend, backend: fixed project forking
Merged 4 years ago by praiskup. Opened 4 years ago by thrnciar.
copr/ thrnciar/copr forking  into  master

file modified
+25 -19 
@@ -132,32 +132,38 @@ 
 

  

              chroot_paths = set()
 

              for chroot, src_dst_dir in builds_map.items():
 

-                 src_dir, dst_dir = src_dst_dir[0], src_dst_dir[1]
 

-                 if not chroot or not src_dir or not dst_dir:
 

+ 

+                 if not chroot or not src_dst_dir:
 

                      continue
 

  

-                 old_chroot_path = os.path.join(old_path, chroot)
 

-                 new_chroot_path = os.path.join(new_path, chroot)
 

-                 chroot_paths.add(new_chroot_path)
 

+                 for old_dir_name, new_dir_name in src_dst_dir.items():
 

+                     src_dir, dst_dir = old_dir_name, new_dir_name
 

  

-                 src_path = os.path.join(old_chroot_path, src_dir)
 

-                 dst_path = os.path.join(new_chroot_path, dst_dir)
 

+                     if not src_dir or not dst_dir:
 

+                         continue
 

  

-                 if not os.path.exists(dst_path):
 

-                     os.makedirs(dst_path)
 

+                     old_chroot_path = os.path.join(old_path, chroot)
 

+                     new_chroot_path = os.path.join(new_path, chroot)
 

+                     chroot_paths.add(new_chroot_path)
 

  

-                 try:
 

-                     copy_tree(src_path, dst_path)
 

-                 except DistutilsFileError as e:
 

-                     self.log.error(str(e))
 

-                     continue
 

+                     src_path = os.path.join(old_chroot_path, src_dir)
 

+                     dst_path = os.path.join(new_chroot_path, dst_dir)
 

+ 

+                     if not os.path.exists(dst_path):
 

+                         os.makedirs(dst_path)
 

+ 

+                     try:
 

+                         copy_tree(src_path, dst_path)
 

+                     except DistutilsFileError as e:
 

+                         self.log.error(str(e))
 

+                         continue
 

  

-                 # Drop old signatures coming from original repo and re-sign.
 

-                 unsign_rpms_in_dir(dst_path, opts=self.opts, log=self.log)
 

-                 if sign:
 

-                     sign_rpms_in_dir(data["user"], data["copr"], dst_path, opts=self.opts, log=self.log)
 

+                     # Drop old signatures coming from original repo and re-sign.
 

+                     unsign_rpms_in_dir(dst_path, opts=self.opts, log=self.log)
 

+                     if sign:
 

+                         sign_rpms_in_dir(data["user"], data["copr"], dst_path, opts=self.opts, log=self.log)
 

  

-                 self.log.info("Forked build %s as %s", src_path, dst_path)
 

+                     self.log.info("Forked build %s as %s", src_path, dst_path)
 

  

              for chroot_path in chroot_paths:
 

                  createrepo(path=chroot_path, front_url=self.front_url,

file modified
+60 -2 
@@ -16,6 +16,8 @@ 
 

  from backend.exceptions import CreateRepoError, CoprKeygenRequestError
 

  from requests import RequestException
 

  

+ import os
 

+ 

  RESULTS_ROOT_URL = "http://example.com/results"
 

  STDOUT = "stdout"
 

  STDERR = "stderr"
 
@@ -43,13 +45,14 @@ 
 

              redis_db=9,
 

              redis_port=7777,
 

  

-             destdir=None,
 

+             destdir="/var/lib/copr/public_html/results/",
 

              frontend_base_url=None,
 

              results_baseurl=RESULTS_ROOT_URL,
 

  

-             do_sign=True,
 

+             do_sign=False,
 

  

              build_deleting_without_createrepo="",
 

+             keygen_host="example.com"
 

          )
 

  

      def teardown_method(self, method):
 
@@ -98,6 +101,61 @@ 
 

  

          self.dummy = str(test_action)
 

  

+     @mock.patch("backend.actions.copy_tree")
 

+     @mock.patch("backend.actions.os.path.exists")
 

+     @mock.patch("backend.actions.unsign_rpms_in_dir")
 

+     @mock.patch("backend.actions.createrepo")
 

+     def test_action_handle_forks(self, mc_createrepo, mc_unsign_rpms_in_dir, mc_exists, mc_copy_tree, mc_time):
 

+         mc_time.time.return_value = self.test_time
 

+         mc_front_cb = MagicMock()
 

+         mc_exists = True
 

+         test_action = Action(
 

+             opts=self.opts,
 

+             action={
 

+                 "action_type": ActionType.FORK,
 

+                 "id": 1,
 

+                 "object_type": "copr",
 

+                 "data": json.dumps(
 

+                     {
 

+                        'builds_map':
 

+                         {
 

+                             'srpm-builds': {
 

+                                 '00000002': '00000009', '00000005': '00000010'},
 

+                             'fedora-17-x86_64': {
 

+                                 '00000002-pkg1': '00000009-pkg1', '00000005-pkg2': '00000010-pkg2'},
 

+                             'fedora-17-i386': {
 

+                                 '00000002-pkg1': '00000009-pkg1', '00000005-pkg2': '00000010-pkg2'}
 

+                         },
 

+                         "user": "thrnciar",
 

+                         "copr": "source-copr"
 

+                     }),
 

+                 "old_value": "thrnciar/source-copr",
 

+                 "new_value": "thrnciar/destination-copr",
 

+             },
 

+             frontend_client=mc_front_cb,
 

+         )
 

+         test_action.run()
 

+         calls = mc_copy_tree.call_args_list
 

+         assert len(calls) == 6
 

+         assert calls[0][0] == (
 

+             "/var/lib/copr/public_html/results/thrnciar/source-copr/srpm-builds/00000002",
 

+             "/var/lib/copr/public_html/results/thrnciar/destination-copr/srpm-builds/00000009")
 

+         assert calls[1][0] == (
 

+             "/var/lib/copr/public_html/results/thrnciar/source-copr/srpm-builds/00000005",
 

+             "/var/lib/copr/public_html/results/thrnciar/destination-copr/srpm-builds/00000010")
 

+         assert calls[2][0] == (
 

+             "/var/lib/copr/public_html/results/thrnciar/source-copr/fedora-17-x86_64/00000002-pkg1",
 

+             "/var/lib/copr/public_html/results/thrnciar/destination-copr/fedora-17-x86_64/00000009-pkg1")
 

+         assert calls[3][0] == (
 

+             "/var/lib/copr/public_html/results/thrnciar/source-copr/fedora-17-x86_64/00000005-pkg2",
 

+             "/var/lib/copr/public_html/results/thrnciar/destination-copr/fedora-17-x86_64/00000010-pkg2")
 

+         assert calls[4][0] == (
 

+             "/var/lib/copr/public_html/results/thrnciar/source-copr/fedora-17-i386/00000002-pkg1",
 

+             "/var/lib/copr/public_html/results/thrnciar/destination-copr/fedora-17-i386/00000009-pkg1")
 

+         assert calls[5][0] == (
 

+             "/var/lib/copr/public_html/results/thrnciar/source-copr/fedora-17-i386/00000005-pkg2",
 

+             "/var/lib/copr/public_html/results/thrnciar/destination-copr/fedora-17-i386/00000010-pkg2")
 

+ 

      @unittest.skip("Fixme, test doesn't work.")
 

      def test_action_run_rename(self, mc_time):

file modified
+7 -2 
@@ -87,6 +87,7 @@ 
 

              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))
 

          for package in copr.main_dir.packages:
 

              fpackage = forking.fork_package(package, fcopr)
 

              build = package.last_build(successful=True)
 
@@ -96,11 +97,15 @@ 
 

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

  

              if build.result_dir:
 

-                 builds_map['srpm-builds'] = (build.result_dir, fbuild.result_dir)
 

+                 srpm_builds_src.append(build.result_dir)
 

+                 srpm_builds_dst.append(fbuild.result_dir)
 

  

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

                  if chroot.result_dir:
 

-                     builds_map[chroot.name] = (chroot.result_dir, fchroot.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))
 

  

          db.session.commit()
 

          ActionsLogic.send_fork_copr(copr, fcopr, builds_map)

file modified
+47 
@@ -282,6 +282,53 @@ 
 

          self.db.session.add_all([self.b1, self.b2, self.b3, self.b4])
 

  

      @pytest.fixture
 

+     def f_fork_prepare(self, f_coprs, f_mock_chroots, f_builds):
 

+ 

+         self.p4 = models.Package(
 

+             copr=self.c2, copr_dir=self.c2_dir, name="hello-world", source_type=0)
 

+         self.b5 = models.Build(
 

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

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

+             source_status=StatusEnum("succeeded"), result_dir='00000005-hello-world')
 

+         self.b6 = models.Build(
 

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

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

+             source_status=StatusEnum("failed"), result_dir='00000006-hello-world',
 

+             source_json='{}')
 

+         self.b7 = models.Build(
 

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

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

+             source_status=StatusEnum("succeeded"), result_dir='00000007-whatsupthere-world')
 

+         self.b8 = models.Build(
 

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

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

+             source_status=StatusEnum("succeeded"), result_dir='00000008-whatsupthere-world')
 

+ 

+         self.basic_builds = [self.b5, self.b6, self.b7, self.b8]
 

+         self.b5_bc = []
 

+         self.b6_bc = []
 

+         self.b7_bc = []
 

+         self.b8_bc = []
 

+         self.db.session.flush()
 

+ 

+         for build, build_chroots in zip(
 

+                 [self.b5, self.b6, self.b7, self.b8],
 

+                 [self.b5_bc, self.b6_bc, self.b7_bc, self.b8_bc]):
 

+ 

+             status = StatusEnum("succeeded")
 

+             for chroot in build.copr.active_chroots:
 

+                 buildchroot = models.BuildChroot(
 

+                     build=build,
 

+                     mock_chroot=chroot,
 

+                     status=status,
 

+                     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.b5, self.b6, self.b7, self.b8])
 

+ 

+     @pytest.fixture
 

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

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

          self.db.session.add(self.c1)

file modified
+14 -1 
@@ -23,8 +23,21 @@ 
 

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

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

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

-                 assert data["builds_map"] == {'fedora-18-x86_64': ['bar', '00000005-hello-world'], 'srpm-builds': ['bar', '00000005']}
 

+                 assert data["builds_map"] == {'srpm-builds': {'bar': '00000005'},'fedora-18-x86_64': {'bar': '00000005-hello-world'}}
 

  

+     @mock.patch("flask.g")
 

+     def test_fork_copr_projects_with_more_builds(self, mc_flask_g, f_users, f_fork_prepare, f_db):
 

+         mc_flask_g.user.name = self.u2.name
 

+         fc2, created = ComplexLogic.fork_copr(self.c2, self.u2, u"dstname")
 

+         self.db.session.commit()
 

+         actions = ActionsLogic.get_many(ActionTypeEnum("fork")).all()
 

+         assert len(actions) == 1
 

+         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'}}
 

  

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

          query = self.db.session.query(models.Copr)

This should fix broken project forking. In order to fix it, I had to
change the format of datas sending from frontend to backend.
Instead of lists containing names of old and new directories I am
sending dictionaries, where keys are old directories and values are
the names of new forked directories.

fixes #652

rebased onto d2e2dabbbc305bc26d5d615c08ce4234b38f9828
4 years ago

Looks like this is going to fix forking finally! Awesome, thank you.

Can you please create some more extensive unit-tests, namely that:
- Backend is able to fork project with 2+ packages, where one package is built 2+ times?
- that Frontend generates the json for backend correctly, in the very same situation ^

Metadata Update from @praiskup:
- Pull-request tagged with: release-blocker
4 years ago

I'd like to have this resolved before we do the release, because after the release we should add Fedora 31 chroots (and that action is implemented by Forking).

[copr-build]

Metadata Update from @praiskup:
- Pull-request untagged with: release-blocker
4 years ago

I was wrong, rawhide to release has it's own action sent by ActionsLogic.send_rawhide_to_release method. No need to block the release by this.

Metadata Update from @thrnciar:
- Pull-request tagged with: needs-work
4 years ago

1 new commit added

  • unit tests for forking, wip
4 years ago

2 new commits added

  • unit tests for forking, wip
  • frontend, backend: fixed project forking
4 years ago

2 new commits added

  • unit tests for forking, wip
  • frontend, backend: fixed project forking
4 years ago

2 new commits added

  • unit tests for forking
  • frontend, backend: fixed project forking
4 years ago

Metadata Update from @thrnciar:
- Pull-request untagged with: needs-work
4 years ago

rebased onto ee41241e469814d7d5be7e0ffb08969f947fcfa7
4 years ago

Why srpm-builds copies 4 => 9, and other chroots copy from 6 => 9?

Can you re-order so we have b5 -> b6 -> b7 -> b8, and use corresponding numbers in result_dir?

p2 is named whatsupthere-world, not whatsupthere-world-1, could we fix that?

Ouch, result_dir in srpm builds only should contain the ID, so I'd expect b7 to have 00000007.

Could we make b6 failed, so we'll test that b5 is actually taken instead?

It would be awesome to add backend test as well, for handle_fork method; we could disable self.opts.sign in the test, and set self.opts.destdir to appropriate directory - create some fake RPMs, and check that all directories were copied appropriately.

rebased onto 82705db0fc086309e7e55532ce66f5116c2e2a8e
4 years ago

Also note that the testsuite fails for copr-frontend.

rebased onto 7f90ffbeeb27a858a47c9f487b490ef6cc9010a0
4 years ago

Metadata Update from @praiskup:
- Pull-request tagged with: needs-work
4 years ago

Marking needs-work since Tomáš is working on backend test-case.

rebased onto 21c077ee39ba6fc6f684c24910584d7bd4d45ea5
4 years ago

rebased onto 9a5f42ccb7af00461219c3e3f864bed8599d35bb
4 years ago

Nice! Can you please check the destination parameter of copy_tree as well?

I'd slightly prefer a bit more "realistic" values, so:
- srpm-builds contain 00000002 => 00000009 pairs, and
- chroot builds contain 00000002-pkg1 => 00000009-pkg1

So one can easily understand what is going on and what is tested. But that's just cosmetics..

I think, I am already checking destination parameter of copy_tree. It should be this assert:
assert calls[1][0][0] == "/var/lib/copr/public_html/results/thrnciar/blabla1", is't it?

rebased onto 8513bf8b3be0ecd65b124ccde660704b3ceb8a8e
4 years ago

Metadata Update from @praiskup:
- Pull-request untagged with: needs-work
4 years ago

rebased onto f415ae1921c3b59e6ca91e1a0085c3a14e8912f5
4 years ago

please check this is necessary

rebased onto cb306c0002057047f72354076ed2b3ad097db5fa
4 years ago

rebased onto 11ade60d9db78c61b9c43c2434911813aaabdcba
4 years ago

please check this is necessary

yes, it has to be there. Test is not working without it.

@praiskup, I know that it looks awful, I have tried to do it like we talked. But I was getting strange AssertionError. The assertions are same, except of those parenthesis () == [].

> assert calls[0][0] == [ "/var/lib/copr/public_html/results/thrnciar/source-copr/srpm-builds/00000002", "/var/lib/copr/public_html/results/thrnciar/destination-copr/srpm-builds/00000009"] E AssertionError: assert ('/var/lib/co...lds/00000009') == ['/var/lib/cop...lds/00000009']

I guess you would have to use "tuples", ie (a, b) instead of [a, b].

But I don't see an issue in this expanded form, eventually.

Can you please put if not chroot into the outer loop?,

Otherwise very good job! Thank you @thrnciar.

rebased onto 6e211fe95923e5ddcea83a62846365feb781653f
4 years ago

rebased onto 6acd07523e2ff33837c05728bf49684f57694f5e
4 years ago

rebased onto a8a03a4
4 years ago

Commit a9b4c53 fixes this pull-request

Pull-Request has been merged by praiskup
4 years ago
Metadata
None
No Tags
Flags
Copr build
success (100%)
#1034486
4 years ago
Copr build
failure
#1034485
4 years ago
Copr build
success (100%)
#1034263
4 years ago
Copr build
failure
#1034262
4 years ago
Copr build
success (100%)
#1034257
4 years ago
Copr build
pending (50%)
#1034256
4 years ago
Copr build
success (100%)
#1033712
4 years ago
Copr build
failure
#1033711
4 years ago
Copr build
success (100%)
#1033707
4 years ago
Copr build
failure
#1033706
4 years ago
Copr build
success (100%)
#1033669
4 years ago
Copr build
failure
#1033668
4 years ago
Copr build
success (100%)
#1033566
4 years ago
Copr build
failure
#1033565
4 years ago
Copr build
success (100%)
#1033530
4 years ago
Copr build
failure
#1033529
4 years ago
Copr build
failure
#1032636
4 years ago
Copr build
failure
#1032635
4 years ago
Copr build
success (100%)
#1031108
4 years ago
Copr build
failure
#1031107
4 years ago
Copr build
success (100%)
#1031057
4 years ago
Copr build
failure
#1031056
4 years ago
Copr build
success (100%)
#1030619
4 years ago
Copr build
failure
#1030618
4 years ago
Copr build
success (100%)
#1030606
4 years ago
Copr build
failure
#1030605
4 years ago
Copr build
success (100%)
#1030602
4 years ago
Copr build
failure
#1030601
4 years ago
Copr build
success (100%)
#1030591
4 years ago
Copr build
failure
#1030590
4 years ago
Copr build
success (100%)
#1028582
4 years ago
Copr build
failure
#1028581
4 years ago
Copr build
success (100%)
#1022243
4 years ago
Copr build
failure
#1022242
4 years ago
Copr build
pending (50%)
#1021235
4 years ago
Copr build
failure
#1021234
4 years ago
Copr build
success (100%)
#1021228
4 years ago
Copr build
failure
#1021227
4 years ago
Changes Summary 5
+25 -19
file changed
backend/backend/actions.py
+60 -2
file changed
backend/tests/test_action.py
+7 -2
file changed
frontend/coprs_frontend/coprs/logic/complex_logic.py
+47 -0
file changed
frontend/coprs_frontend/tests/coprs_test_case.py
+14 -1
file changed
frontend/coprs_frontend/tests/test_logic/test_complex_logic.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.