#3729 Build image from uploaded kickstart
Merged 10 months ago by tkopecek. Opened a year ago by tkopecek.
tkopecek/koji issue3183  into  master

file modified
+18 -1
@@ -2563,6 +2563,23 @@ 

                  "imagefactory, oz and possibly python-hashlib")

              raise koji.ApplianceError('ImageFactory functions not available')

  

+         # Policy check

+         task_info = self.session.getTaskInfo(self.id)

+         policy_data = {

+             'user_id': task_info['owner'],

+             'source': opts.get('ksurl'),

+             'task_id': self.id,

+             'build_tag': build_tag,  # id

+             'skip_tag': bool(self.opts.get('skip_tag')),

+             'scratch': bool(opts.get('scratch')),

+             'from_scm': False,

+             'repo_id': opts.get('repo_id'),

+             'target': target_info['name'],

+         }

+         if not self.opts.get('skip_tag'):

+             policy_data['tag'] = target_info['dest_tag']  # id

+         self.session.host.assertPolicy('build_rpm', policy_data)

+ 

          # build image(s)

          bld_info = None

          try:
@@ -2635,7 +2652,7 @@ 

                  for arch in results:

                      if arch in ignored_arches:

                          continue

-                     ks = os.path.basename(opts.get('kickstart'))

+                     ks = os.path.basename(opts['kickstart'])

                      if ks in results[arch]['files']:

                          if saw_ks:

                              results[arch]['files'].remove(ks)

@@ -6004,9 +6004,6 @@ 

      # Upload the KS file to the staging area.

      # If it's a URL, it's kojid's job to go get it when it does the checkout.

      if not task_opts.ksurl:

-         if not task_opts.scratch:

-             # only scratch builds can omit ksurl

-             raise koji.GenericError("Non-scratch builds must provide ksurl")

          ksfile = task_opts.kickstart

          serverdir = unique_path('cli-image')

          session.uploadWrapper(ksfile, serverdir, callback=callback)

file modified
-2
@@ -10322,8 +10322,6 @@ 

                          'only admins may create high-priority tasks')

  

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

-         if 'scratch' not in opts and 'ksurl' not in opts:

-             raise koji.ActionNotAllowed('Non-scratch builds must provide ksurl')

  

          return make_task('image', [name, version, arches, target, inst_tree, opts], **taskOpts)

  

@@ -185,6 +185,36 @@ 

              '/path/to/cli-image',

              callback=None)

  

+     def test_build_image_oz_local_ks(self):

+         task_id = 107

+         # self.task_options.kickstart will be

+         # changed in _build_image_oz()

+         ksfile = self.task_options.kickstart

+         self.task_options.ksurl = None

+         self.task_options.scratch = False

+ 

+         self.session.getBuildTarget.return_value = self.target_info

+         self.session.getTag.return_value = self.tag_info

+         self.session.buildImageOz.return_value = task_id

+ 

+         self.task_options.background = True

+         self.running_in_bg.return_value = True

+         with mock.patch('sys.stdout', new_callable=six.StringIO) as stdout:

+             _build_image_oz(

+                 self.options, self.task_options, self.session, self.args)

+         expected = '' + '\n'

+         expected += "Created task: %d" % task_id + "\n"

+         expected += "Task info: %s/taskinfo?taskID=%s" % \

+                     (self.options.weburl, task_id) + "\n"

+         self.assert_console_message(stdout, expected)

+         self.watch_tasks.assert_not_called()

+         self.session.buildImageOz.assert_called_once()

+         self.unique_path.assert_called_with('cli-image')

+         self.session.uploadWrapper.assert_called_with(

+             ksfile,

+             '/path/to/cli-image',

+             callback=None)

+ 

      def test_build_image_oz_exception(self):

          self.session.getBuildTarget.return_value = {}

          with self.assertRaises(koji.GenericError) as cm:
@@ -198,13 +228,6 @@ 

          self.assertEqual(str(cm.exception),

                           'No such destination tag: %s' % self.target_info['dest_tag_name'])

  

-         self.session.getTag.return_value = self.tag_info

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

-             self.task_options.ksurl = None

-             self.task_options.scratch = False

-             _build_image_oz(self.options, self.task_options, self.session, self.args)

-         self.assertEqual(str(cm.exception), 'Non-scratch builds must provide ksurl')

- 

  

  class TestImageBuild(utils.CliTestCase):

      def setUp(self):

@@ -50,12 +50,3 @@ 

              self.exports.buildImageOz(self.name, self.version, self.arches, self.target,

                                        self.inst_tree, priority=priority)

          self.assertEqual("only admins may create high-priority tasks", str(cm.exception))

- 

-     def test_opts_without_expected_keys(self):

-         priority = 10

-         opts = {}

-         self.context.session.assertPerm.side_effect = None

-         with self.assertRaises(koji.ActionNotAllowed) as cm:

-             self.exports.buildImageOz(self.name, self.version, self.arches, self.target,

-                                       self.inst_tree, opts=opts, priority=priority)

-         self.assertEqual("Non-scratch builds must provide ksurl", str(cm.exception))

1 new commit added

  • fix unit tests
a year ago

Checking the build_rpm policy for a non-rpm build is highly counter-intuitive

Any better idea? Creating new policy doing the same seems like an overengineering. build_rpm should have been more general (aka build), but changing it now is quite late :-(

I added your proposed patch in our (CERN Linux team) koji installation and it runs in our QA cluster the last 2 weeks , with no side effects.
I also tested it and it indeed covers our case, as we can now build images from locally stored kickstart files.
So, from a technical perspective, we are good.

As for the policy, I really cannot think of something that is not overengineering or a big change. Nevertheless, isn't 'cg_import' a policy that is the same like 'build_rpm'?
If there are already 2 policies with different name, but same content, why not a third one? e.g. 'build_img'

Dear koji developers, is there any progress on this issue?

We'll need some followup, at least for docs, but we can merge this for now.

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

10 months ago

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

10 months ago

Commit 4ce5fc1 fixes this pull-request

Pull-Request has been merged by tkopecek

10 months ago