#2989 honour taginfo option in policy_get_build_tags
Merged 2 years ago by tkopecek. Opened 2 years ago by tkopecek.
tkopecek/koji issue2917a  into  master

file modified
+28 -17
@@ -3380,6 +3380,8 @@ 

      - maven_support :           maven support flag (boolean)

      - maven_include_all :       maven include all flag (boolean)

      - extra :   extra tag parameters (dictionary)

+     - query_event : return "event" parameter for current call

+                     if something was passed in

  

      If there is no tag matching the given tagInfo, and strict is False,

      return None.  If strict is True, raise a GenericError.
@@ -3442,6 +3444,8 @@ 

              raise koji.GenericError("No such tagInfo: %r" % tagInfo)

          return None

      result['extra'] = get_tag_extra(result, event, blocked=blocked)

+     if event is not None:

+         result['query_event'] = event

      return result

  

  
@@ -9579,26 +9583,33 @@ 

  

  def policy_get_build_tags(data, taginfo=False):

      """If taginfo is set, return list of taginfos, else list of names only"""

+     tags = {}

      if 'build_tag' in data:

-         return [get_tag(data['build_tag'], strict=True)['name']]

+         buildtag = get_tag(data['build_tag'], strict=True, event="auto")

+         tags[buildtag['name']] = buildtag

      elif 'build_tags' in data:

-         return [get_tag(t, strict=True)['name'] for t in data['build_tags']]

+         build_tags = [get_tag(t, strict=True, event="auto") for t in data['build_tags']]

+         for tag in build_tags:

+             tags[tag['name']] = tag

  

-     # see if we have a target

-     target = data.get('target')

-     if target:

-         target = get_build_target(target, strict=False)

+     if not tags:

+         # see if we have a target

+         target = data.get('target')

          if target:

-             return [target['build_tag_name']]

- 

-     # otherwise look at buildroots

-     tags = {}

-     for br_id in policy_get_brs(data):

-         if br_id is None:

-             tags[None] = None

-         else:

-             tinfo = get_buildroot(br_id, strict=True)

-             tags[tinfo['tag_name']] = tinfo

+             target = get_build_target(target, strict=False)

+             if target:

+                 tags[target['build_tag_name']] = get_tag(target['build_tag'], strict=True,

+                                                          event="auto")

+ 

+     if not tags:

+         # otherwise look at buildroots

+         for br_id in policy_get_brs(data):

+             if br_id is None:

+                 tags[None] = None

+             else:

+                 tinfo = get_buildroot(br_id, strict=True)

+                 tags[tinfo['tag_name']] = get_tag(tinfo['tag_name'], strict=True,

+                                                   event=tinfo['repo_create_event_id'])

  

      if taginfo:

          tags = tags.values()
@@ -9817,7 +9828,7 @@ 

                  # content generator buildroots might not have tag info

                  continue

  

-             for tag in readFullInheritance(tinfo['tag_id']):

+             for tag in readFullInheritance(tinfo['id'], event=tinfo.get('query_event')):

                  if multi_fnmatch(tag['name'], args):

                      return True

          # otherwise...

@@ -108,7 +108,10 @@ 

  

      def _fakebr(self, br_id, strict):

          self.assertEqual(strict, True)

-         return {'cg_name': self._cgname(br_id)}

+         return {

+             'cg_name': self._cgname(br_id),

+             'repo_create_event_id': 1234,

+         }

  

      def _cgname(self, br_id):

          if br_id is None:
@@ -176,7 +179,7 @@ 

          self.list_rpms.assert_not_called()

          self.list_archives.assert_not_called()

          self.get_buildroot.assert_not_called()

-         self.get_tag.assert_called_once_with('TAGINFO', strict=True)

+         self.get_tag.assert_called_once_with('TAGINFO', event='auto', strict=True)

  

      def test_build_tag_given_alt(self):

          obj = kojihub.BuildTagTest('buildtag foo*')
@@ -206,7 +209,10 @@ 

          self.get_buildroot.assert_not_called()

  

      def _fakebr(self, br_id, strict=None):

-         return {'tag_name': self._brtag(br_id)}

+         return {

+             'tag_name': self._brtag(br_id),

+             'repo_create_event_id': 123,

+         }

  

      def _brtag(self, br_id):

          if br_id == '':
@@ -228,7 +234,14 @@ 

          data = {'build': 'BUILDINFO'}

          self.assertFalse(obj.run(data))

  

-         self.get_tag.assert_not_called()

+         self.get_tag.assert_has_calls([

+             mock.call('a', event=123, strict=True),

+             mock.call('b', event=123, strict=True),

+             mock.call('c', event=123, strict=True),

+             mock.call('d', event=123, strict=True),

+             mock.call('not-foo-5', event=123, strict=True),

+             mock.call('foo-3-build', event=123, strict=True),

+         ], any_order=True)

  

      def test_build_tag_no_info(self):

          obj = kojihub.BuildTagTest('buildtag foo*')
@@ -287,35 +300,35 @@ 

      def test_no_inheritance(self):

          obj = kojihub.BuildTagInheritsFromTest('buildtag_inherits_from foo*')

          data = {}

-         self.policy_get_build_tags.return_value = [{'tag_id': 123, 'tag_name': 'foox'}]

+         self.policy_get_build_tags.return_value = [{'id': 123, 'tag_name': 'foox'}]

          self.readFullInheritance.return_value = []

          self.assertFalse(obj.run(data))

          self.policy_get_build_tags.assert_called_once_with(data, taginfo=True)

-         self.readFullInheritance.assert_called_once_with(123)

+         self.readFullInheritance.assert_called_once_with(123, event=None)

  

      def test_inheritance_pass(self):

          obj = kojihub.BuildTagInheritsFromTest('buildtag_inherits_from foo*')

          data = {}

-         self.policy_get_build_tags.return_value = [{'tag_id': 123, 'tag_name': 'wrong'}]

+         self.policy_get_build_tags.return_value = [{'id': 123, 'tag_name': 'wrong'}]

          self.readFullInheritance.return_value = [

              {'name': 'still-wrong'},

              {'name': 'foox'},

          ]

          self.assertTrue(obj.run(data))

          self.policy_get_build_tags.assert_called_once_with(data, taginfo=True)

-         self.readFullInheritance.assert_called_once_with(123)

+         self.readFullInheritance.assert_called_once_with(123, event=None)

  

      def test_inheritance_fail(self):

          obj = kojihub.BuildTagInheritsFromTest('buildtag_inherits_from foo*')

          data = {}

-         self.policy_get_build_tags.return_value = [{'tag_id': 123, 'tag_name': 'wrong'}]

+         self.policy_get_build_tags.return_value = [{'id': 123, 'tag_name': 'wrong'}]

          self.readFullInheritance.return_value = [

              {'name': 'still-wrong'},

              {'name': 'still-still-wrong'},

          ]

          self.assertFalse(obj.run(data))

          self.policy_get_build_tags.assert_called_once_with(data, taginfo=True)

-         self.readFullInheritance.assert_called_once_with(123)

+         self.readFullInheritance.assert_called_once_with(123, event=None)

  

  

  class TestBuildTypeTest(unittest.TestCase):

I'm wondering if it makes sense to use auto also for tag and fromtag tests. As after deletion there is no longer active record in tag_packages I would incline to not adding it there. @mikem @julian8628 ?

Agree, I think the target of tag and fromtag tests should always exist. It's different from the buildtag test

The part of this that is actually related to #2917 (passing event=auto) is not reflected in the description of the PR.

This won't fix the issue for the buildtag_inherits_from test, but I guess (?) there might be some cases where the buildtag test could fail on a deleted tag.

Looking at this code again, it appears we're still not completely honoring the taginfo flag. When we get the to the part where we look at buildroots, we currently do:

tinfo = get_buildroot(br_id, strict=True)
tags[tinfo['tag_name']] = tinfo

When means in the taginfo=True case, we're going to return a buildroot dict instead of a taginfo one.

rebased onto 5e9d398

2 years ago

2 new commits added

  • query inheritance from last known tag appearance
  • fix getting tag from buildroot
2 years ago

I've fixed the buildroot part. What remains is how exactly query inheritance for deleted tag. Does it make sense (as in last commit) to query inheritance from the last known state? Or should we look into the moment when build was done? I don't find the expected semantics in this case quite clear.

3 new commits added

  • query inheritance from last known tag appearance
  • fix getting tag from buildroot
  • honour taginfo option in policy_get_build_tags
2 years ago

Yeah, the event thing is kind of a mess. Consider the cases in policy_get_build_tags...

When policy data has an explicit build tag, then event="auto" seems correct.

When policy data has an explicit target, we're checking for that target at current event too. I guess that could also fail the same way, but I think this only happens when handling a current task (either in channel policy or a policy checked by the builder). Either way, we can reasonably expect the target to exist. That's a long way of saying that "auto" is reasonable here.

When we're looking up the buildroots, we should probably actually use the repo_create_event_id for that buildroot instead.

Side note: there's a lot of repetition in policy_get_build_tags. I wonder if we could clean that up a bit, but that's not a big priority.

Meanwhile in buildtag_inherits_from, we get our tag list from policy_get_build_tags. In principle I think, we want to query inheritance the same way that we queried the tag. That's probably going to be messy to propagate.

I don't think we need to add the "auto" option to readFullInheritance. If the tag came from an event=auto query, then we can just use event=tinfo.get('revoke_event') as you have here, but if the tag came from a specific event query, as I suggest for the buildroot case, then we probably want that same event. We could just stick an extra field in the taginfos returned by policy_get_build_tags to propagate it, or we could possibly even have get_tag record query_event field.

pretty please pagure-ci rebuild

2 years ago

1 new commit added

  • getTag returns query_event
2 years ago

The get_tag() change looks good.

With the changes to policy_get_build_tags, we're changing the priority of data sources. Previously, we relied on checking different cases and returning when we found a match. Now that we're not returning, we will always go through all the checks, but the last matching one will win instead of the first matching one.

It might not matter much, but it does seem like a "build_tags" field in the policy data should take precedence over target, which should similarly take precedence over digging into buildroots.

Perhaps just wrap each of these later cases in if not tags so they are only checked if we haven't found a match yet.

in BuildTagInheritsFromTest, we check for tinfo.get('query_event') == 'auto', but that field will never be "auto". In the auto case we're storing the specific chosen numeric event that was used. We can probably just simplify this to using the numeric query_event if it is there.

Also the if 'build_tag' in data case should not be a special return. The policy_get_build_tags function is only called two places and and both assume the result is a list of tags.

1 new commit added

  • fix query logic
2 years ago

1 new commit added

  • fix tests
2 years ago

Fixed, I wonder if original 'build_tag was ever used in any test.

Fixed, I wonder if original build_tag was ever used in any test

It's given in policy data by the build task when checking the build_from_srpm and
build_from_repo_id. OTOH, it looks like the build_tags case is not used, at least by Koji itself.

Looks like I added the build_tags case when the policy_get_build_tags logic was first split out for the volume policy work. Before that, the logic was directly in BuildTagTest.

Maybe an interim change in the volume policy work actually did set build_tags, or maybe this was just for completeness.

At any rate, current changes look good!

:thumbsup:

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

2 years ago

Commit 9a3f9d4 fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago