PR#4021: newRepo: support hints for oldrepo value - koji

koji

#4021 newRepo: support hints for oldrepo value

Merged 4 months ago by tkopecek. Opened a year ago by mikem.

mikem/koji oldrepo-hint into master

add unit test. drop redundant call arg

Mike McLean • 10 months ago

6c20712

fix rebase issue and missing return values

Mike McLean • 10 months ago

1617c50

newRepo: support hints for oldrepo value

Mike McLean • 10 months ago

d25d523

builder/kojid

file modified

+46 -21

		`@@ -5824,6 +5824,50 @@`
		`self.logger.debug('Arch repo test passed %s' % arch)`
		`return True`

		`+ def get_old_repo(self, tinfo):`
		`+ # oldrepo is not used if createrepo_update is not set, so don't waste time`
		`+ if not self.options.createrepo_update:`
		`+ return None, None`
		`+`
		`+ # see if we can find a previous repo to update from`
		`+ oldrepo = self.session.getRepo(tinfo['id'])`
		`+ oldrepo_path = None`
		`+ if oldrepo:`
		`+ oldrepo_path = koji.pathinfo.repo(oldrepo['id'], tinfo['name'])`
		`+ oldrepo['tag_id'] = tinfo['id']`
		`+ return oldrepo, oldrepo_path`
		`+`
		`+ # If there is no old repo, try to find a suitable repo from a related tag`
		`+ # Check configured hints first - can be a single tag or list`
		`+ hint = tinfo['extra'].get('repo.oldrepo_hint', [])`
		`+ if not isinstance(hint, list):`
		`+ hint = [hint]`
		`+ for _tag in hint:`
		`+ reftag = self.session.getTag(_tag, strict=False)`
		`+ if not reftag:`
		`+ self.logger.warning('oldrepo_hint refers to invalid tag: %s', _tag)`
		`+ continue`
		`+ oldrepo = self.session.getRepo(reftag['id'])`
		`+ if oldrepo:`
		`+ oldrepo_path = koji.pathinfo.repo(oldrepo['id'], reftag['name'])`
		`+ oldrepo['tag_id'] = reftag['id']`
		`+ return oldrepo, oldrepo_path`
		`+`
		`+ # Otherwise consider tags in the inheritance`
		`+ parents = self.session.getFullInheritance(tinfo['id'])`
		`+ # we care about best candidate which should be (not necessarily)`
		`+ # something on higher levels. Sort tags according to depth.`
		`+ for link in sorted(parents, key=lambda x: x['currdepth']):`
		`+ oldrepo = self.session.getRepo(link['parent_id'])`
		`+ if oldrepo:`
		`+ parenttag = self.session.getTag(link['parent_id'])`
		`+ oldrepo_path = koji.pathinfo.repo(oldrepo['id'], parenttag['name'])`
		`+ oldrepo['tag_id'] = parenttag['id']`
		`+ return oldrepo, oldrepo_path`
		`+`
		`+ # otherwise no match`
		`+ return None, None`
		`+`
		`def handler(self, tag, event=None, src=None, debuginfo=None, separate_src=None, opts=None):`
		`tinfo = self.session.getTag(tag, strict=True, event=event)`

		`@@ -5862,27 +5906,8 @@`
		`for fn in os.listdir(path):`
		`if fn != 'groups' and os.path.isfile("%s/%s/pkglist" % (path, fn)):`
		`arches.append(fn)`
		`- # see if we can find a previous repo to update from`
		`- oldrepo_state = koji.REPO_READY`
		`- oldrepo = self.session.getRepo(tinfo['id'], state=oldrepo_state)`
		`- oldrepo_path = None`
		`- if oldrepo:`
		`- oldrepo_path = koji.pathinfo.repo(oldrepo['id'], tinfo['name'])`
		`- oldrepo['tag_id'] = tinfo['id']`
		`- # If there is no old repo, try to find first usable repo in`
		`- # inheritance chain and use it as a source. oldrepo is not used if`
		`- # createrepo_update is not set, so don't waste call in such case.`
		`- if not oldrepo and self.options.createrepo_update:`
		`- tags = self.session.getFullInheritance(tinfo['id'])`
		`- # we care about best candidate which should be (not necessarily)`
		`- # something on higher levels. Sort tags according to depth.`
		`- for tag in sorted(tags, key=lambda x: x['currdepth']):`
		`- oldrepo = self.session.getRepo(tag['parent_id'], state=oldrepo_state)`
		`- if oldrepo:`
		`- parenttag = self.session.getTag(tag['parent_id'])`
		`- oldrepo_path = koji.pathinfo.repo(oldrepo['id'], parenttag['name'])`
		`- oldrepo['tag_id'] = parenttag['id']`
		`- break`
		`+`
		`+ oldrepo, oldrepo_path = self.get_old_repo(tinfo)`
		`newrepo_path = koji.pathinfo.repo(repo_id, tinfo['name'])`
		`newrepo = {'tag_id': tinfo['id'], 'create_event': event_id}`
		`if self.options.copy_old_repodata:`

tests/test_builder/test_newRepo.py

file added

+173

		`@@ -0,0 +1,173 @@`
		`+ from __future__ import absolute_import`
		`+ import mock`
		`+ import os`
		`+ import rpm`
		`+ import shutil`
		`+ import tempfile`
		`+ import unittest`
		`+ import koji`
		`+ import koji.tasks`
		`+ from .loadkojid import kojid`
		`+ from six.moves import range`
		`+`
		`+`
		`+ class TestChooseTaskarch(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.tempdir = tempfile.mkdtemp()`
		`+ self.session = mock.MagicMock()`
		`+ self.options = mock.MagicMock()`
		`+ self.options.copy_old_repodata = False`
		`+ self.options.createrepo_update = True`
		`+ self.topdir = self.tempdir + '/topdir'`
		`+ self.options.topdir = self.topdir`
		`+ self.pathinfo = koji.PathInfo(self.topdir)`
		`+ mock.patch('koji.pathinfo', new=self.pathinfo).start()`
		`+`
		`+ # set up task handler`
		`+ task_id = 99`
		`+ method = 'newRepo'`
		`+ params = ['TAG']`
		`+ self.handler = kojid.NewRepoTask(task_id, method, params, self.session,`
		`+ self.options, self.tempdir + '/work')`
		`+`
		`+ # mock some more things`
		`+ self.wait = mock.MagicMock()`
		`+ self.handler.wait = self.wait`
		`+ self.session.getExternalRepoList.return_value = []`
		`+`
		`+ def tearDown(self):`
		`+ mock.patch.stopall()`
		`+ shutil.rmtree(self.tempdir)`
		`+`
		`+ def test_basic(self):`
		`+ self.session.getTag.return_value = {'id': 'TAGID', 'name': 'TAG'}`
		`+ self.session.host.repoInit.return_value = ['REPOID', 'EVENTID']`
		`+ os.makedirs(self.topdir + '/repos/TAG/REPOID')`
		`+ arches = ['x86_64', 'aarch64']`
		`+ for arch in arches:`
		`+ os.makedirs(f'{self.topdir}/repos/TAG/REPOID/{arch}')`
		`+ # touch pkglist`
		`+ with open(f'{self.topdir}/repos/TAG/REPOID/{arch}/pkglist', 'wt') as fo:`
		`+ fo.write('fake-1.2.3-1.rpm\n')`
		`+`
		`+ result = self.handler.run()`
		`+`
		`+ self.assertEqual(result, ('REPOID', 'EVENTID'))`
		`+ self.assertEqual(len(self.session.host.subtask.mock_calls), 2) # once per arch`
		`+`
		`+ def test_no_dir_access(self):`
		`+ self.session.getTag.return_value = {'id': 'TAGID', 'name': 'TAG'}`
		`+ self.session.host.repoInit.return_value = ['REPOID', 'EVENTID']`
		`+ # we don't make the repos dir`
		`+`
		`+ with self.assertRaises(koji.tasks.RefuseTask):`
		`+ result = self.handler.run()`
		`+`
		`+ self.session.host.repoInit.assert_not_called()`
		`+`
		`+ def test_oldrepo_no_update(self):`
		`+ taginfo = {'id': 'TAGID', 'name': 'TAG'}`
		`+ self.options.createrepo_update = False`
		`+`
		`+ result = self.handler.get_old_repo(taginfo)`
		`+`
		`+ self.assertEqual(result, (None, None))`
		`+ self.session.getRepo.assert_not_called()`
		`+`
		`+ def test_oldrepo_no_update(self):`
		`+ taginfo = {'id': 'TAGID', 'name': 'TAG'}`
		`+ self.options.createrepo_update = False`
		`+`
		`+ result = self.handler.get_old_repo(taginfo)`
		`+`
		`+ self.assertEqual(result, (None, None))`
		`+ self.session.getRepo.assert_not_called()`
		`+`
		`+ def test_oldrepo_simple(self):`
		`+ taginfo = {'id': 'TAGID', 'name': 'TAG'}`
		`+ repo = {'id': 'OLDREPOID', 'tag_id': 'TAGID'}`
		`+ self.session.getRepo.return_value = repo`
		`+`
		`+ result = self.handler.get_old_repo(taginfo)`
		`+`
		`+ self.assertEqual(result, (repo, self.topdir + '/repos/TAG/OLDREPOID'))`
		`+ self.session.getRepo.assert_called_once_with('TAGID')`
		`+ self.session.getFullInheritance.assert_not_called()`
		`+`
		`+ def test_oldrepo_hint(self):`
		`+ taginfo = {'id': 'TAGID', 'name': 'TAG', 'extra': {'repo.oldrepo_hint': 'OTHERTAG'}}`
		`+ taginfo2 = {'id': 'TAGID2', 'name': 'OTHERTAG'}`
		`+ repo = {'id': 'OLDREPOID', 'tag_id': 'TAGID2'}`
		`+ self.session.getRepo.side_effect = [None, repo]`
		`+ self.session.getTag.return_value = taginfo2`
		`+`
		`+ result = self.handler.get_old_repo(taginfo)`
		`+`
		`+ self.assertEqual(result, (repo, self.topdir + '/repos/OTHERTAG/OLDREPOID'))`
		`+ expected_calls = [`
		`+ mock.call('TAGID'),`
		`+ mock.call('TAGID2'),`
		`+ ]`
		`+ self.assertEqual(self.session.getRepo.mock_calls, expected_calls)`
		`+ self.session.getFullInheritance.assert_not_called()`
		`+`
		`+ def test_oldrepo_bad_hint(self):`
		`+ taginfo = {'id': 'TAGID', 'name': 'TAG',`
		`+ 'extra': {'repo.oldrepo_hint': ['BADTAG', 'OTHERTAG']}}`
		`+ taginfo2 = {'id': 'TAGID2', 'name': 'OTHERTAG'}`
		`+ repo = {'id': 'OLDREPOID', 'tag_id': 'TAGID2'}`
		`+ self.session.getRepo.side_effect = [None, repo]`
		`+ self.session.getTag.side_effect = [None, taginfo2]`
		`+`
		`+ result = self.handler.get_old_repo(taginfo)`
		`+`
		`+ self.assertEqual(result, (repo, self.topdir + '/repos/OTHERTAG/OLDREPOID'))`
		`+ expected_calls = [`
		`+ mock.call('TAGID'),`
		`+ mock.call('TAGID2'),`
		`+ ]`
		`+ self.assertEqual(self.session.getRepo.mock_calls, expected_calls)`
		`+ expected_calls = [`
		`+ mock.call('BADTAG', strict=False),`
		`+ mock.call('OTHERTAG', strict=False),`
		`+ ]`
		`+ self.assertEqual(self.session.getTag.mock_calls, expected_calls)`
		`+ self.session.getFullInheritance.assert_not_called()`
		`+`
		`+ def test_oldrepo_inherit(self):`
		`+ taginfo = {'id': 'TAGID', 'name': 'TAG', 'extra': {}}`
		`+ taginfo2 = {'id': 'PARENTID', 'name': 'PARENTTAG'}`
		`+ repo = {'id': 'OLDREPOID', 'tag_id': 'PARENTID'}`
		`+ self.session.getRepo.side_effect = [None, repo]`
		`+ self.session.getFullInheritance.return_value = [{'parent_id': 'PARENTID', 'currdepth': 1}]`
		`+ self.session.getTag.return_value = taginfo2`
		`+`
		`+ result = self.handler.get_old_repo(taginfo)`
		`+`
		`+ self.assertEqual(result, (repo, self.topdir + '/repos/PARENTTAG/OLDREPOID'))`
		`+ expected_calls = [`
		`+ mock.call('TAGID'),`
		`+ mock.call('PARENTID'),`
		`+ ]`
		`+ self.assertEqual(self.session.getRepo.mock_calls, expected_calls)`
		`+ self.session.getFullInheritance.assert_called_once()`
		`+`
		`+ def test_oldrepo_no_match(self):`
		`+ taginfo = {'id': 'TAGID', 'name': 'TAG', 'extra': {'repo.oldrepo_hint': 'OTHERTAG'}}`
		`+ taginfo2 = {'id': 'TAGID2', 'name': 'OTHERTAG'}`
		`+ self.session.getRepo.return_value = None`
		`+ self.session.getFullInheritance.return_value = [{'parent_id': 'PARENTID', 'currdepth': 1}]`
		`+ self.session.getTag.return_value = taginfo2`
		`+`
		`+ result = self.handler.get_old_repo(taginfo)`
		`+`
		`+ self.assertEqual(result, (None, None))`
		`+ expected_calls = [`
		`+ mock.call('TAGID'),`
		`+ mock.call('TAGID2'),`
		`+ mock.call('PARENTID'),`
		`+ ]`
		`+ self.assertEqual(self.session.getRepo.mock_calls, expected_calls)`
		`+ self.session.getFullInheritance.assert_called_once()`
		`+ # the end`

mikem commented a year ago

Opening this for discussion.

In tag clone situations and similar, the initial repo can't use --update, even though the content is substantially similar to existing tags. This change allows setting a hint in tag.extra that the newRepo task will consider.

For readability, I moved the oldrepo logic into its own call, but it's basically the same except for the bit in the middle that handles the hint.

A couple other tweaks are here:

the oldrepo_state option really only makes sense for SHADOWBUILD tags, so we don't pass it for the related tags
renamed a couple variables for clarity
moved the options.createrepo_update check up

Fixes https://pagure.io/koji/issue/4129

mikem commented a year ago

Side note: the code is (and was) tacitly assuming that options.createrepo_update is the same for all the createrepo builders. Of course, it's fairly nonsensical to have the setting vary, but it is technically a local setting on each host.

tkopecek commented a year ago

I think it usable in this state. Anyway, not sure where it would make sense to set this flag. Doing it for any clone operation probably doesn't make much sense as most cloned tags are not build tags. So, can we use it more broadly than in plugins?

mikem commented 10 months ago

This fell by the wayside. Needs rebasing (conflicts with kojira work) and I see a couple issues in the code. Maybe a good candidate for 1.35.1

rebased onto 9544b86

10 months ago

1 new commit added

fix rebase issue and missing return values

10 months ago

1 new commit added

add unit test. drop redundant call arg

10 months ago

mikem commented 10 months ago

Doing it for any clone operation probably doesn't make much sense as most cloned tags are not build tags

It would be harmless for it to be there if the tag is not used to make a repo, and it would help in the few cases where a cloned tag is used for building.

But yeah, this is definitely aimed at a narrow special case in a plugin. It might not get much use otherwise, but it will save a ton of time in the target case.