koji

Clone

#4396 channel defaults
Opened 2 months ago by mikem. Modified 8 days ago
mikem/koji channel-defaults  into  master

Merge
file modified
+33 -22 
@@ -605,7 +605,8 @@ 
 

          parent: the id of the parent task (creates a subtask)
 

          label: (subtasks only) the label of the subtask
 

          owner: the user_id that should own the task
 

-         channel: the channel to place the task in
 

+         channel: requested channel override
 

+         default_channel: default channel
 

          arch: the arch for the task
 

          priority: the priority of the task
 

          assign: a host_id to assign the task to
 
@@ -659,6 +660,12 @@ 
 

      for key in 'arch', 'parent', 'label', 'owner':
 

          policy_data[key] = opts[key]
 

      policy_data['user_id'] = opts['owner']
 

+     default_channel = 'default'
 

+     if 'default_channel' in opts:
 

+         default_channel = opts['default_channel']
 

+         if 'channel' not in opts and context.opts.get('DefaultChannelCompat'):
 

+             # in the compat case, treat explicit default as an override
 

+             opts['channel'] = default_channel
 

      if 'channel' in opts:
 

          policy_data['req_channel'] = opts['channel']
 

          channel_info = get_channel(opts['channel'])
 
@@ -673,8 +680,8 @@ 
 

      ruleset = context.policy.get('channel')
 

      result = ruleset.apply(policy_data)
 

      if result is None:
 

-         logger.warning('Channel policy returned no result, using default')
 

-         opts['channel_id'] = get_channel_id('default', strict=True)
 

+         logger.debug('Channel policy returned no result, using default')
 

+         opts['channel_id'] = get_channel_id(default_channel, strict=True)
 

      else:
 

          try:
 

              parts = result.split()
 
@@ -696,6 +703,9 @@ 
 

                                   ruleset.last_rule())
 

                      raise koji.GenericError("invalid channel policy")
 

                  opts['channel_id'] = req_channel_id
 

+             elif parts[0] == "default":
 

+                 # note this is different from "use default" if default_channel is passed
 

+                 opts['channel_id'] = get_channel_id(default_channel, strict=True)
 

              else:
 

                  logger.error("Invalid result from channel policy: %s", ruleset.last_rule())
 

                  raise koji.GenericError("invalid channel policy")
 
@@ -10859,7 +10869,7 @@ 
 

  

          return make_task('chainbuild', [srcs, target, opts], **taskOpts)
 

  

-     def mavenBuild(self, url, target, opts=None, priority=None, channel='maven'):
 

+     def mavenBuild(self, url, target, opts=None, priority=None, channel=None):
 

          """Create a Maven build task
 

  

          url: The url to checkout the source from.  May be a CVS, SVN, or GIT repository.
 
@@ -10867,7 +10877,7 @@ 
 

          priority: the amount to increase (or decrease) the task priority, relative
 

                    to the default priority; higher values mean lower priority; only
 

                    admins have the right to specify a negative priority here
 

-         channel: the channel to allocate the task to (defaults to the "maven" channel)
 

+         channel: override the channel to allocate the task to
 

  

          Returns the task ID
 

          """
 
@@ -10877,7 +10887,7 @@ 
 

          convert_value(url, cast=str, check_only=True)
 

          if not opts:
 

              opts = {}
 

-         taskOpts = {}
 

+         taskOpts = {'default_channel': 'maven'}
 

          if priority:
 

              if priority < 0:
 

                  if not context.session.hasPerm('admin'):
 
@@ -10888,7 +10898,7 @@ 
 

  

          return make_task('maven', [url, target, opts], **taskOpts)
 

  

-     def wrapperRPM(self, build, url, target, priority=None, channel='maven', opts=None):
 

+     def wrapperRPM(self, build, url, target, priority=None, channel=None, opts=None):
 

          """Create a top-level wrapperRPM task
 

  

          build: The build to generate wrapper rpms for.  Must be in the COMPLETE state and have no
 
@@ -10900,7 +10910,7 @@ 
 

          priority: the amount to increase (or decrease) the task priority, relative
 

                    to the default priority; higher values mean lower priority; only
 

                    admins have the right to specify a negative priority here
 

-         channel: the channel to allocate the task to (defaults to the "maven" channel)
 

+         channel: override the channel to allocate the task to
 

  

          returns the task ID
 

          """
 
@@ -10923,18 +10933,19 @@ 
 

              logger.warning('The wrapperRPM call ignores repo_id options')
 

              del opts['repo_id']
 

  

-         taskOpts = {}
 

+         taskOpts = {'default_channel': 'maven'}
 

          if priority:
 

              if priority < 0:
 

                  if not context.session.hasPerm('admin'):
 

                      raise koji.ActionNotAllowed('only admins may create high-priority tasks')
 

              taskOpts['priority'] = koji.PRIO_DEFAULT + priority
 

-         convert_value(channel, cast=str, check_only=True)
 

-         taskOpts['channel'] = channel
 

+         if channel is not None:
 

+             convert_value(channel, cast=str, check_only=True)
 

+             taskOpts['channel'] = channel
 

  

          return make_task('wrapperRPM', [url, build_target, build, None, opts], **taskOpts)
 

  

-     def chainMaven(self, builds, target, opts=None, priority=None, channel='maven'):
 

+     def chainMaven(self, builds, target, opts=None, priority=None, channel=None):
 

          """Create a Maven chain-build task
 

  

          builds: a list of maps defining the parameters for the sequence of builds
 
@@ -10942,7 +10953,7 @@ 
 

          priority: the amount to increase (or decrease) the task priority, relative
 

                    to the default priority; higher values mean lower priority; only
 

                    admins have the right to specify a negative priority here
 

-         channel: the channel to allocate the task to (defaults to the "maven" channel)
 

+         channel: override the channel to allocate the task to
 

  

          Returns the task ID
 

          """
 
@@ -10950,7 +10961,7 @@ 
 

          if not context.opts.get('EnableMaven'):
 

              raise koji.GenericError("Maven support not enabled")
 

          convert_value(builds, cast=dict, check_only=True)
 

-         taskOpts = {}
 

+         taskOpts = {'default_channel': 'maven'}
 

          if priority:
 

              if priority < 0:
 

                  if not context.session.hasPerm('admin'):
 
@@ -10961,7 +10972,7 @@ 
 

  

          return make_task('chainmaven', [builds, target, opts], **taskOpts)
 

  

-     def winBuild(self, vm, url, target, opts=None, priority=None, channel='vm'):
 

+     def winBuild(self, vm, url, target, opts=None, priority=None, channel=None):
 

          """
 

          Create a Windows build task
 

  
@@ -10972,7 +10983,7 @@ 
 

          priority: the amount to increase (or decrease) the task priority, relative
 

                    to the default priority; higher values mean lower priority; only
 

                    admins have the right to specify a negative priority here
 

-         channel: the channel to allocate the task to (defaults to the "vm" channel)
 

+         channel: override the channel to allocate the task to
 

  

          Returns the task ID
 

          """
 
@@ -10987,7 +10998,7 @@ 
 

          assert_policy('vm', policy_data)
 

          if not opts:
 

              opts = {}
 

-         taskOpts = {}
 

+         taskOpts = {'default_channel': 'vm'}
 

          if priority:
 

              if priority < 0:
 

                  if not context.session.hasPerm('admin'):
 
@@ -11012,7 +11023,7 @@ 
 

  

          context.session.assertPerm(img_type)
 

  

-         taskOpts = {'channel': img_type}
 

+         taskOpts = {'default_channel': img_type}
 

          if img_type == 'livemedia':
 

              taskOpts['arch'] = 'noarch'
 

          else:
 
@@ -11034,7 +11045,7 @@ 
 

          Create an image using two other images and an indirection template
 

          """
 

          context.session.assertPerm('image')
 

-         taskOpts = {'channel': 'image'}
 

+         taskOpts = {'default_channel': 'image'}
 

          if priority:
 

              if priority < 0:
 

                  if not context.session.hasPerm('admin'):
 
@@ -11059,7 +11070,7 @@ 
 

          for i in [name, inst_tree, version]:
 

              convert_value(i, cast=str, check_only=True)
 

          context.session.assertPerm('image')
 

-         taskOpts = {'channel': 'image'}
 

+         taskOpts = {'default_channel': 'image'}
 

          if priority:
 

              if priority < 0:
 

                  if not context.session.hasPerm('admin'):
 
@@ -13678,7 +13689,7 @@ 
 

                  logger.debug("Cancelling distRepo task %d" % task_id)
 

                  Task(task_id).cancel(recurse=True)
 

          return make_task('distRepo', [tag, repo_id, keys, task_opts],
 

-                          priority=15, channel='createrepo')
 

+                          priority=15, default_channel='createrepo')
 

  

      def newRepo(self, tag, event=None, src=False, debuginfo=False, separate_src=False):
 

          """Create a newRepo task. returns task id"""
 
@@ -13706,7 +13717,7 @@ 
 

          if debuginfo:
 

              opts['debuginfo'] = True
 

          args = koji.encode_args(tag, **opts)
 

-         return make_task('newRepo', args, priority=15, channel='createrepo')
 

+         return make_task('newRepo', args, priority=15, default_channel='createrepo')
 

  

      def repoExpire(self, repo_id):
 

          """mark repo expired"""

file modified
+3 -1 
@@ -472,6 +472,8 @@ 
 

      ['EnableMaven', 'boolean', False],
 

      ['EnableWin', 'boolean', False],
 

  

+     ['DefaultChannelCompat', 'boolean', True],
 

+ 

      ['RLIMIT_AS', 'string', None],
 

      ['RLIMIT_CORE', 'string', None],
 

      ['RLIMIT_CPU', 'string', None],
 
@@ -630,7 +632,7 @@ 
 

      'channel': '''
 

              has req_channel :: req
 

              is_child_task :: parent
 

-             all :: use default
 

+             all :: default
 

              ''',
 

      'vm': '''
 

              has_perm admin win-admin :: allow

file modified
+4 -4 
@@ -5719,7 +5719,7 @@ 
 

                  },
 

                  {
 

                      "name": "channel",
 

-                     "default": "'maven'"
 

+                     "default": "None"
 

                  }
 

              ],
 

              "varargs": null,
 
@@ -9363,7 +9363,7 @@ 
 

                  },
 

                  {
 

                      "name": "channel",
 

-                     "default": "'maven'"
 

+                     "default": "None"
 

                  }
 

              ],
 

              "varargs": null,
 
@@ -10312,7 +10312,7 @@ 
 

                  },
 

                  {
 

                      "name": "channel",
 

-                     "default": "'vm'"
 

+                     "default": "None"
 

                  }
 

              ],
 

              "varargs": null,
 
@@ -10342,7 +10342,7 @@ 
 

                  },
 

                  {
 

                      "name": "channel",
 

-                     "default": "'maven'"
 

+                     "default": "None"
 

                  },
 

                  {
 

                      "name": "opts",

file added
+133 
@@ -0,0 +1,133 @@ 
 

+ import unittest
 

+ 

+ from unittest import mock
 

+ 

+ from kojihub import kojihub, kojixmlrpc
 

+ 

+ QP = kojihub.QueryProcessor
 

+ IP = kojihub.InsertProcessor
 

+ 

+ 

+ class TestMakeTask(unittest.TestCase):
 

+     def getQuery(self, *args, **kwargs):
 

+         query = QP(*args, **kwargs)
 

+         query.execute = mock.MagicMock()
 

+         query.executeOne = mock.MagicMock()
 

+         self.queries.append(query)
 

+         return query
 

+ 

+     def getInsert(self, *args, **kwargs):
 

+         insert = IP(*args, **kwargs)
 

+         insert.execute = mock.MagicMock()
 

+         self.inserts.append(insert)
 

+         return insert
 

+ 

+     def setUp(self):
 

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

+         self.context_db = mock.patch('kojihub.db.context').start()
 

+         # self.get_user = mock.patch('kojihub.kojihub.get_user').start()
 

+         self.opts = {
 

+             'DefaultChannelCompat': True,
 

+             'policy': {
 

+                 'channel': kojixmlrpc._default_policies['channel'],
 

+                 'priority': kojixmlrpc._default_policies['priority'],
 

+             }
 

+         }
 

+         self.context.opts = self.opts
 

+ 

+         self._dml = mock.patch('kojihub.db._dml').start()
 

+         self.QueryProcessor = mock.patch('kojihub.kojihub.QueryProcessor',
 

+                                          side_effect=self.getQuery).start()
 

+         self.queries = []
 

+         self.InsertProcessor = mock.patch('kojihub.kojihub.InsertProcessor',
 

+                                           side_effect=self.getInsert).start()
 

+         self.inserts = []
 

+ 

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

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

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

+         self.auto_arch_refuse = mock.patch('kojihub.scheduler.auto_arch_refuse').start()
 

+ 

+         self.set_policy()
 

+ 

+     def tearDown(self):
 

+         mock.patch.stopall()
 

+ 

+     def set_policy(self, opts=None):
 

+         # set policy from options
 

+         if opts is None:
 

+             opts = self.context.opts
 

+         plugins = {}
 

+         policy = kojixmlrpc.get_policy(opts, plugins)
 

+         self.context.policy = policy or {}
 

+ 

+     def test_make_task_simple(self):
 

+         self.get_channel_id.return_value = 1
 

+ 

+         kojihub.make_task('something', [1, 2, 3])
 

+ 

+         self.get_channel_id.assert_called_once_with('default', strict=True)
 

+         self.assertEqual(len(self.inserts), 1)
 

+         expected = {'state': 0, 'method': 'something', 'parent': None, 'arch': 'noarch',
 

+                     'channel_id': 1}
 

+         for key in expected:
 

+             self.assertEqual(self.inserts[0].data[key], expected[key])
 

+ 

+     def test_make_task_default_channel_compat(self):
 

+         self.get_channel.return_value = {'name': 'testing', 'id': 23, 'enabled': True}
 

+         self.opts['DefaultChannelCompat'] = True
 

+         self.opts['policy']['channel'] = '''
 

+             has req_channel :: req
 

+             all :: use bad
 

+         '''
 

+         self.set_policy()
 

+ 

+         kojihub.make_task('something', [1, 2, 3], default_channel='testing')
 

+ 

+         # in compat mode we expect to hit the "req" policy result
 

+         self.get_channel.assert_called_once_with('testing')
 

+         self.get_channel_id.assert_not_called()
 

+         self.assertEqual(len(self.inserts), 1)
 

+         expected = {'state': 0, 'method': 'something', 'parent': None, 'arch': 'noarch',
 

+                     'channel_id': 23}
 

+         for key in expected:
 

+             self.assertEqual(self.inserts[0].data[key], expected[key])
 

+ 

+     def test_make_task_default_channel_nocompat(self):
 

+         self.get_channel_id.return_value = 23
 

+         self.opts['DefaultChannelCompat'] = False
 

+         self.opts['policy']['channel'] = '''
 

+             has req_channel :: use bad
 

+             all :: default
 

+         '''
 

+         self.set_policy()
 

+ 

+         kojihub.make_task('something', [1, 2, 3], default_channel='testing')
 

+ 

+         # without compat mode we expect to hit the "default" policy result
 

+         self.get_channel_id.assert_called_once_with('testing', strict=True)
 

+         self.get_channel.assert_not_called()
 

+         self.assertEqual(len(self.inserts), 1)
 

+         expected = {'state': 0, 'method': 'something', 'parent': None, 'arch': 'noarch',
 

+                     'channel_id': 23}
 

+         for key in expected:
 

+             self.assertEqual(self.inserts[0].data[key], expected[key])
 

+ 

+     def test_make_task_no_policy_result(self):
 

+         # if channel policy does not match, we should use the default
 

+         self.get_channel.return_value = {'name': 'testing', 'id': 23, 'enabled': True}
 

+         self.get_channel_id.return_value = 23
 

+         self.opts['policy']['channel'] = ''
 

+         self.set_policy()
 

+ 

+         kojihub.make_task('something', [1, 2, 3], default_channel='testing')
 

+ 

+         self.get_channel_id.assert_called_once_with('testing', strict=True)
 

+         self.get_channel.assert_called_once_with('testing')
 

+         self.assertEqual(len(self.inserts), 1)
 

+         expected = {'state': 0, 'method': 'something', 'parent': None, 'arch': 'noarch',
 

+                     'channel_id': 23}
 

+         for key in expected:
 

+             self.assertEqual(self.inserts[0].data[key], expected[key])
 

+ 

+ # the end

Currently, we use the channel param to make_task() as both a user override mechanism a way for the call handler to adjust the default.

Channel is always ultimately determined by policy, but the policy cannot distinguish between the user passing a channel override for a maven task and the call handler indicating that the default should be "maven" instead of "default".

This PR adds a separate mechanism for specifying the default and alters call handlers to use that instead (but still use channel for user overrides).

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

Unfortunately this is not quite backwards compatible. There's the (minor) api change for the default values for some calls, but more importantly having these calls pass default_channel=foo instead of channel=foo could easily yield different results in the channel policy. Granted, this is the point, but the results could be surprising to admins.

So, marking this as a draft for now. I'm not sure what the best way forward is. Perhaps it would help to add some sort of compat config option that would cause the hub to still treat these defaults as overrides.
Edited 8 days ago by mikem

2 new commits added

  • add unit tests
  • compat option for previous behavior
2 months ago

1 new commit added

  • empty commit for testing
16 days ago

4 new commits added

  • add unit tests
  • compat option for previous behavior
  • update api data for new defaults
  • separate channel default from overrides
16 days ago

rebased onto 2df628b
16 days ago
Metadata
None
No Tags
Changes Summary 4
+33 -22
file changed
kojihub/kojihub.py
+3 -1
file changed
kojihub/kojixmlrpc.py
+4 -4
file changed
tests/test_api/data/api.json
+133
file added
tests/test_hub/test_make_task.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.