PR#4267: fix repo handing for bare wrapperRPM task - koji

koji

#4267 fix repo handing for bare wrapperRPM task

Merged 6 months ago by tkopecek. Opened 7 months ago by mikem.

mikem/koji wrapper-repo-check into master

fix repo handing for bare wrapperRPM task

Mike McLean • 7 months ago

bb63c26

builder/kojid

file modified

+8 -4

		`@@ -2245,13 +2245,17 @@`
		`error_msg = 'custom_user_metadata is not JSON serializable'`
		`raise koji.BuildError(error_msg)`

		`+ # sort out remaining buildroot params`
		`+ build_tag = self.session.getTag(build_target['build_tag'], strict=True)`
		`repo_id = opts.get('repo_id')`
		`if not repo_id:`
		`- raise koji.BuildError("A repo id must be provided")`
		`-`
		`- repo_info = self.session.repoInfo(repo_id, strict=True)`
		`+ # a parent task will generally pass in the repo it used, but if`
		`+ # we are top level, we'll need to find our own`
		`+ repo_info = self.getRepo(build_tag['id'])`
		`+ else:`
		`+ repo_info = self.session.repoInfo(repo_id, strict=True)`
		`+ repo_id = repo_info['id']`
		`event_id = repo_info['create_event']`
		`- build_tag = self.session.getTag(build_target['build_tag'], strict=True)`
		`br_arch = self.find_arch('noarch', self.session.host.getHost(`
		`), self.session.getBuildConfig(build_tag['id'], event=event_id))`

kojihub/kojihub.py

file modified

+4 -5

		`@@ -10828,11 +10828,10 @@`
		`raise koji.PreBuildError('wrapper rpms for %s have already been built' %`
		`koji.buildLabel(build))`
		`build_target = self.getBuildTarget(target, strict=True)`
		`- build_tag = self.getTag(build_target['build_tag'], strict=True)`
		`- repo_info = self.getRepo(build_tag['id'])`
		`- if not repo_info:`
		`- raise koji.PreBuildError('no repo for tag: %s' % build_tag['name'])`
		`- opts['repo_id'] = repo_info['id']`
		`+ if 'repo_id' in opts:`
		`+ # we ignore this opt for backwards compatibility`
		`+ logger.warning('The wrapperRPM call ignores repo_id options')`
		`+ del opts['repo_id']`
tkopecek commented 7 months ago This is not clear to me - kojid part still use that option, so why it is remove here?

		`taskOpts = {}`
		`if priority:`

tests/test_hub/test_wrapper_rpm.py

file modified

+20 -12

		`@@ -76,18 +76,6 @@`
		`self.assertEqual("wrapper rpms for %s have already been built" % self.build,`
		`str(cm.exception))`

		`- def test_no_repo_for_tag(self):`
		`- self.context.opts.get.return_value = True`
		`- self.exports.getBuild.return_value = self.buildinfo`
		`- self.list_rpms.return_value = []`
		`- self.exports.getBuildTarget.return_value = self.targetinfo`
		`- self.exports.getTag.return_value = self.taginfo`
		`- self.exports.getRepo.return_value = None`
		`-`
		`- with self.assertRaises(koji.PreBuildError) as cm:`
		`- self.exports.wrapperRPM(self.build, self.url, self.target)`
		`- self.assertEqual("no repo for tag: %s" % self.taginfo['name'], str(cm.exception))`
		`-`
		`def test_priority_without_admin(self):`
		`priority = -10`
		`self.context.opts.get.return_value = True`
		`@@ -100,3 +88,23 @@`
		`with self.assertRaises(koji.GenericError) as cm:`
		`self.exports.wrapperRPM(self.build, self.url, self.target, priority=priority)`
		`self.assertEqual("only admins may create high-priority tasks", str(cm.exception))`
		`+`
		`+ def test_repo_id_ignored(self):`
		`+ self.context.opts.get.return_value = True`
		`+ self.exports.getBuild.return_value = self.buildinfo`
		`+ self.exports.getBuildTarget.return_value = self.targetinfo`
		`+ self.list_rpms.return_value = []`
		`+ self.exports.getTag.return_value = self.taginfo`
		`+ self.exports.getRepo.return_value = self.repoinfo`
		`+ self.context.session.hasPerm.return_value = False`
		`+`
		`+ opts = {'repo_id': 'REPO', 'scratch': True}`
		`+ self.exports.wrapperRPM(self.build, self.url, self.target, opts=opts)`
		`+`
		`+ self.make_task.assert_called_once()`
		`+ taskargs = self.make_task.call_args.args[1]`
		`+ expect_opts = {'scratch': True} # repo_id filtered out`
		`+ self.assertEqual(taskargs[4], expect_opts)`
		`+`
		`+`
		`+ # the end`

mikem commented 7 months ago

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

tkopecek commented on line 13 of kojihub/kojihub.py 7 months ago

This is not clear to me - kojid part still use that option, so why it is remove here?

mikem commented 7 months ago

This is not clear to me - kojid part still use that option, so why it is remove here?

The reason is that the previous code unconditionally set opts['repo_id'], even if a value was already present. So previously, a 'repo_id' field passed into the call was ignored. It's a bit pedantic, but this preserves the api more accurately.

You're right that the repo_id option is still accepted by the task itself. Kojid does pass it when it makes subtasks, but that does not use this call.

I did debate this a bit. We can always drop the restriction later to add an "allow user to specify repo for wrapper rpm tasks" feature.

Edited 7 months ago by mikem