PR#4125: partial draft support for cg_import

Merges #4125
https://pagure.io/koji/pull-request/4125

Relates: #4124
https://pagure.io/koji/issue/4124
Support draft builds for CGs

cli/koji_cli/commands.py

@@ -1474,15 +1474,25 @@ def handle_import_cg(goptions, session, args):
                       help="Attempt to hardlink instead of uploading")
     parser.add_option("--test", action="store_true", help="Don't actually import")
     parser.add_option("--token", action="store", default=None, help="Build reservation token")
+    parser.add_option("--draft", action="store_true", default=False, help="Import as a draft")
+    parser.add_option("--build-id", action="store", type="int", default=None,
+                      help="Reserved build id")
     (options, args) = parser.parse_args(args)
     if len(args) < 2:
         parser.error("Please specify metadata files directory")
     activate_session(session, goptions)
+
+    # get the metadata
     metadata = koji.load_json(args[0])
+    if options.build_id:
+        metadata['build']['build_id'] = options.build_id
+    if options.draft:
+        metadata['build']['draft'] = True
+
+    # prepare output list for upload
     if 'output' not in metadata:
         error("Metadata contains no output")
     localdir = args[1]
-
     to_upload = []
     for info in metadata['output']:
         if info.get('metadata_only', False):
@@ -1495,10 +1505,9 @@ def handle_import_cg(goptions, session, args):
     if options.test:
         return
 
-    # get upload path
+    # upload our outputs
-    # XXX - need a better way
+    # TODO - need a better way to select upload path
     serverdir = unique_path('cli-import')
-
     for localpath, info in to_upload:
         relpath = os.path.join(serverdir, info.get('relpath', ''))
         if _running_in_bg() or options.noprogress:
@@ -1516,6 +1525,58 @@ def handle_import_cg(goptions, session, args):
     session.CGImport(metadata, serverdir, options.token)
 
 
+def handle_reserve_cg(goptions, session, args):
+    "[admin] Reserve a build entry for later import"
+    usage = "usage: %prog reserve-cg [options] <cg> <nvr>|<metadata_file>"
+    parser = OptionParser(usage=get_usage_str(usage))
+    parser.add_option("--draft", action="store_true", default=False, help="Reserve a draft entry")
+    parser.add_option("--epoch", type="int", help="Specify epoch for nvr argument")
+    parser.add_option("--metadata", action="store_true", default=False,
+                      help="Treat argument as a metadata file")
+    (options, args) = parser.parse_args(args)
+    if len(args) != 2:
+        parser.error("The command takes exactly two arguments")
+    activate_session(session, goptions)
+
+    cg = args[0]
+
+    def read_nvr():
+        try:
+            data = koji.parse_NVR(args[1])
+        except Exception:
+            return None
+        # fix epoch
+        if options.epoch is not None:
+            data['epoch'] = options.epoch
+        elif data['epoch'] == '':
+            data['epoch'] = None
+        else:
+            data['epoch'] = int(data['epoch'])
+        return data
+
+    def read_metadata():
+        if options.epoch is not None:
+            parser.error('The --epoch option cannot be used with --metadata')
+        metadata = koji.load_json(args[1])
+        data = metadata['build']
+        fields = ('name', 'version', 'release', 'epoch', 'draft')
+        data = koji.util.dslice(data, fields, strict=False)
+        return data
+
+    if options.metadata:
+        data = read_metadata()
+    else:
+        data = read_nvr()
+        if not data:
+            data = read_metadata()
+
+    if options.draft:
+        data['draft'] = True
+
+    result = session.CGInitBuild(cg, data)
+    print('Reserved build %(build_id)s with token %(token)r' % result)
+
+
 def handle_import_comps(goptions, session, args):
     "Import group/package information from a comps file"
     usage = "usage: %prog import-comps [options] <file> <tag>"

docs/source/content_generator_metadata.rst

@@ -46,6 +46,7 @@ The build map contains the following entries:
    is optional.
 -  build_id: Reserved build ID. This field is optional.
 -  task_id: In case there is a corresponding task in koji (int/None)
+-  draft: True if the build should be imported as a draft. Optional.
 -  extra: A map of extra metadata associated with the build, which
    must include at least one of:
 

kojihub/kojihub.py

@@ -6794,7 +6794,6 @@ def cg_init_build(cg, data):
                       other values will be ignored anyway (owner, state, ...)
     :return: dict with build_id and token
     """
-    reject_draft(data)
     assert_cg(cg)
     cg_id = lookup_name('content_generator', cg, strict=True)['id']
     data['owner'] = context.session.user_id
@@ -6994,6 +6993,7 @@ class CG_Importer(object):
             'release': self.buildinfo['release'],
             'btypes': list(self.typeinfo),
             'source': self.buildinfo.get('source'),
+            'draft': self.buildinfo.get('draft'),
             'metadata_only': self.metadata_only,
             'cg_list': [self.cg],
             # TODO: provide more data
@@ -7058,9 +7058,19 @@ class CG_Importer(object):
                 raise koji.GenericError('Build is owned by task %(task_id)s' % buildinfo)
             if buildinfo['state'] != koji.BUILD_STATES['BUILDING']:
                 raise koji.GenericError('Build ID %s is not in BUILDING state' % build_id)
-            if buildinfo['name'] != metadata['build']['name'] or \
+            if buildinfo['draft'] != metadata['build'].get('draft', False):
-               buildinfo['version'] != metadata['build']['version'] or \
+                raise koji.GenericError("Draft field does not match reservation (build id = %s)"
-               buildinfo['release'] != metadata['build']['release']:
+                                        % build_id)
+            if (buildinfo['name'] != metadata['build']['name'] or
+                    buildinfo['version'] != metadata['build']['version']):
+                raise koji.GenericError("Build (%i) NVR is different" % build_id)
+            # release is complicated by draft field
+            if buildinfo['draft']:
+                # metadata should have the target release, convert it before we check
+                release = koji.gen_draft_release(metadata['build']['release'], build_id)
+            else:
+                release = metadata['build']['release']
+            if buildinfo['release'] != release:
                 raise koji.GenericError("Build (%i) NVR is different" % build_id)
             if ('epoch' in metadata['build'] and
                     buildinfo['epoch'] != metadata['build']['epoch']):
@@ -7070,16 +7080,16 @@ class CG_Importer(object):
         elif token is not None:
             raise koji.GenericError('Reservation token given, but no build_id '
                                     'in metadata')
+
         else:
             # no build reservation
             buildinfo = get_build(metadata['build'], strict=False)
             if buildinfo:
                 if (koji.BUILD_STATES[buildinfo['state']] not in ('CANCELED', 'FAILED')):
-                    raise koji.GenericError("Build already exists: %r" % buildinfo)
+                    raise koji.GenericError("Build already exists: %(nvr)s (id=%(id)s)"
+                                            % buildinfo)
                 # note: the checks in recycle_build will also apply when we call new_build later
-
+                # our state check is stricter than the one in recycle_build
-        if buildinfo:
-            reject_draft(buildinfo)
 
         # gather needed data
         buildinfo = dslice(metadata['build'], ['name', 'version', 'release', 'extra', 'source'])
@@ -7087,6 +7097,7 @@ class CG_Importer(object):
             buildinfo['build_id'] = metadata['build']['build_id']
         # epoch is not in the metadata spec, but we allow it to be specified
         buildinfo['epoch'] = metadata['build'].get('epoch', None)
+        buildinfo['draft'] = metadata['build'].get('draft', False)
         buildinfo['start_time'] = convert_timestamp(float(metadata['build']['start_time']))
         buildinfo['completion_time'] = convert_timestamp(float(metadata['build']['end_time']))
         owner = metadata['build'].get('owner', None)

tests/test_cli/data/list-commands-admin.txt

@@ -55,6 +55,7 @@ admin commands:
         remove-tag                Remove a tag
         remove-tag-inheritance    Remove a tag inheritance link
         remove-target             Remove a build target
+        reserve-cg                Reserve a build entry for later import
         restart-hosts             Restart enabled hosts
         revoke-cg-access          Remove a user from a content generator
         revoke-permission         Revoke a permission from a user

tests/test_cli/data/list-commands.txt

@@ -55,6 +55,7 @@ admin commands:
         remove-tag                Remove a tag
         remove-tag-inheritance    Remove a tag inheritance link
         remove-target             Remove a build target
+        reserve-cg                Reserve a build entry for later import
         restart-hosts             Restart enabled hosts
         revoke-cg-access          Remove a user from a content generator
         revoke-permission         Revoke a permission from a user

tests/test_cli/test_import_cg.py

@@ -1,35 +1,30 @@
 from __future__ import absolute_import
+import json
 import os
-import unittest
+import shutil
+import tempfile
 try:
     from unittest import mock
-    from unittest.mock import call
 except ImportError:
     import mock
-    from mock import call
 
-import six
+import koji
-
+from koji_cli.commands import handle_import_cg, _progress_callback
-from koji_cli.commands import handle_import_cg
 from . import utils
 
 
 class TestImportCG(utils.CliTestCase):
-    def mock_os_path_exists(self, filepath):
-        if filepath in self.custom_os_path_exists:
-            return self.custom_os_path_exists[filepath]
-        return self.os_path_exists(filepath)
 
     def setUp(self):
         self.maxDiff = None
         self.options = mock.MagicMock()
         self.session = mock.MagicMock()
-        self.custom_os_path_exists = {}
+        self.workdir = tempfile.mkdtemp()
-        self.os_path_exists = os.path.exists
+        self.outdir = self.workdir + '/output'
         self.unique_path_mock = mock.patch('koji_cli.commands.unique_path').start()
         self.running_in_bg = mock.patch('koji_cli.commands._running_in_bg').start()
         self.running_in_bg.return_value = False
-        self.linked_upload_mock = mock.patch('koji_cli.commands.linked_upload').start()
+        self.linked_upload = mock.patch('koji_cli.commands.linked_upload').start()
         self.activate_session_mock = mock.patch('koji_cli.commands.activate_session').start()
         self.error_format = """Usage: %s import-cg [options] <metadata_file> <files_dir>
 (Specify the --help global option for a list of other help options)
@@ -39,143 +34,210 @@ class TestImportCG(utils.CliTestCase):
 
     def tearDown(self):
         mock.patch.stopall()
+        shutil.rmtree(self.workdir)
 
-    @mock.patch('sys.stdout', new_callable=six.StringIO)
+    def make_metadata(self, metadata=None, defaults=True, write_outputs=True):
-    @mock.patch('koji_cli.commands._progress_callback')
+        """Fill in metadata and write to workdir"""
-    @mock.patch('koji.json')
+        if metadata is None:
-    def test_handle_import_cg(self, json_mock, progress_callback_mock, stdout):
+            metadata = {}
+
+        if defaults:
+            build = metadata.setdefault('build', {})
+            build.setdefault('name', 'mypackage')
+            build.setdefault('version', '1')
+            build.setdefault('release', '2')
+
+            if 'output' not in metadata:
+                metadata['output'] = [
+                    {'relpath': 'relative/path', 'filename': 'file-1'},
+                    {'relpath': 'relative/path', 'filename': 'file-2'},
+                ]
+
+        # write metdata
+        fn = os.path.join(self.workdir, 'metadata.json')
+        with open(fn, 'wt') as fd:
+            json.dump(metadata, fd, indent=4)
+
+        if write_outputs:
+            self.write_outputs(metadata)
+
+        return metadata, fn
+
+    def write_outputs(self, metadata):
+        # write outputs
+        for info in metadata.get('output', []):
+            fdir = os.path.join(self.outdir, info.get('relpath', ''))
+            koji.ensuredir(fdir)
+            fn = os.path.join(fdir, info['filename'])
+            with open(fn, 'wt') as fd:
+                fd.write('fnord\n')
+
+    def test_handle_import_cg(self):
         """Test handle_import_cg function"""
-        arguments = ['fake-metafile', '/path/to/files/']
-        expected = ''
-        fake_srv_path = '/path/to/server/cli-import'
 
-        metadata = {
+        metadata, fn = self.make_metadata()
-            'output': [
+        arguments = [fn, self.outdir]
-                {'relpath': '/real/path', 'filename': 'file-1'},
-                {'relpath': '/real/path', 'filename': 'file-2'}
-            ]
-        }
-
-        #
-        # we just need to change original os.path.exists behavior, if the input
-        # is matched return the value we expected.
-        self.custom_os_path_exists = dict(('%(relpath)s/%(filename)s' % v, True)
-                                          for v in metadata['output'])
-
-        # setup and start os.path.exists patch
-        os_path_exists_patch = mock.patch('os.path.exists',
-                                          new=self.mock_os_path_exists)
-        os_path_exists_patch.start()
-
-        def gen_value(fmt, callback):
-            calls, expect = [], ''
-            for item in metadata['output']:
-                filepath = "%(relpath)s/%(filename)s" % item
-                calls.append(call(filepath,
-                                  item['relpath'],
-                                  callback=callback))
-                expect += fmt % filepath + "\n"
-            return calls, expect
-
-        json_mock.load.return_value = metadata
-        self.unique_path_mock.return_value = fake_srv_path
 
         # Case 1, running in fg, progress on
-        with mock.patch(utils.get_builtin_open()):
+        handle_import_cg(self.options, self.session, arguments)
-            handle_import_cg(self.options, self.session, arguments)
 
-        calls, expected = gen_value("Uploading %s\n", progress_callback_mock)
+        self.assertEqual(len(self.session.uploadWrapper.mock_calls), len(metadata['output']))
-        self.assert_console_message(stdout, expected)
+        kwargs = self.session.uploadWrapper.call_args.kwargs
-        self.linked_upload_mock.assert_not_called()
+        self.assertEqual(kwargs['callback'], _progress_callback)
-        self.session.uploadWrapper.assert_has_calls(calls)
+        self.session.CGImport.assert_called_once()
-        self.session.CGImport.assert_called_with(metadata, fake_srv_path, None)
+        args = self.session.CGImport.call_args.args
+        self.assertEqual(args[0], metadata)
+        self.linked_upload.assert_not_called()
 
-        # Case 2, running in fg, progress off
+    def test_handle_import_cg_nodir(self):
-        with mock.patch(utils.get_builtin_open()):
+        """Test handle_import_cg function"""
-            handle_import_cg(self.options, self.session, arguments + ['--noprogress'])
 
-        calls, expected = gen_value("Uploading %s", None)
+        metadata, fn = self.make_metadata(write_outputs=False)
-        self.assert_console_message(stdout, expected)
+        arguments = [fn]
-        self.linked_upload_mock.assert_not_called()
-        self.session.uploadWrapper.assert_has_calls(calls)
-        self.session.CGImport.assert_called_with(metadata, fake_srv_path, None)
 
-        # reset mocks
-        self.linked_upload_mock.reset_mock()
-        self.session.uploadWrapper.reset_mock()
-        self.session.CGImport.reset_mock()
-
-        # Case 3, --test option
-        with mock.patch(utils.get_builtin_open()):
-            handle_import_cg(self.options, self.session, arguments + ['--test'])
-
-        self.linked_upload_mock.assert_not_called()
-        self.session.uploadWrapper.assert_not_called()
-        self.session.CGImport.assert_not_called()
-
-        calls = [call("%(relpath)s/%(filename)s" % item, item['relpath'])
-                 for item in metadata['output']]
-
-        # Case 4, --link option
-        with mock.patch(utils.get_builtin_open()):
-            handle_import_cg(self.options, self.session, arguments + ['--link'])
-
-        self.linked_upload_mock.assert_has_calls(calls)
-        self.session.uploadWrapper.assert_not_called()
-        self.session.CGImport.assert_called_with(metadata, fake_srv_path, None)
-
-        # make sure there is no message on output
-        self.assert_console_message(stdout, '')
-
-        # stop os.path.exists patch
-        os_path_exists_patch.stop()
-
-    def test_handle_import_argument_test(self):
-        """Test handle_import_cg function without arguments"""
-        arguments = ['fake-metafile', '/path/to/files/']
-
-        # Case 1. empty argument
         self.assert_system_exit(
             handle_import_cg,
-            self.options, self.session, [],
+            self.options, self.session, arguments,
-            stderr=self.format_error_message("Please specify metadata files directory"),
+            stderr=self.format_error_message('Please specify metadata files directory'),
-            activate_session=None)
+            stdout='',
+            activate_session=None
+        )
 
-        # Case 2. JSON module does not exist
+        self.session.uploadWrapper.assert_not_called()
-        # dropped - it is now part of stdlib
+        self.session.CGImport.assert_not_called()
+        self.linked_upload.assert_not_called()
 
-        metadata = {
+    def test_handle_import_cg_output(self):
-            'output': [
+        """Test handle_import_cg function"""
-                {'metadata_only': True},
-                {'relpath': '/real/path', 'filename': 'filename'}
-            ]
-        }
 
-        #
+        metadata, fn = self.make_metadata({}, defaults=False)
-        # we just need to change original os.path.exists behavior, if the input
+        arguments = [fn, self.outdir]
-        # is matched return the value we expected.
-        self.custom_os_path_exists['/real/path/filename'] = False
 
-        with mock.patch(utils.get_builtin_open()):
+        self.assert_system_exit(
-            with mock.patch('os.path.exists', new=self.mock_os_path_exists):
+            handle_import_cg,
-                with mock.patch('koji.json') as json_mock:
+            self.options, self.session, arguments,
+            stderr='Metadata contains no output\n',
+            stdout='',
+            activate_session=None,
+            exit_code=1
+        )
 
-                    # Case 3. metafile doesn't have output section
+        self.session.uploadWrapper.assert_not_called()
-                    json_mock.load.return_value = {}
+        self.session.CGImport.assert_not_called()
-                    self.assert_system_exit(
+        self.linked_upload.assert_not_called()
-                        handle_import_cg,
-                        self.options, self.session, arguments,
-                        stderr="Metadata contains no output\n",
-                        exit_code=1)
 
-                    # Case 4. path not exist
+    def test_handle_import_cg_nofiles(self):
-                    file_path = '%(relpath)s/%(filename)s' % metadata['output'][1]
+        """Test handle_import_cg function"""
-                    json_mock.load.return_value = metadata
+
-                    self.assert_system_exit(
+        metadata, fn = self.make_metadata(write_outputs=False)
-                        handle_import_cg,
+        arguments = [fn, self.outdir]
-                        self.options, self.session, arguments,
+
-                        stderr=self.format_error_message("No such file: %s" % file_path),
+        expect = "No such file: %s" % os.path.join(self.outdir, 'relative/path/file-1')
-                        exit_code=2)
+        self.assert_system_exit(
+            handle_import_cg,
+            self.options, self.session, arguments,
+            stderr=self.format_error_message(expect),
+            stdout='',
+            activate_session=None,
+            exit_code=2
+        )
+
+        self.session.uploadWrapper.assert_not_called()
+        self.linked_upload.assert_not_called()
+        self.session.CGImport.assert_not_called()
+
+    def test_handle_import_cg_test(self):
+        """Test handle_import_cg function"""
+
+        metadata, fn = self.make_metadata()
+        arguments = [fn, self.outdir, '--test']
+
+        handle_import_cg(self.options, self.session, arguments)
+
+        self.session.uploadWrapper.assert_not_called()
+        self.linked_upload.assert_not_called()
+        self.session.CGImport.assert_not_called()
+
+    def test_handle_import_cg_metaonly(self):
+        """Test handle_import_cg function"""
+
+        metadata, fn = self.make_metadata(write_outputs=False)
+        for info in metadata['output']:
+            info['metadata_only'] = True
+        metadata, fn = self.make_metadata(metadata, defaults=False, write_outputs=False)
+        arguments = [fn, self.outdir]
+
+        handle_import_cg(self.options, self.session, arguments)
+
+        self.session.uploadWrapper.assert_not_called()
+        self.linked_upload.assert_not_called()
+        self.session.CGImport.assert_called_once()
+        args = self.session.CGImport.call_args.args
+        self.assertEqual(args[0], metadata)
+
+    def test_handle_import_cg_draft(self):
+        """Test handle_import_cg function"""
+
+        metadata, fn = self.make_metadata()
+        arguments = [fn, self.outdir, '--draft']
+        # metadata from the call should have draft flag added
+        expect = metadata.copy()
+        expect['build']['draft'] = True
+
+        # Case 1, running in fg, progress on
+        handle_import_cg(self.options, self.session, arguments)
+
+        self.assertEqual(len(self.session.uploadWrapper.mock_calls), len(metadata['output']))
+        self.session.CGImport.assert_called_once()
+        args = self.session.CGImport.call_args.args
+        self.assertEqual(args[0], metadata)
+
+    def test_handle_import_cg_reserve(self):
+        """Test handle_import_cg function"""
+
+        metadata, fn = self.make_metadata()
+        arguments = [fn, self.outdir, '--build-id', '12345']
+        # metadata from the call should have draft flag added
+        expect = metadata.copy()
+        expect['build']['build_id'] = 12345
+
+        # Case 1, running in fg, progress on
+        handle_import_cg(self.options, self.session, arguments)
+
+        self.assertEqual(len(self.session.uploadWrapper.mock_calls), len(metadata['output']))
+        self.session.CGImport.assert_called_once()
+        args = self.session.CGImport.call_args.args
+        self.assertEqual(args[0], expect)
+
+    def test_handle_import_cg_linked(self):
+        """Test handle_import_cg function"""
+
+        metadata, fn = self.make_metadata()
+        arguments = [fn, self.outdir, '--link']
+
+        handle_import_cg(self.options, self.session, arguments)
+
+        self.session.uploadWrapper.assert_not_called()
+        self.assertEqual(len(self.linked_upload.mock_calls), len(metadata['output']))
+        self.session.CGImport.assert_called_once()
+        args = self.session.CGImport.call_args.args
+        self.assertEqual(args[0], metadata)
+
+    def test_handle_import_cg_noprogress(self):
+        """Test handle_import_cg function"""
+
+        metadata, fn = self.make_metadata()
+        arguments = [fn, self.outdir, '--noprogress']
+
+        handle_import_cg(self.options, self.session, arguments)
+
+        self.assertEqual(len(self.session.uploadWrapper.mock_calls), len(metadata['output']))
+        kwargs = self.session.uploadWrapper.call_args.kwargs
+        self.assertEqual(kwargs['callback'], None)
+        self.session.CGImport.assert_called_once()
+        args = self.session.CGImport.call_args.args
+        self.assertEqual(args[0], metadata)
+        self.linked_upload.assert_not_called()
 
     def test_handle_import_cg_help(self):
         """Test handle_import_cg help message"""
@@ -185,13 +247,14 @@ class TestImportCG(utils.CliTestCase):
 (Specify the --help global option for a list of other help options)
 
 Options:
-  -h, --help     show this help message and exit
+  -h, --help           show this help message and exit
-  --noprogress   Do not display progress of the upload
+  --noprogress         Do not display progress of the upload
-  --link         Attempt to hardlink instead of uploading
+  --link               Attempt to hardlink instead of uploading
-  --test         Don't actually import
+  --test               Don't actually import
-  --token=TOKEN  Build reservation token
+  --token=TOKEN        Build reservation token
+  --draft              Import as a draft
+  --build-id=BUILD_ID  Reserved build id
 """ % self.progname)
 
 
-if __name__ == '__main__':
+# the end
-    unittest.main()