Should the extra info follow the inheritance as the package list does?

yes, it makes sense

As I'm thinking abou it more - maybe not. Problem is that it wouldn't be coherent. E.g we're probably not wanting to clone architecture list. So options:
a) use in fact getBuildConfig to create new tag (arches, extra and honour noconfig options)
b) clone only basic tag without inherited config
What's your opinion?

@julian8628 @cobrien ^ ?

I think a) is more a clone action for build tag, but let's listen to the real cases

Well, there's some cases where either are wrong -- some extra config settings only work if they're on the tag in question, for example. That means inheriting them doesn't impact how koji operates -- so if we copy all the inherited extras fields, the new tag will potentially gain a setting which impacts it in a way the original was not impacted. But then the same becomes true where the tag is relying on a setting which is inherited to operate correctly.

I think that following inheritance is more correct for the most part, but it's unfortunately not a perfect solution.

From the case of cloning a build tag right now, especially a rhel-8 tag, it'll be wrong one way or another.

As an example:

[cobrien@crayon ~]$ brew list-tag-extras jb-eap-7.4-rhel-8-build
Setting                   Value    Tag                                     
------------------------  -------  ----------------------------------------
mock.new_chroot           0        rhel-8-build-base                       
mock.package_manager      dnf      rhel-8-build-base                       
mock.yum.module_hotfixes  1        rhel-8-build-base                       
repo_include_all          1        module-ant-1.10-820181213135032-5ea3b708
repos_include_all         1        rhel-8-build-base                       
rhpkg_dist                .el8eap  jb-eap-7.4-rhel-8-build                 
rhpkg_scl_prefix          eap7     jb-eap-7.4-rhel-8-build                 
rpm.macro.dist            .el8eap  jb-eap-7.4-rhel-8-build   

If inheritance is followed, then the new tag would receive the repo_include_all setting, which means that every single RPM from every single build in the inheritance would be used in the resulting build tag repo. If inheritance is not followed, then the new tag would not know it should be using dnf.

Speaking from a purely usage oriented standpoint, if we were to set repo_include_all to 0 on rhel-8-build-base, the problem goes away. At that point it becomes most correct to follow inheritance for extra configs. So really it might be that inheritance is right and we simply need to be more careful with our settings.

It looksrepo_include_all is the only key should be without inheritance? maybe we should make it following inheritance as well?

And I think we may bring this problem with the relationship with #2283, we are also willing to have some thing like blocking the extra values from the inheritance.

I think repo_include_all is so dangerous that it probably shouldn't be inherited. If it becomes so, we'll need to take measures to ensure it's cleared in a number of places.

mbs's behavior unfortunately sets that flag on all the module tags, so we end up inheriting it all over the place.

PR #2493 will alter getBuildConfig to return inherited values. In that moment we will use it also here, so default behaviour would be inheritance.

PR #2493 will alter getBuildConfig to return inherited values. In that moment we will use it also here, so default behaviour would be inheritance.

getBuildConfig has always returned inherited extra values. The change in #2493 was to return additional data about which tag those values are inherited from.

Regardless, this cli code does not use getBuildConfig. It uses the extra field returned by getTag(). So, at present, we're not pulling inherited extra data and I doubt we'll want to make a change in the default behavior of getTag in that regard.

It doesn't look like these changes appear in the final summary that is printed.

It sounds like we're in agreement that we want inherited extra data, so we should probably make that change. Looks like this should be as simple as calling getBuildConfig instead of getTag to populate srctag. Though we have to deal with lack of the strict arg. E.g.

--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -3427,10 +3427,11 @@ def handle_clone_tag(goptions, session, args):
         print(_("Cloning at event %(id)i (%(timestr)s)") % event)
     # store tags.
-    srctag = session.getTag(args[0], event=event.get('id'))
-    dsttag = session.getTag(args[1])
-    if not srctag:
+    try:
+        srctag = session.getBuildConfig(args[0], event=event.get('id'))
+    except koji.GenericError:
         parser.error(_("Unknown src-tag: %s" % args[0]))
+    dsttag = session.getTag(args[1])
     if (srctag['locked'] and not options.force) \
             or (dsttag and dsttag['locked'] and not options.force):
         parser.error(_("Error: You are attempting to clone from or to a tag which is locked.\n"

Commit 3b0e0dfd fixes this pull-request