PR#1000: Fix target handling in make_task - koji

koji

#1000 Fix target handling in make_task

Merged 5 years ago by mikem. Opened 5 years ago by tkopecek.

tkopecek/koji issue998 into master

Fix target handling in make_task

Tomas Kopecek • 5 years ago

3d9445c

hub/kojihub.py

file modified

+6 -6

		`@@ -546,16 +546,16 @@`
		`hastarget = True`
		`break`
		`if hastarget:`
		`- if target is None:`
		`- policy_data['target'] = None`
		`- elif isinstance(target, dict):`
		`+ if isinstance(target, dict):`
		`if 'name' not in target:`
		`- hastarget = False`
		`logger.warning("Bad build target parameter: %r", target)`
		`+ target = None`
		`else:`
		`target = target.get('name')`
		`- if hastarget:`
		`- policy_data['target'] = get_build_target(target, strict=True)['name']`
		`+ if target is None:`
		`+ policy_data['target'] = None`
		`+ else:`
		`+ policy_data['target'] = get_build_target(target, strict=True)['name']`
		`t_opts = params.get('opts', {})`
		`policy_data['scratch'] = t_opts.get('scratch', False)`

tkopecek commented 5 years ago

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

julian8628 commented on line 5 of hub/kojihub.py 5 years ago

None target means null-target, in koji-shadow it could be None.
I think if there's no target in parameters, target shouldn't be put in policy_data

franzh commented on line 6 of hub/kojihub.py 5 years ago

Wondering if hastarget is necessary, can we use if target instead?

tkopecek commented 5 years ago

Add test (note for me)

mikem commented 5 years ago

We should not add a target value where there is none defined. No target defined is different from an explicit target value of None. I think instead that we should have some of the tests handle this more gracefully.

BoolTest could default to false when the field is absent. Policies that care about the difference can use the has test to distinguish.
Similarly our other base tests that use fields (MatchTest and CompareTest) could return False if there is no field to match
We could add an explicit target test (subclass of MatchTest, similar to other such tests), that is friendlier than using the match test directly. (This part could maybe be deferred a little).

Edited 5 years ago by mikem

rebased onto 3d9445c

5 years ago

tkopecek commented 5 years ago

I've fixed the PR. @mikem Do we want to track those tests' improvements in separate issue?

mikem commented 5 years ago

Commit cd76114 fixes this pull-request