parse target_release from draft release instead of extra.draft.target_release

cli/koji_cli/commands.py

@@ -1364,10 +1364,7 @@ def handle_import(goptions, session, args):
         b_state = koji.BUILD_STATES[draft_build['state']]
         if b_state != 'COMPLETE':
             error("draft build %s is expected as COMPLETE, got %s" % (draft_build['nvr'], b_state))
-        target_release = draft_build.get('extra', {}).get('draft', {}).get('target_release')
+        target_release = koji.parse_target_release(draft_build)
-        if not target_release:
-            error("Invalid draft build: %s,"
-                  " no draft.target_release found in extra" % draft_build['nvr'])
         draft_target_nvr = "%s-%s-%s" % (
             draft_build['name'], draft_build['version'], target_release
         )

koji/__init__.py

@@ -296,8 +296,9 @@ PRIO_DEFAULT = 20
 DEFAULT_REQUEST_TIMEOUT = 60 * 60 * 12
 DEFAULT_AUTH_TIMEOUT = 60
 
-# draft release format
+# draft release constants
 DRAFT_RELEASE_FORMAT = '{release}#draft_{id}'
+DRAFT_RELEASE_PARSING_REGEX = r'^(?P<release>.+)#draft_(?P<id>[0-9]+)$'
 
 # BEGIN kojikamid dup #
 
@@ -3928,6 +3929,52 @@ def fixEncodingRecurse(value, fallback='iso8859-15', remove_nonprintable=False):
     return walker.walk()
 
 
+def gen_draft_release(buildinfo):
+    """Generate draft_release based on input build information
+
+    Currently, it's generated as {target_release}#draft_{build_id}
+
+    :param buildinfo: buildinfo with target "release", used to generate draft_release
+    :type buildinfo: dict
+    :return: draft release
+    :rtype: str
+    :raises GenericError: if missing any required field
+    """
+    for k in ('id', 'release'):
+        if not buildinfo.get(k):
+            raise GenericError("key: '%s' not found in %s" % (k, buildinfo))
+    return DRAFT_RELEASE_FORMAT.format(**buildinfo)
+
+
+def parse_target_release(buildinfo):
+    """Generate target_release from a draft buildinfo reversely
+
+    :param buildinfo: buildinfo of a draft build
+    :type buildinfo: dict
+    :rtype: str
+    :raises GenericError: if missing draft field in buildinfo
+    :raises GenericError: if cannot get a valid target_release
+    """
+    draft_release = buildinfo.get('release')
+    if not draft_release:
+        raise GenericError("'release' not found in %s" % buildinfo)
+    build_id = buildinfo.get('id')
+    if build_id:
+        build_id = str(build_id)
+    else:
+        raise GenericError("'id' not found in %s" % buildinfo)
+    match = re.match(DRAFT_RELEASE_PARSING_REGEX, draft_release)
+    if match:
+        if build_id != match.group('id'):
+            raise GenericError(
+                "buildinfo.id: %s doesn't match build id part: %s in buildinfo.release: %s" %
+                (build_id, match.group('id'), draft_release)
+            )
+        return match.group('release')
+    else:
+        raise GenericError("draft release: %s is not in valid format" % draft_release)
+
+
 def add_file_logger(logger, fn):
     if not os.path.exists(fn):
         try:

kojihub/kojihub.py

@@ -6104,7 +6104,7 @@ def new_build(data, strict=False):
     # handle draft suffix in release
     if data.get('draft'):
         target_release = data['release']
-        data['release'] = insert_data['release'] = koji.DRAFT_RELEASE_FORMAT.format(**data)
+        data['release'] = insert_data['release'] = koji.gen_draft_release(data)
         # it's still possible to already have a build with the same nvr
         draft_nvr = dslice(data, ['name', 'version', 'release'])
         if find_build_id(draft_nvr):
@@ -6413,13 +6413,8 @@ def import_rpm(fn, buildinfo=None, brootid=None, wrapper=False, fileinfo=None, d
         # only enforce the srpm name matching the build for non-wrapper rpms
         nvrinfo = buildinfo.copy()
         if draft:
-            # for draft build the release is buildinfo.extra.draft.target_release
+            # for draft build, change release to target_release
-            target_release = (buildinfo.get('extra') or {}).get('draft', {}).get('target_release')
+            nvrinfo['release'] = koji.parse_target_release(buildinfo)
-            if not target_release:
-                raise koji.GenericError(
-                    f'target release of draft build not found in extra of build: {buildinfo}'
-                )
-            nvrinfo['release'] = target_release
         srpmname = "%(name)s-%(version)s-%(release)s.src.rpm" % nvrinfo
         # either the sourcerpm field should match the build, or the filename
         # itself (for the srpm)
@@ -15996,17 +15991,20 @@ def _promote_build(build, user=None, strict=True, force=False):
         else:
             return None
     old_release = binfo['release']
-    extra = binfo.get('extra') or {}
+
-    # get target release in binfo.extra
+    # get target release
-    draft_info = extra.get('draft', {}).copy()
+    target_release = None
-    target_release = draft_info.get('target_release')
+    try:
+        target_release = koji.parse_target_release(binfo)
+    except Exception:
+        if strict:
+            raise
     if not target_release:
         if strict:
-            raise koji.GenericError(
+            # should never happen
-                f'draft.target_release not found in extra of build: {binfo}'
+            raise koji.GenericError('Cannot generate target_release')
-            )
+        return None
-        else:
+
-            return None
     # drop id to get build by NVR
     target_build = dslice(binfo, ['name', 'version'])
     target_build['release'] = target_release
@@ -16040,7 +16038,8 @@ def _promote_build(build, user=None, strict=True, force=False):
     # temp solution with python generated time, maybe better to be an event?
     now = datetime.datetime.now()
 
-    extra['draft'] = draft_info
+    extra = binfo.get('extra') or {}
+    draft_info = extra.setdefault('draft', {})
     draft_info['promoted'] = True
     draft_info['old_release'] = old_release
     draft_info['promotion_time'] = encode_datetime(now)

tests/test_cli/test_import.py

@@ -1,9 +1,11 @@
 from __future__ import absolute_import
-import mock
+
 import os
-import six
 import unittest
 
+import mock
+import six
+
 import koji
 from koji_cli.commands import handle_import
 from . import utils
@@ -111,22 +113,24 @@ class TestImport(utils.CliTestCase):
 
         # check mock calls
         activate_session_mock.assert_called_with(session, options)
-        
+
         get_header_fields_calls = [
             mock.call(arguments[i],
                       ('name', 'version', 'release', 'epoch',
                        'arch', 'sigmd5', 'sourcepackage', 'sourcerpm')
                       ) for i in range(len(rpm_headers) - 1)
-            ]
+        ]
 
         get_header_fields_mock.assert_has_calls(get_header_fields_calls)
-        
+
         if getrpm_calls:
             session.getRPM.assert_has_calls(getrpm_calls)
         elif getrpm_called:
             session.getRPM.assert_called_with(
-                dict((k, rpm_header.get(k, ''))
+                dict(
-                    for k in ['release', 'version', 'arch', 'name'])
+                    (k, rpm_header.get(k, ''))
+                    for k in ['release', 'version', 'arch', 'name']
+                )
             )
         else:
             session.getRPM.assert_not_called()
@@ -174,8 +178,10 @@ class TestImport(utils.CliTestCase):
         )
         if getrpm_called:
             session.getRPM.assert_called_with(
-                dict((k, rpm_header.get(k, ''))
+                dict(
-                    for k in ['release', 'version', 'arch', 'name'])
+                    (k, rpm_header.get(k, ''))
+                    for k in ['release', 'version', 'arch', 'name']
+                )
             )
         else:
             session.getRPM.assert_not_called()
@@ -821,17 +827,13 @@ class TestImport(utils.CliTestCase):
             'version': '4.4.12',
             'release': '5.fc26#draft_123',
             'nvr': 'bash-4.4.12-5.fc26',
+            'id': '123',
             'draft': True,
-            'extra': {
-                'draft': {
-                    'target_release': '5.fc26'
-                }
-            },
             'state': self.bstate['COMPLETE']
         }
         session.getBuild.return_value = build
 
-        # Case 1. import src rpm with --draft-build 
+        # Case 1. import src rpm with --draft-build
         # result: success
         expected = "uploading %s... done\n" % arguments[0]
         expected += "importing %s... done\n" % arguments[0]
@@ -859,25 +861,21 @@ class TestImport(utils.CliTestCase):
         session = mock.MagicMock()
         session.getRPM.return_value = None
         build = {
+            'id': '123',
             'name': 'bash',
             'version': '4.4.12',
             'release': '5.fc26#draft_123',
             'nvr': 'bash-4.4.12-5.fc26',
             'draft': True,
-            'extra': {
-                'draft': {
-                    'target_release': '5.fc26'
-                }
-            },
             'state': self.bstate['COMPLETE']
         }
         session.getBuild.return_value = build
 
-        # Case 1. import bin rpm with --draft-build 
+        # Case 1. import bin rpm with --draft-build
         # result: success
         expected = "uploading %s... done\n" % arguments[0]
         expected += "importing %s... done\n" % arguments[0]
-        
+
         self.__do_import_test(
             options, session, arguments,
             rpm_header=self.rpm_header,
@@ -892,32 +890,55 @@ class TestImport(utils.CliTestCase):
         arguments = ['/path/to/bash-4.4.12-5.fc26.rpm', '--draft-build', '286']
         options = mock.MagicMock()
         session = mock.MagicMock()
-        
+
         cases = [
-            # build, stderr 
+            # build, stderr
             (None, "No such build: 286"),
             ({'draft': False, 'nvr': 'a-bad-draft'}, "a-bad-draft is not a draft build"),
             ({'draft': True, 'nvr': 'a-bad-draft', 'state': koji.BUILD_STATES['DELETED']},
-             "draft build a-bad-draft is expected as COMPLETE, got DELETED"),
+             "draft build a-bad-draft is expected as COMPLETE, got DELETED")
-            ({'draft': True, 'nvr': 'a-bad-draft', 'state': koji.BUILD_STATES['COMPLETE'],
-              'extra': {'draft': {'no_target_release': 'omg'}}},
-             "Invalid draft build: a-bad-draft, no draft.target_release found in extra")
         ]
         for build, expected in cases:
             session.getBuild.return_value = build
             # result: error
             expected += "\n"
             self.assert_system_exit(
-                    handle_import,
+                handle_import,
-                    options,
+                options,
-                    session,
+                session,
-                    arguments,
+                arguments,
-                    stderr=expected,
+                stderr=expected,
-                    activate_session=None,
+                activate_session=None,
-                    exit_code=1)
+                exit_code=1)
             options.reset_mock()
             session.reset_mock()
 
+    def test_handle_import_specified_draft_build_invalid_target_release(self):
+        """Test handle_import RPM import with --draft-build case.
+        Invalid target_release parsing
+        """
+        arguments = ['/path/to/bash-4.4.12-5.fc26.rpm', '--draft-build', '286']
+        options = mock.MagicMock()
+        session = mock.MagicMock()
+
+        cases = [
+            # build, stderr
+            ({'draft': True, 'nvr': 'a-bad-draft', 'state': koji.BUILD_STATES['COMPLETE'],
+              'release': 'no_id'},
+             "'id' not found in"
+             " {'draft': True, 'nvr': 'a-bad-draft', 'state': 1, 'release': 'no_id'}"),
+            ({'draft': True, 'nvr': 'a-bad-draft', 'state': koji.BUILD_STATES['COMPLETE'],
+              'id': 123, 'release': 'tgt#draft_234'},
+             "buildinfo.id: 123 doesn't match build id part: 234"
+             " in buildinfo.release: tgt#draft_234")
+        ]
+        for build, expected in cases:
+            session.getBuild.return_value = build
+            # result: error
+            with self.assertRaises(koji.GenericError) as cm:
+                handle_import(options, session, arguments)
+            self.assertEqual(str(cm.exception), expected)
+
     def test_handle_import_help(self):
         """Test handle_import function help message"""
         self.assert_help(

tests/test_hub/test_import_rpm.py

@@ -78,7 +78,6 @@ class TestImportRPM(unittest.TestCase):
             kojihub.import_rpm(self.filename)
         self.assertEqual("Build is FAILED: name-version-release", str(cm.exception))
         self.assertEqual(len(self.inserts), 0)
-        
 
     def test_import_rpm_completed_build(self):
         self.os_path_basename.return_value = 'name-version-release.arch.rpm'
@@ -212,14 +211,13 @@ class TestImportRPM(unittest.TestCase):
         self.assertEqual(insert.data, data)
         self.assertEqual(insert.rawdata, {})
 
-
     def test_import_draft_conflict(self):
         with self.assertRaises(koji.GenericError) as cm:
             kojihub.import_rpm(self.filename, buildinfo={'id': 1024, 'draft': False}, draft=True)
         self.assertEqual("draft property: False of build: 1024 mismatch, True is expected",
                          str(cm.exception))
         self.assertEqual(len(self.inserts), 0)
-    
+
     def test_import_draft_rpm_without_buildinfo(self):
         self.os_path_basename.return_value = 'name-version-release.arch.rpm'
         self.get_rpm_header.return_value = self.rpm_header_retval
@@ -229,8 +227,8 @@ class TestImportRPM(unittest.TestCase):
         self.assertEqual(f"Cannot import draft rpm: {self.os_path_basename.return_value}"
                          " without specifying a build", str(cm.exception))
         self.assertEqual(len(self.inserts), 0)
-        
+
-    def test_import_draft_rpm_non_extra_target_release(self):
+    def test_import_draft_rpm_invalid_release(self):
         self.os_path_basename.return_value = 'name-version-release.arch.rpm'
         self.get_rpm_header.return_value = self.rpm_header_retval
 
@@ -238,7 +236,7 @@ class TestImportRPM(unittest.TestCase):
             'state': koji.BUILD_STATES['DELETED'],
             'name': 'name',
             'version': 'version',
-            'release': 'release',
+            'release': 'badrelease',
             'id': 12345,
             'draft': True
         }
@@ -246,11 +244,11 @@ class TestImportRPM(unittest.TestCase):
         with self.assertRaises(koji.GenericError) as cm:
             kojihub.import_rpm(self.filename, buildinfo=buildinfo, draft=True)
         self.assertEqual(
-            f'target release of draft build not found in extra of build: {buildinfo}',
+            'draft release: badrelease is not in valid format',
             str(cm.exception)
         )
         self.assertEqual(len(self.inserts), 0)
-    
+
     def test_import_draft_rpm_valid(self):
         self.os_path_basename.return_value = 'name-version-release.arch.rpm'
         self.get_rpm_header.return_value = self.rpm_header_retval
@@ -259,7 +257,7 @@ class TestImportRPM(unittest.TestCase):
             'state': koji.BUILD_STATES['COMPLETE'],
             'name': 'name',
             'version': 'version',
-            'release': 'release',
+            'release': 'release#draft_12345',
             'id': 12345,
             'draft': True,
             'extra': {
@@ -305,14 +303,9 @@ class TestImportRPM(unittest.TestCase):
             'state': koji.BUILD_STATES['COMPLETE'],
             'name': 'name',
             'version': 'version',
-            'release': 'release',
+            'release': 'release#draft_12345',
             'id': 12345,
-            'draft': True,
+            'draft': True
-            'extra': {
-                'draft': {
-                    'target_release': 'release'
-                }
-            }
         }
         self.nextval.return_value = 9876
         kojihub.import_rpm(self.src_filename, buildinfo=buildinfo, draft=True)
@@ -384,4 +377,4 @@ class TestImportRPM(unittest.TestCase):
         self.assertEqual(insert.data, data)
         self.assertEqual(insert.rawdata, {})
         self.get_build.assert_called_once_with(5566, strict=True)
-        self.assertEqual(self.get_build.call_count, 1)
+        self.assertEqual(self.get_build.call_count, 1)

tests/test_hub/test_promote_build.py

@@ -36,7 +36,9 @@ class TestPromoteBuild(unittest.TestCase):
         self.list_tags = mock.patch('kojihub.kojihub.list_tags',
                                     return_value=[{'id': 101}]).start()
         self.set_tag_update = mock.patch('kojihub.kojihub.set_tag_update').start()
-        self.encode_datetime = mock.patch('kojihub.kojihub.encode_datetime', return_value='NOW').start()
+        self.encode_datetime = mock.patch(
+            'kojihub.kojihub.encode_datetime', return_value='NOW'
+        ).start()
         self._now = datetime.datetime.now()
         self._datetime = mock.patch('kojihub.kojihub.datetime.datetime').start()
         self.now = self._datetime.now = mock.MagicMock(return_value=self._now)
@@ -45,12 +47,12 @@ class TestPromoteBuild(unittest.TestCase):
             'id': 1,
             'name': 'foo',
             'version': 'bar',
-            'release': 'dftrel_1',
+            'release': 'tgtrel#draft_1',
             'nvr': 'testnvr',
             'extra': {
                 'draft': {
                     'promoted': False,
-                    'target_release': 'tgtrel_1'
+                    'target_release': 'tgtrel'
                 }},
             'state': 1,
             'draft': True,
@@ -58,13 +60,13 @@ class TestPromoteBuild(unittest.TestCase):
             'volume_name': 'X',
             'task_id': 222
         }
-        
+
-        self.new_build = {   
+        self.new_build = {
             # no check on the info
             'id': 1,
             'name': 'foo',
             'version': 'bar',
-            'release': 'tgtrel_1',
+            'release': 'tgtrel',
             'volume_name': 'X'
         }
 
@@ -82,15 +84,15 @@ class TestPromoteBuild(unittest.TestCase):
             {
                 'draft': {
                     'promoted': True,
-                    'target_release': 'tgtrel_1',
+                    'target_release': 'tgtrel',
-                    'old_release': 'dftrel_1',
+                    'old_release': 'tgtrel#draft_1',
                     'promotion_time': 'NOW',
                     'promotion_ts': self._now.timestamp(),
                     'promoter': self.user['name']
                 }
             }
         )
-        
+
         ret = self.exports.promoteBuild('a-draft-build', strict=True)
         self.assertEqual(ret, self.new_build)
         self.assertEqual(len(self.updates), 1)
@@ -98,7 +100,7 @@ class TestPromoteBuild(unittest.TestCase):
         self.assertEqual(update.table, 'build')
         self.assertEqual(update.values, self.draft_build)
         self.assertEqual(update.data, {'draft': False,
-                                       'release': 'tgtrel_1',
+                                       'release': 'tgtrel',
                                        'extra': extra})
         self.assertEqual(update.rawdata, {})
         self.assertEqual(update.clauses, ['id=%(id)i'])
@@ -113,21 +115,22 @@ class TestPromoteBuild(unittest.TestCase):
             self.exports.promoteBuild('a-regular-build', strict=True)
         self.assertEqual(str(cm.exception), "Not a draft build: {'draft': False}")
         self.assertEqual(len(self.updates), 0)
-        
+
         ret = self.exports.promoteBuild('a-regular-build', strict=False)
         self.assertIsNone(ret)
         self.assertEqual(len(self.updates), 0)
 
-    def test_promote_build_no_target_release(self):
+    def test_promote_build__target_release(self):
         draft = {
             'id': 1,
             'name': 'foo',
             'version': 'bar',
-            'release': 'dftrel_1',
+            'release': 'tgtrel#draft_2',
             'extra': {
                 'draft': {
-                    'promoted': False
+                    'promoted': False,
-                    # no target_release
+                    # target_release doesn't matter now
+                    'target_release': 'any'
                 }},
             'state': 1,
             'draft': True,
@@ -135,14 +138,17 @@ class TestPromoteBuild(unittest.TestCase):
             'volume_name': 'X',
             'task_id': 222
         }
-        
+
         self.get_build.return_value = draft
 
         with self.assertRaises(koji.GenericError) as cm:
             self.exports.promoteBuild('a-regular-build', strict=True)
-        self.assertEqual(str(cm.exception), f"draft.target_release not found in extra of build: {draft}")
+        self.assertEqual(
+            str(cm.exception),
+            "buildinfo.id: 1 doesn't match build id part: 2 in buildinfo.release: tgtrel#draft_2"
+        )
         self.assertEqual(len(self.updates), 0)
-        
+
         ret = self.exports.promoteBuild('a-regular-build', strict=False)
         self.assertIsNone(ret)
         self.assertEqual(len(self.updates), 0)
@@ -152,12 +158,12 @@ class TestPromoteBuild(unittest.TestCase):
             'id': 1,
             'name': 'foo',
             'version': 'bar',
-            'release': 'dftrel_1',
+            'release': 'tgtrel#draft_1',
             'nvr': 'testnvr',
             'extra': {
                 'draft': {
-                    'promoted': False
+                    'promoted': False,
-                    # no target_release
+                    'target_release': 'any'
                 }},
             'draft': True,
             'state': 0,
@@ -193,9 +199,9 @@ class TestPromoteBuild(unittest.TestCase):
         self.get_build.assert_called_with({
             'name': 'foo',
             'version': 'bar',
-            'release': 'tgtrel_1'
+            'release': 'tgtrel'
         })
-        
+
         self.get_build.reset_mock()
         self.get_build.side_effect = [self.draft_build, old]
         ret = self.exports.promoteBuild('a-regular-build', strict=False)