PR#2496: hub: extract cg_name from build in policy_get_cgs - koji

koji

#2496 hub: extract cg_name from build in policy_get_cgs

Closed 3 years ago by tkopecek. Opened 3 years ago by tkopecek.

tkopecek/koji issue2251 into master

hub: extract cg_name from build in policy_get_cgs

Tomas Kopecek • 3 years ago

53d57f2

hub/kojihub.py

file modified

+12

		`@@ -9277,6 +9277,18 @@`
		`cgs = [lookup_name('content_generator', cg, strict=True)`
		`for cg in data['cg_list']]`
		`return set(cgs)`
		`+`
		`+ if 'build' in data:`
		`+ if isinstance(data['build'], (str, int)):`
		`+ build = get_build(data['build'], strict=True)`
		`+ elif isinstance(data['build'], dict):`
		`+ build = data['build']`
		`+ else:`
		`+ raise koji.GenericError('Unexpected policy_data["build"] type %s' %`
		`+ type(data['build']))`
		`+ if 'cg_name' in build:`
		`+ return set([build['cg_name']])`
		`+`
		`# otherwise try buildroot data`
		`cgs = set()`
		`for br_id in policy_get_brs(data):`

tests/test_hub/test_policy_tests.py

file modified

+8 -1

		`@@ -128,7 +128,14 @@`
		`self.assertEqual(result, expect)`
		`self.list_rpms.assert_called_once_with(buildID=42)`
		`self.list_archives.assert_called_once_with(buildID=42)`
		`- self.get_build.assert_called_once_with('NVR', strict=True)`
		`+ self.get_build.assert_called_with('NVR', strict=True)`
		`+`
		`+ def test_policy_get_cg_from_build(self):`
		`+ self.get_build.return_value = {'id': 42, 'cg_name': 'cgname'}`
		`+ # let's see...`
		`+ result = kojihub.policy_get_cgs({'build': 'NVR'})`
		`+ self.assertEqual(result, set(['cgname']))`
		`+ self.get_build.assert_called_with('NVR', strict=True)`

		`def test_policy_get_cg_from_cgs(self):`
		`data = {`

tkopecek commented 3 years ago

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

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

3 years ago

julian8628 commented 3 years ago

:thumbsup:

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

build is undefined if data['build'] is not a string. Otherwise, :thumbsup:

1 new commit added

check types

3 years ago

1 new commit added

check types

3 years ago

rebased onto 9ac0652945fa1ed23d30363de84eac177acddef5

3 years ago

tkopecek commented 3 years ago

updated

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

it could be an int as build_id

rebased onto c5f35758f264abc2e5516638502b237af0dd779d

3 years ago

rebased onto 53d57f2

3 years ago

julian8628 commented 3 years ago

:thumbsup:

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done

3 years ago

mikem commented 3 years ago

Note that in light of #2511, this change is of questionable value at the moment.

mikem commented 3 years ago

Unless we can address #2511, likely we'll have to take an approach more like policy_get_brs where we examine the build components. This would be most in line with the policy at CG Import time.

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-done, testing-ready

3 years ago

tkopecek commented 3 years ago

Moved to 1.24 (with #2511) as it needs more work.

mikem commented 3 years ago

Unless we can address #2511, likely we'll have to take an approach more like policy_get_brs where we examine the build components. This would be most in line with the policy at CG Import time.

I think when I looked at this before, I must not have looked at enough diff context. The existing policy test does look at buildroot data and has for years.

Looking at build.cg_name is faster, but less reliable. When we added the field, we did not provide a mechanism to populate it for existing data, so there are old content generator builds where the only way to determine this is to look at the buildroots.

The current change overrides the buildroot check for such builds, even when cg_id is not set for the build. So looks like this will return {} in cases where the old code would return a more correct answer.

:thumbsdown:

tkopecek commented 3 years ago

It looks that we've solved most problems with other PRs. Let's close this one for now.

Pull-Request has been closed by tkopecek

3 years ago