#1385 Add --no-delete option to clone-tag
Merged 3 months ago by tkopecek. Opened a year ago by tkopecek.
tkopecek/koji issue1384  into  master

file modified
+129 -101

@@ -3377,6 +3377,9 @@ 

                               "the dest tag"))

      parser.add_option('--ts', type='int', metavar="TIMESTAMP",

                        help=_('Clone tag at last event before specific timestamp'))

+     parser.add_option('--no-delete', action='store_false', dest="delete",

+                       default=True,

+                       help=_("Don't delete any existing content in dest tag."))

      parser.add_option('--event', type='int',

                        help=_('Clone tag at a specific event'))

      parser.add_option('--repo', type='int',

@@ -3526,6 +3529,8 @@ 

                                              inherited=True):

                  dstpkgs[pkg['package_name']] = pkg

          if options.builds:

+             # listTagged orders builds latest-first

+             # so reversing that gives us oldest-first

              for build in reversed(session.listTagged(srctag['id'],

                                                       event=event.get('id'),

                                                       inherit=options.inherit_builds,

@@ -3557,26 +3562,46 @@ 

          pdellist.sort(key=lambda x: x['package_name'])

          baddlist = []  # list containing new builds to be added from src tag

          bdellist = []  # list containing new builds to be removed from dst tag

-         for (pkg, dstblds) in six.iteritems(dstbldsbypkg):

-             if pkg not in srcbldsbypkg:

-                 bdellist.extend(dstblds.values())

+         if options.delete:

+             # remove builds for packages that are absent from src tag

+             for (pkg, dstblds) in six.iteritems(dstbldsbypkg):

+                 if pkg not in srcbldsbypkg:

+                     bdellist.extend(dstblds.values())

+         # add and/or remove builds from dst to match src contents and order

          for (pkg, srcblds) in six.iteritems(srcbldsbypkg):

              dstblds = dstbldsbypkg[pkg]

              ablds = []

              dblds = []

-             # firstly, remove builds from dst tag

+             # firstly, deal with extra builds in dst

              removed_nvrs = set(dstblds.keys()) - set(srcblds.keys())

-             dnvrs = []

-             for (dstnvr, dstbld) in six.iteritems(dstblds):

-                 if dstnvr in removed_nvrs:

-                     dnvrs.append(dstnvr)

-                     dblds.append(dstbld)

-             for dnvr in dnvrs:

-                 del dstblds[dnvr]

+             bld_order = srcblds.copy()

+             if options.delete:

+                 # mark the extra builds for deletion

+                 dnvrs = []

+                 for (dstnvr, dstbld) in six.iteritems(dstblds):

+                     if dstnvr in removed_nvrs:

+                         dnvrs.append(dstnvr)

+                         dblds.append(dstbld)

+                 # we also remove them from dstblds now so that they do not

+                 # interfere with the order comparison below

+                 for dnvr in dnvrs:

+                     del dstblds[dnvr]

+             else:

+                 # in the no-delete case, the extra builds should be forced

+                 # to last in the tag

+                 bld_order = OrderedDict()

+                 for (dstnvr, dstbld) in six.iteritems(dstblds):

+                     if dstnvr in removed_nvrs:

+                         bld_order[dstnvr] = dstbld

+                 for (nvr, srcbld) in six.iteritems(srcblds):

+                     bld_order[nvr] = srcbld

              # secondly, add builds from src tag and adjust the order

-             for (nvr, srcbld) in six.iteritems(srcblds):

+             for (nvr, srcbld) in six.iteritems(bld_order):

                  found = False

                  out_of_order = []

+                 # note that dstblds is trimmed as we go, so we are only

+                 # considering the tail corresponding to where we are at

+                 # in the srcblds loop

                  for (dstnvr, dstbld) in six.iteritems(dstblds):

                      if nvr == dstnvr:

                          found = True

@@ -3592,11 +3617,13 @@ 

                      # loop

                      del dstblds[nvr]

                  else:

+                     # missing from dst, so we need to add it

                      ablds.append(srcbld)

              baddlist.extend(ablds)

              bdellist.extend(dblds)

          baddlist.sort(key=lambda x: x['package_name'])

          bdellist.sort(key=lambda x: x['package_name'])

+ 

          gaddlist = []  # list containing new groups to be added from src tag

          for (grpname, group) in six.iteritems(srcgroups):

              if grpname not in dstgroups:

@@ -3715,103 +3742,104 @@ 

                                                  force=options.force)

          if not options.test:

              session.multiCall(batch=options.batch)

-         # DEL packages.

-         ninhrtpdellist = []

-         inhrtpdellist = []

-         for pkg in pdellist:

-             if pkg['tag_name'] == dsttag['name']:

-                 ninhrtpdellist.append(pkg)

-             else:

-                 inhrtpdellist.append(pkg)

-         session.multicall = True

-         # delete only non-inherited packages.

-         for pkg in ninhrtpdellist:

-             # check if package have owned builds inside.

-             session.listTagged(dsttag['name'],

-                                package=pkg['package_name'],

-                                inherit=False)

-         bump_builds = session.multiCall(batch=options.batch)

-         if not options.test:

+         if options.delete:

+             # DEL packages

+             ninhrtpdellist = []

+             inhrtpdellist = []

+             for pkg in pdellist:

+                 if pkg['tag_name'] == dsttag['name']:

+                     ninhrtpdellist.append(pkg)

+                 else:

+                     inhrtpdellist.append(pkg)

              session.multicall = True

-         for pkg, [builds] in zip(ninhrtpdellist, bump_builds):

-             # remove all its builds first if there are any.

-             for build in builds:

-                 # add missing 'name' field.

-                 build['name'] = build['package_name']

-                 chgbldlist.append(('[del]',

-                                    build['package_name'],

-                                    build['nvr'],

-                                    koji.BUILD_STATES[build['state']],

-                                    build['owner_name'],

-                                    build['tag_name']))

-                 # so delete latest build(s) from new tag.

-                 if not options.test:

-                     session.untagBuildBypass(dsttag['name'],

-                                              build,

-                                              force=options.force,

-                                              notify=options.notify)

-             # now safe to remove package itself since we resolved its builds.

-             chgpkglist.append(('[del]',

-                                pkg['package_name'],

-                                pkg['blocked'],

-                                pkg['owner_name'],

-                                pkg['tag_name']))

-             if not options.test:

-                 session.packageListRemove(dsttag['name'],

-                                           pkg['package_name'],

-                                           force=False)

-         # mark as blocked inherited packages.

-         for pkg in inhrtpdellist:

-             chgpkglist.append(('[blk]',

-                                pkg['package_name'],

-                                pkg['blocked'],

-                                pkg['owner_name'],

-                                pkg['tag_name']))

+             # delete only non-inherited packages.

+             for pkg in ninhrtpdellist:

+                 # check if package have owned builds inside.

+                 session.listTagged(dsttag['name'],

+                                    package=pkg['package_name'],

+                                    inherit=False)

+             bump_builds = session.multiCall(batch=options.batch)

              if not options.test:

-                 session.packageListBlock(dsttag['name'], pkg['package_name'])

-         if not options.test:

-             session.multiCall(batch=options.batch)

-         # DEL groups.

-         if not options.test:

-             session.multicall = True

-         for group in gdellist:

-             # Only delete a group that isn't inherited

-             if group['tag_id'] == dsttag['id']:

+                 session.multicall = True

+             for pkg, [builds] in zip(ninhrtpdellist, bump_builds):

+                 # remove all its builds first if there are any.

+                 for build in builds:

+                     # add missing 'name' field.

+                     build['name'] = build['package_name']

+                     chgbldlist.append(('[del]',

+                                        build['package_name'],

+                                        build['nvr'],

+                                        koji.BUILD_STATES[build['state']],

+                                        build['owner_name'],

+                                        build['tag_name']))

+                     # so delete latest build(s) from new tag.

+                     if not options.test:

+                         session.untagBuildBypass(dsttag['name'],

+                                                  build,

+                                                  force=options.force,

+                                                  notify=options.notify)

+                 # now safe to remove package itself since we resolved its builds.

+                 chgpkglist.append(('[del]',

+                                    pkg['package_name'],

+                                    pkg['blocked'],

+                                    pkg['owner_name'],

+                                    pkg['tag_name']))

                  if not options.test:

-                     session.groupListRemove(dsttag['name'],

-                                             group['name'],

-                                             force=options.force)

-                 for pkg in group['packagelist']:

-                     chggrplist.append(('[del]', pkg['package'], group['name']))

-             # mark as blocked inherited groups.

-             else:

+                     session.packageListRemove(dsttag['name'],

+                                               pkg['package_name'],

+                                               force=False)

+             # mark as blocked inherited packages.

+             for pkg in inhrtpdellist:

+                 chgpkglist.append(('[blk]',

+                                    pkg['package_name'],

+                                    pkg['blocked'],

+                                    pkg['owner_name'],

+                                    pkg['tag_name']))

                  if not options.test:

-                     session.groupListBlock(dsttag['name'], group['name'])

-                 for pkg in group['packagelist']:

-                     chggrplist.append(('[blk]', pkg['package'], group['name']))

-         if not options.test:

-             session.multiCall(batch=options.batch)

-         # DEL group pkgs.

-         if not options.test:

-             session.multicall = True

-         for group in grpchanges:

-             for pkg in grpchanges[group]['dels']:

+                     session.packageListBlock(dsttag['name'], pkg['package_name'])

+             if not options.test:

+                 session.multiCall(batch=options.batch)

+             # DEL groups.

+             if not options.test:

+                 session.multicall = True

+             for group in gdellist:

                  # Only delete a group that isn't inherited

-                 if not grpchanges[group]['inherited']:

-                     chggrplist.append(('[del]', pkg, group))

+                 if group['tag_id'] == dsttag['id']:

                      if not options.test:

-                         session.groupPackageListRemove(dsttag['name'],

-                                                        group,

-                                                        pkg,

-                                                        force=options.force)

+                         session.groupListRemove(dsttag['name'],

+                                                 group['name'],

+                                                 force=options.force)

+                     for pkg in group['packagelist']:

+                         chggrplist.append(('[del]', pkg['package'], group['name']))

+                 # mark as blocked inherited groups.

                  else:

-                     chggrplist.append(('[blk]', pkg, group))

                      if not options.test:

-                         session.groupPackageListBlock(dsttag['name'],

-                                                       group,

-                                                       pkg)

-         if not options.test:

-             session.multiCall(batch=options.batch)

+                         session.groupListBlock(dsttag['name'], group['name'])

+                     for pkg in group['packagelist']:

+                         chggrplist.append(('[blk]', pkg['package'], group['name']))

+             if not options.test:

+                 session.multiCall(batch=options.batch)

+             # DEL group pkgs.

+             if not options.test:

+                 session.multicall = True

+             for group in grpchanges:

+                 for pkg in grpchanges[group]['dels']:

+                     # Only delete a group that isn't inherited

+                     if not grpchanges[group]['inherited']:

+                         chggrplist.append(('[del]', pkg, group))

+                         if not options.test:

+                             session.groupPackageListRemove(dsttag['name'],

+                                                            group,

+                                                            pkg,

+                                                            force=options.force)

+                     else:

+                         chggrplist.append(('[blk]', pkg, group))

+                         if not options.test:

+                             session.groupPackageListBlock(dsttag['name'],

+                                                           group,

+                                                           pkg)

+             if not options.test:

+                 session.multiCall(batch=options.batch)

      # print final list of actions.

      if options.verbose:

          pfmt = '    %-7s %-28s %-10s %-10s %-10s\n'

@@ -622,6 +622,209 @@ 

      [blk]   cpkg                         group2                      

  """)

  

+     @mock.patch('sys.stdout', new_callable=six.StringIO)

+     def test_handle_clone_tag_existing_dsttag_nodelete(self, stdout):

+         args = ['src-tag', 'dst-tag', '--all', '-v', '--no-delete']

+         self.session.multiCall.return_value = []

+         self.session.listPackages.return_value = []

+         self.session.listTagged.side_effect = [[{'id': 1,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-1.0-23',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 {'id': 2,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-1.0-21',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 {'id': 3,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-0.1-1',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 ],

+                                                [],

+                                                ]

+         self.session.getTagGroups.return_value = []

+         self.session.getTag.side_effect = [{'id': 1,

+                                             'name': 'src-tag',

+                                             'arches': 'arch1 arch2',

+                                             'perm_id': 1,

+                                             'maven_support': False,

+                                             'maven_include_all': True,

+                                             'locked': False},

+                                            {'id': 2,

+                                             'name': 'dst-tag',

+                                             'arches': 'arch1 arch2',

+                                             'perm_id': 1,

+                                             'maven_support': False,

+                                             'maven_include_all': True,

+                                             'locked': False}]

+         handle_clone_tag(self.options, self.session, args)

+         self.activate_session.assert_called_once()

+         self.assert_console_message(stdout, """

+ List of changes:

+ 

+     Action  Package                      Blocked    Owner      From Tag  

+     ------- ---------------------------- ---------- ---------- ----------

+ 

+     Action  From/To Package              Build(s)                                 State      Owner      From Tag  

+     ------- ---------------------------- ---------------------------------------- ---------- ---------- ----------

+     [add]   pkg                          pkg-0.1-1                                COMPLETE   b_owner    src-tag   

+     [add]   pkg                          pkg-1.0-21                               COMPLETE   b_owner    src-tag   

+     [add]   pkg                          pkg-1.0-23                               COMPLETE   b_owner    src-tag   

+ 

+     Action  Package                      Group                       

+     ------- ---------------------------- ----------------------------

+ """)

+ 

+     @mock.patch('sys.stdout', new_callable=six.StringIO)

+     def test_handle_clone_tag_existing_dsttag_nodelete_1(self, stdout):

+         args = ['src-tag', 'dst-tag', '--all', '-v', '--no-delete']

+         self.session.multiCall.return_value = []

+         self.session.listPackages.return_value = []

+         self.session.listTagged.side_effect = [[{'id': 1,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-1.0-23',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 {'id': 2,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-1.0-21',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 {'id': 3,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-0.1-1',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 ],

+                                                 [{'id': 1,

+                                                   'package_name': 'pkg',

+                                                  'nvr': 'pkg-1.0-23',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 {'id': 3,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-0.1-1',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 {'id': 2,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-1.0-21',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 ]

+                                                ]

+         self.session.getTagGroups.return_value = []

+         self.session.getTag.side_effect = [{'id': 1,

+                                             'name': 'src-tag',

+                                             'arches': 'arch1 arch2',

+                                             'perm_id': 1,

+                                             'maven_support': False,

+                                             'maven_include_all': True,

+                                             'locked': False},

+                                            {'id': 2,

+                                             'name': 'dst-tag',

+                                             'arches': 'arch1 arch2',

+                                             'perm_id': 1,

+                                             'maven_support': False,

+                                             'maven_include_all': True,

+                                             'locked': False}]

+         handle_clone_tag(self.options, self.session, args)

+         self.activate_session.assert_called_once()

+         self.assert_console_message(stdout, """

+ List of changes:

+ 

+     Action  Package                      Blocked    Owner      From Tag  

+     ------- ---------------------------- ---------- ---------- ----------

+ 

+     Action  From/To Package              Build(s)                                 State      Owner      From Tag  

+     ------- ---------------------------- ---------------------------------------- ---------- ---------- ----------

+     [add]   pkg                          pkg-1.0-21                               COMPLETE   b_owner    src-tag   

+     [add]   pkg                          pkg-1.0-23                               COMPLETE   b_owner    src-tag   

+ 

+     Action  Package                      Group                       

+     ------- ---------------------------- ----------------------------

+ """)

+ 

+     @mock.patch('sys.stdout', new_callable=six.StringIO)

+     def test_handle_clone_tag_existing_dsttag_nodelete_2(self, stdout):

+         args = ['src-tag', 'dst-tag', '--all', '-v', '--no-delete']

+         self.session.multiCall.return_value = []

+         self.session.listPackages.return_value = []

+         self.session.listTagged.side_effect = [[{'id': 1,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-1.0-23',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 {'id': 2,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-1.0-21',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 {'id': 3,

+                                                  'package_name': 'pkg',

+                                                  'nvr': 'pkg-0.1-1',

+                                                  'state': 1,

+                                                  'owner_name': 'b_owner',

+                                                  'tag_name': 'src-tag'},

+                                                 ],

+                                                 [{'id': 2,

+                                                   'package_name': 'pkg',

+                                                   'nvr': 'pkg-1.0-21',

+                                                   'state': 1,

+                                                   'owner_name': 'b_owner',

+                                                   'tag_name': 'src-tag'},

+                                                  {'id': 3,

+                                                   'package_name': 'pkg',

+                                                   'nvr': 'pkg-0.1-1',

+                                                   'state': 1,

+                                                   'owner_name': 'b_owner',

+                                                   'tag_name': 'src-tag'},

+                                                  ]

+                                                ]

+         self.session.getTagGroups.return_value = []

+         self.session.getTag.side_effect = [{'id': 1,

+                                             'name': 'src-tag',

+                                             'arches': 'arch1 arch2',

+                                             'perm_id': 1,

+                                             'maven_support': False,

+                                             'maven_include_all': True,

+                                             'locked': False},

+                                            {'id': 2,

+                                             'name': 'dst-tag',

+                                             'arches': 'arch1 arch2',

+                                             'perm_id': 1,

+                                             'maven_support': False,

+                                             'maven_include_all': True,

+                                             'locked': False}]

+         handle_clone_tag(self.options, self.session, args)

+         self.activate_session.assert_called_once()

+         self.assert_console_message(stdout, """

+ List of changes:

+ 

+     Action  Package                      Blocked    Owner      From Tag  

+     ------- ---------------------------- ---------- ---------- ----------

+ 

+     Action  From/To Package              Build(s)                                 State      Owner      From Tag  

+     ------- ---------------------------- ---------------------------------------- ---------- ---------- ----------

+     [add]   pkg                          pkg-1.0-23                               COMPLETE   b_owner    src-tag   

+ 

+     Action  Package                      Group                       

+     ------- ---------------------------- ----------------------------

+ """)

      def test_handle_clone_tag_help(self):

          self.assert_help(

              handle_clone_tag,

@@ -640,6 +843,7 @@ 

    --inherit-builds  Include all builds inherited into the source tag into the

                      dest tag

    --ts=TIMESTAMP    Clone tag at last event before specific timestamp

+   --no-delete       Don't delete any existing content in dest tag.

    --event=EVENT     Clone tag at a specific event

    --repo=REPO       Clone tag at a specific repo event

    -v, --verbose     show changes

file modified
-1

@@ -1,6 +1,5 @@ 

  from __future__ import print_function

  from __future__ import absolute_import

- import locale

  import mock

  import os

  import six

Question is if not blocking packages (or just not deleting) is what user expects.

rebased onto b05667873b22e7358f743406f3e15868a01f3c53

a year ago

Another question here: --merge may break the order of builds, should it be warned?

I would say, that merging will break the order anyway. @tmlcoch any concerns about this?

What is meant by the order?

If the --merge is used, then I would expect that the builds from the source tags will be latest builds in the destination tag after merger.

I.e. if the source_tag contains 5 builds of a pkg "foo" where "foo-5-1" is the latest-build (returned by $ koji latest-build source_tag foo). Then when I merge content of the source_tag to the destination tag, then I would expect the $ koji latest-build destination_tag foo to return the "foo-5-1".

Ah, maybe I've got a problem now. If src contains nvr_a and dst contains nvr_b as latest and nvr_a as older. What should happen? I expect, that nvr_a is retagged as new latest, but it is not completely clear if it is the best behaviour.

@tkopecek, that's the behavior I would expect as it mimics the work-around I have in place today where I just get a list of latests builds from src tag and then tag all of them to dst tag (the builds from the src are always shown as "latest" regardless their NVRs).

However the name of the option {{--merge}} may be a bit confusing here as the merge suggests that there is a some logic behind. In this case something like {{--add}} would be maybe better name.

1 new commit added

  • rename --merge to --add
a year ago

1 new commit added

  • clarify --add option
a year ago

Ok, if it is not problem for that usecase, I'm fine with it :-) Added option rename + some help.

1 new commit added

  • fix test for renamed option
a year ago

Ah, maybe I've got a problem now. If src contains nvr_a and dst contains nvr_b as latest and nvr_a as older. What should happen? I expect, that nvr_a is retagged as new latest, but it is not completely clear if it is the best behaviour.

Sorry for late reply.
+1
And I guess in this case --force should be added anyway.

I'm not sure that --add is much better.

I often lament the name of of this command. It's more of a sync or snapshot than a clone. So, let's consider a parallel to the rsync command. There, the behavior is not to delete by default, but the command has a --delete option to get that behavior.

I'm not suggesting changing the default, but perhaps --no-delete would be a clear and accurately named option. Another option that occurs to me is --overlay, which is I think an accurate description of the behavior.

--no-delete- was my original idea, but I've found it little bit obfuscating in the beginning :-) Anyway, returning this one.

rebased onto 19cc6b882bdc2475f7077b4f7d3d27384938fc60

a year ago

Don't delete any existing content in dest tag
Note, that you can end with older latest builds in dest
than in src, if they are already tagged

This option comment has me wondering what the typical user would expect here. It seems like they might expect the latest to still match the source, particularly if they are also using --latest.

Note that bdellist is also populated with out_of_order builds. This looks like it might break the code corrects the order. Such builds are removed and re-added to fix the order. If we drop the remove, then the add will fail.

I think implementing --no-delete may require a somewhat more complex change. How critical is #1384 ?

My expectation is that the I have a tag "foo" with some set of pkgs and "bar" with other set of pkgs. Some builds have higher NVR in one but other builds have higher NVR in other, same for date/time of tagging (latest).

If I sync "foo" to "bar" I would expect this simple behavior:
All builds (or all latest if --latest is used) from "foo" will land in "bar" and will be as latest there regardless their NVR of time of tagging compared to bar set of builds.

rebased onto d01116f491b85aaea3148dc53a12e28d494a7ffb

4 months ago

I've changed the logic a bit. If --no-delete is used, all src builds will be forcefully tagged again to dst. It is behaviour @tmlcoch expects (I believe :-)).

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

4 months ago

@tkopecek, yeah, all builds from src should appear in the dst and when I use koji latest-build dst a_package_in_src a latest build of the package from src should be returned.
On top of this, none builds/packages should be removed from the dst.

Example:

src tag contains:
a-1.1-1.f31
b-1.2-3.f31
c-1.2-3.f31

dst contains:
a-2.2-2.f31
d-1.2-3.f31

after koji clone-tag --no-delete src dst the dst content (of builds) should be:
a-1.1-1.f31
b-1.2-3.f31
c-1.2-3.f31
a-2.2-2.f31
d-1.2-3.f31
(nothing - d here - was removed from the dst, builds/pks were only added)

koji latest-build dst a should return:
a-1.1-1.f31
(as the a-1.1-1.f31 comes from the src tag and was latest tagged build to the dst tag)

yep, it should work now. Let's test it.

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

3 months ago

About this part...

@@ -3567,6 +3570,8 @@ def handle_clone_tag(goptions, session, args):
                     # remove it for next pass so we stay aligned with outer
                     # loop
                     del dstblds[nvr]
+                    if not options.delete:
+                        ablds.append(srcbld)
                 else:
                     ablds.append(srcbld)
             baddlist.extend(ablds)

I see the comment from @tkopecek

I've changed the logic a bit. If --no-delete is used, all src builds will be forcefully tagged again to dst. It is behaviour @tmlcoch expects (I believe :-)).

This threw me at first, but I see the problem this is trying to solve. However, it doesn't actually work. The resulting tagBuild calls fail with a TagError, but this is hidden in the multicall invocation which is not using strict. If --force is passed, the retagging will succeed, but that shouldn't be required and has other implications.

Additionally, the above results in a lot of retagging even when it is unnecessary. In particular, successive runs of the same clone-tag with --no-delete --force will retag everything to accomplish no actual change.

Also, because of the way order preservation works, bdellist contains more than just the builds in dst that are not in src. Some of these builds are getting untagged so that they can be retagged in the right order. Simply turning off all the untag operations is not the right thing here.

Here is an example of this going wrong with the new option:

Set up a tag to clone

[mikem@localhost koji]$ kdev lkoji add-tag order3
[mikem@localhost koji]$ kdev lkoji add-pkg order3 fake --owner mikem
Adding 1 packages to tag order3
[mikem@localhost koji]$ kdev lkoji call tagBuildBypass order3 fake-1.0-23
None
[mikem@localhost koji]$ kdev lkoji call tagBuildBypass order3 fake-1.0-21
None
[mikem@localhost koji]$ kdev lkoji call tagBuildBypass order3 fake-1.0-1
None

Show the resulting build order with list-history

[mikem@localhost koji]$ kdev lkoji list-history -s tag_listing --active --tag order3
Wed Apr  1 10:45:36 2020 fake-1.0-23 tagged into order3 by mikem [still active]
Wed Apr  1 10:45:49 2020 fake-1.0-21 tagged into order3 by mikem [still active]
Wed Apr  1 10:45:51 2020 fake-1.0-1 tagged into order3 by mikem [still active]

Make an initial clone

[mikem@localhost koji]$ kdev lkoji clone-tag order3 order3_copy --all --no-delete
[mikem@localhost koji]$ kdev lkoji list-history -s tag_listing --active --tag order3_copy
Wed Apr  1 10:47:10 2020 fake-1.0-23 tagged into order3_copy by mikem [still active]
Wed Apr  1 10:47:10 2020 fake-1.0-21 tagged into order3_copy by mikem [still active]
Wed Apr  1 10:47:10 2020 fake-1.0-1 tagged into order3_copy by mikem [still active]

The first clone is done correctly because there is no dst content to retag. However, if we do the following...

[mikem@localhost koji]$ kdev lkoji untag-build order3_copy fake-1.0-23
[mikem@localhost koji]$ kdev lkoji clone-tag order3 order3_copy --all --no-delete
[mikem@localhost koji]$ kdev lkoji list-history -s tag_listing --active --tag order3_copy
Wed Apr  1 10:47:10 2020 fake-1.0-21 tagged into order3_copy by mikem [still active]
Wed Apr  1 10:47:10 2020 fake-1.0-1 tagged into order3_copy by mikem [still active]
Wed Apr  1 10:47:43 2020 fake-1.0-23 tagged into order3_copy by mikem [still active]

The build that we untag gets re-added as latest by the second clone-tag run. However, it is not latest in the src tag.

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-done, testing-ready

3 months ago

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

3 months ago

rebased onto 6191d46

3 months ago

Here's a variation of the case that @tmlcoch was concerned about :

  • dst exists
  • dst already has the builds from src
  • dst has an additional build, not in src, which is latest

E.g., using that same setup as my previous test...

[mikem@localhost koji]$ kdev lkoji call tagBuildBypass order3_copy fake-1.0-1.signed
None
[mikem@localhost pagure-tool]$ kdev lkoji clone-tag order3 order3_copy --all --no-delete
[mikem@localhost pagure-tool]$ kdev lkoji list-history -s tag_listing --active --tag order3_copy
Mon Apr  6 12:20:50 2020 fake-1.0-23 tagged into order3_copy by mikem [still active]
Mon Apr  6 12:20:50 2020 fake-1.0-21 tagged into order3_copy by mikem [still active]
Mon Apr  6 12:20:50 2020 fake-1.0-1 tagged into order3_copy by mikem [still active]
Mon Apr  6 12:23:00 2020 fake-1.0-1.signed tagged into order3_copy by mikem [still active]

So, the extra build which is not in src remains latest after the command, because it is not deleted. The other builds are already in the same order as in src, so they are not retagged.

Granted, the interaction of --no-delete with order preservation is complicated and perhaps even somewhat inherently contradictory. Maybe this behavior is ok.

It's a little unclear exactly what use-case --no-delete is supposed to address. There isn't much detail in #1384.

This behavior is probably reasonable.

@tmlcoch, any concerns about the above?

@mikem "So, the extra build which is not in src remains latest after the command, because it is not deleted. The other builds are already in the same order as in src, so they are not retagged."

Ack, not touching builds in DST tag for packages that are not in SRC tag during the clone is what I expected so this is perfect.

Ack, not touching builds in DST tag for packages that are not in SRC tag during the clone is what I expected so this is perfect.

@tmlcoch reading your response, I'm not sure we're talking about the same thing. Yes, certainly if DST has builds for, say, glibc and SRC does not, then this command with with --no-delete would leave those alone.

The case I'm talking about above would be if DST has all the glibc builds that SRC does, plus an additional one that is newer. In that case this command could also leave that build alone. At the end, the set of latest builds for the two tags would be different because that newer glibc package was not deleted.

@mikem, in such case what I would need is to have latest "glibc" build from SRC as the latest.

The desired semantics of the new clone-tag option for me is the same as if we do this via inheritance where SRC inherits from DST (DST is parent of SRC). I.e. during brew latest-build on the SRC tag all packages available in SRC are always latest, if a package is missing in SRC but is available in DST then latest for the package is latest from DST.

I have some changes here that should give the desired results.

https://github.com/mikem23/koji-playground/commits/pagure/pr/1385

This will preserve extra builds in the dst tag when using --no-delete, but force them to be last. So, every build from the src tag will be later than any extra build. Works well in my limited testing.

So, every build from the src tag will be later than any extra build

This sounds like the behavior we need, thank you!

3 new commits added

  • clone-tag: avoid marking and then unmarking for deletion
  • clone-tag: force extra builds last if not removing them
  • comments
3 months ago

Commit 207b9a0 fixes this pull-request

Pull-Request has been merged by tkopecek

3 months ago

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

3 months ago