#1797 hub: build for policy check should be build_id in host.tagBuild
Merged 2 years ago by tkopecek. Opened 2 years ago by julian8628.
julian8628/koji typo_tag_build  into  master

file modified
+8 -5
@@ -9080,15 +9080,18 @@ 

      True if any of them lack a buildroot (strict)"""

      name = 'imported'

      def run(self, data):

-         rpms = context.handlers.call('listRPMs', buildID=data['build'])

-         #no test args

-         for rpminfo in rpms:

+         build_info = data.get('build')

+         if not build_info:

+             raise koji.GenericError('policy data must contain a build')

+         build_id = get_build(build_info, strict=True)['id']

+         # no test args

+         for rpminfo in list_rpms(buildID=build_id):

              if rpminfo['buildroot_id'] is None:

                  return True

-         for archive in list_archives(buildID=data['build']):

+         for archive in list_archives(buildID=build_id):

              if archive['buildroot_id'] is None:

                  return True

-         #otherwise...

+         # otherwise...

          return False

  

  class ChildTaskTest(koji.policy.BoolTest):

@@ -296,3 +296,66 @@ 

          data = {'build': 123, 'btypes': set(['rpm'])}

          self.assertTrue(obj.run(data))

          self.get_build_type.assert_not_called()

+ 

+ 

+ class TestImportedTest(unittest.TestCase):

+     def setUp(self):

+         self.list_rpms = mock.patch('kojihub.list_rpms').start()

+         self.list_archives = mock.patch('kojihub.list_archives').start()

+         self.get_build = mock.patch('kojihub.get_build').start()

+ 

+     def tearDown(self):

+         mock.patch.stopall()

+ 

+     def test_no_build(self):

+         self.get_build.side_effect = koji.GenericError

+         obj = kojihub.ImportedTest('imported - no build')

+         data = {}

+         with self.assertRaises(koji.GenericError) as cm:

+             obj.run(data)

+         self.assertEqual(cm.exception.args[0],

+                          'policy data must contain a build')

+         self.get_build.assert_not_called()

+ 

+     def test_invalid_build(self):

+         self.get_build.side_effect = koji.GenericError

+         obj = kojihub.ImportedTest('imported - invalid build')

+         data = {'build': 'nvr-1-1'}

+         with self.assertRaises(koji.GenericError):

+             obj.run(data)

+         self.get_build.assert_called_once_with('nvr-1-1', strict=True)

+ 

+     def test_imported_rpm(self):

+         binfo = {'id': 1, 'name': 'nvr-1-1'}

+         self.get_build.return_value = binfo

+         self.list_rpms.return_value = [{'id': 1, 'buildroot_id': None}]

+         obj = kojihub.ImportedTest('imported - imported rpm')

+         data = {'build': 'nvr-1-1'}

+         self.assertTrue(obj.run(data))

+         self.get_build.assert_called_once_with('nvr-1-1', strict=True)

+         self.list_rpms.assert_called_once_with(buildID=1)

+         self.list_archives.assert_not_called()

+ 

+     def test_imported_archive(self):

+         binfo = {'id': 1, 'name': 'nvr-1-1'}

+         self.get_build.return_value = binfo

+         self.list_rpms.return_value = [{'id': 1, 'buildroot_id': 1}]

+         self.list_archives.return_value = [{'id': 1, 'buildroot_id': None}]

+         obj = kojihub.ImportedTest('imported - imported archive')

+         data = {'build': 'nvr-1-1'}

+         self.assertTrue(obj.run(data))

+         self.get_build.assert_called_once_with('nvr-1-1', strict=True)

+         self.list_rpms.assert_called_once_with(buildID=1)

+         self.list_archives.assert_called_once_with(buildID=1)

+ 

+     def test_false(self):

+         binfo = {'id': 1, 'name': 'nvr-1-1'}

+         self.get_build.return_value = binfo

+         self.list_rpms.return_value = [{'id': 1, 'buildroot_id': 1}]

+         self.list_archives.return_value = [{'id': 1, 'buildroot_id': 2}]

+         obj = kojihub.ImportedTest('imported - false test')

+         data = {'build': 'nvr-1-1'}

+         self.assertFalse(obj.run(data))

+         self.get_build.assert_called_once_with('nvr-1-1', strict=True)

+         self.list_rpms.assert_called_once_with(buildID=1)

+         self.list_archives.assert_called_once_with(buildID=1)

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

2 years ago

I think we're addressing the wrong problem here.

It is true that #1702 changed the type of the build field in the policy data provided in host.tagBuild from the build id to the dict. However, there are other places in the code where the build field in the policy data is given as such (e.g. for places where we check volume policy).

All the other policy test handlers that look at the build field end up passing through get_build first (or passing it to another call that does so). The imported test is the only one that assumes it must be an integer. We should probably fix this test instead.

Based on my above comments, I'd expect similar errors if someone used the imported test in volume policy.

@mikem does an invalid data['build'] or absence of data['build'] make sense for imported test?
I think an exception should be raised in this case.

rebased onto 03564b6

2 years ago

Based on my above comments, I'd expect similar errors if someone used the imported test in volume policy.

updated

:thumbsup:

I think, that raising an exception makes a sense here. imported is expected to return boolean, if there is no build, it is wrong usage and returning False/True would hide a problem and is definitely not true value.

Commit a365c9a fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago

I've merged it, so we can proceed with fix. @mikem if you've concerns about exception, we can open new issue for that.