PR#952: cli: [clone-tag] preserve build order - koji

koji

#952 cli: [clone-tag] preserve build order

Merged 5 years ago by mikem. Opened 5 years ago by julian8628.

julian8628/koji issue/916 into master

cli: [clone-tag] preserve build order

Yuming Zhu • 5 years ago

fe510ee

cli/koji_cli/commands.py

file modified

+14 -11

		`@@ -1,8 +1,9 @@`
		`from __future__ import absolute_import`
		`from __future__ import division`
		`+`
		`import ast`
		`import base64`
		`- import dateutil.parser`
		`+ from collections import OrderedDict`
		`import fnmatch`
		`import json`
		`import logging`
		`@@ -11,12 +12,13 @@`
		`import pprint`
		`import random`
		`import re`
		`- import six`
		`import stat`
		`import sys`
		`import time`
		`import traceback`

		`+ import dateutil.parser`
		`+ import six`
		`import six.moves.xmlrpc_client`
		`from six.moves import filter`
		`from six.moves import map`
		`@@ -3302,9 +3304,10 @@`
		`session.multiCall()`
		`if options.builds:`
		`# get --all latest builds from src tag`
		`- builds = session.listTagged(srctag['id'], event=event.get('id'),`
		`- inherit=options.inherit_builds,`
		`- latest=options.latest_only)`
		`+ builds = reversed(session.listTagged(srctag['id'],`
		`+ event=event.get('id'),`
		`+ inherit=options.inherit_builds,`
		`+ latest=options.latest_only))`
		`if not options.test:`
		`session.multicall = True`
		`for build in builds:`
		`@@ -3337,7 +3340,7 @@`
		`# get fresh list of packages & builds into maps.`
		`srcpkgs = {}`
		`dstpkgs = {}`
		`- srclblds = {}`
		`+ srclblds = OrderedDict()`
		`dstlblds = {}`
		`srcgroups = {}`
		`dstgroups = {}`
		`@@ -3347,10 +3350,10 @@`
		`for pkg in session.listPackages(tagID=dsttag['id'], inherited=True):`
		`dstpkgs[pkg['package_name']] = pkg`
		`if options.builds:`
		`- src_builds = session.listTagged(srctag['id'],`
		`- event=event.get('id'),`
		`- inherit=options.inherit_builds,`
		`- latest=options.latest_only)`
		`+ src_builds = list(reversed(session.listTagged(srctag['id'],`
		`+ event=event.get('id'),`
		`+ inherit=options.inherit_builds,`
		`+ latest=options.latest_only)))`
		`for build in src_builds:`
		`srclblds[build['nvr']] = build`
		`for build in session.getLatestBuilds(dsttag['name']):`
		`@@ -3376,7 +3379,7 @@`
		`if nvr not in dstlblds:`
		`baddlist.append(lbld)`
		`baddlist.sort(key = lambda x: x['package_name'])`
		`- bdellist = [] # list containing new builds to be removed from src tag`
		`+ bdellist = [] # list containing new builds to be removed from dst tag`
		`for (nvr, lbld) in six.iteritems(dstlblds):`
		`if nvr not in srclblds:`
		`bdellist.append(lbld)`

julian8628 commented 5 years ago

Fixes #916
This is a sort-term fix for this issue

Simply reverse build list when dsttag doesn't exist
Simply reverse added build list when dsttag does exist, no matter if the builds to be added will be the latest one

tkopecek commented 5 years ago

:thumbsup:

mikem commented on line 42 of cli/koji_cli/commands.py 5 years ago

The condition does not match the text. If dsttag is non-null here, then the destination is not a new tag.
In any case, cloning builds should not require --force so routinely.

rebased onto 9f1f7d374781c7a621473ef1a18efe27a58c8a8f

5 years ago

mikem commented 5 years ago

I really don't like that this change requires --force.

I think that the important case for ordering here is the new tag case. That part of the fix is trivial.

For the existing tag case, I think I'd almost rather not go to the lengths that this patch does, at least for now. We should probably add missing builds in the correct order, but I'm not sure how importan #916 is for the existing tag case.

If we do want to handle this for the existing tag case, then the correct way is to compare the orders, and determine which combinations of tagging and untagging of builds is required to correct it (rather than relying on force).

That is significantly more complicated that the new tag case, so maybe we should defer that to a later PR.

rebased onto 7d9d321602f66a7d7a7a15dcbe7ff681e367a127

5 years ago

julian8628 commented 5 years ago

For the existing tag case, I think I'd almost rather not go to the lengths that this patch does, at least for now. We should probably add missing builds in the correct order, but I'm not sure how importan #916 is for the existing tag case.

--force tagging is required when the srctag is older than dsttag or we want to keep the order completely same as srctag. I think it's possible but obviously rare

If we do want to handle this for the existing tag case, then the correct way is to compare the orders, and determine which combinations of tagging and untagging of builds is required to correct it (rather than relying on force).

That is significantly more complicated that the new tag case, so maybe we should defer that to a later PR.

it makes sense, I'll just reverse the order for both new tag and existing tag

Edited 5 years ago by julian8628

rebased onto fe510ee

5 years ago

julian8628 commented 5 years ago

@tkopecek @mikem please review again

mikem commented 5 years ago

srclblds and dstlblds seem to stand for "source latest builds" and "destination latest builds" respectively. However:

srclblds is only latest if --latest-only is given, whereas dstlblds is always latest
these two values, though so similarly named, are generated quite differently

The fact that dstlblds is always the latest builds for the tag causes this command it get things wrong: