PR#2472: cli: clone-tag --config clones also extra info - koji

koji

#2472 cli: clone-tag --config clones also extra info

Merged 3 years ago by tkopecek. Opened 3 years ago by tkopecek.

tkopecek/koji issue2371 into master

cli: clone-tag --config clones also extra info

Tomas Kopecek • 3 years ago

d5cd816

cli/koji_cli/commands.py

file modified

+17 -2

		`@@ -3427,7 +3427,10 @@`
		`print(_("Cloning at event %(id)i (%(timestr)s)") % event)`

		`# store tags.`
		`- srctag = session.getTag(args[0], event=event.get('id'))`
		`+ 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 not srctag:`
		`parser.error(_("Unknown src-tag: %s" % args[0]))`
		`@@ -3449,7 +3452,8 @@`
		`perm=srctag['perm_id'],`
		`locked=srctag['locked'],`
		`maven_support=srctag['maven_support'],`
		`- maven_include_all=srctag['maven_include_all'])`
		`+ maven_include_all=srctag['maven_include_all'],`
		`+ extra=srctag['extra'])`
		`else:`
		`session.createTag(args[1], parent=None)`
		`# store the new tag, need its assigned id.`
		`@@ -3523,6 +3527,17 @@`
		`_multicall_with_check(session, options.batch)`
		`# case of existing dst-tag.`
		`if dsttag:`
		`+ if options.config and not options.test:`
		`+ if dsttag['extra']:`
		`+ session.editTag2(dsttag['id'], remove_extra=list(dsttag['extra'].keys()))`
		`+ session.editTag2(dsttag['id'], parent=None, arches=srctag['arches'],`
		`+ perm=srctag['perm_id'],`
		`+ locked=srctag['locked'],`
		`+ maven_support=srctag['maven_support'],`
		`+ maven_include_all=srctag['maven_include_all'],`
		`+ extra=srctag['extra'])`
		`+ dsttag = session.getTag(dsttag['id'], strict=True)`
		`+`
		`# get fresh list of packages & builds into maps.`
		`srcpkgs = {}`
		`dstpkgs = {}`

tests/test_cli/test_clone_tag.py

file modified

+60 -11

		`@@ -184,7 +184,8 @@`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False},`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`None,`
		`{'id': 2,`
		`'name': 'dst-tag',`
		`@@ -192,7 +193,8 @@`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False}]`
		`+ 'locked': False,`
		`+ 'extra': {}}]`
		`self.session.multiCall.return_value = []`
		`handle_clone_tag(self.options, self.session, args)`
		`self.activate_session.assert_called_once()`
		`@@ -204,7 +206,8 @@`
		`locked=False,`
		`maven_include_all=True,`
		`maven_support=False,`
		`- parent=None, perm=1),`
		`+ parent=None, perm=1,`
		`+ extra={}),`
		`call.getTag('dst-tag', strict=True),`
		`call.listPackages(event=None,`
		`inherited=True,`
		`@@ -460,19 +463,35 @@`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False},`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`{'id': 2,`
		`'name': 'dst-tag',`
		`'arches': 'arch1 arch2',`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False}]`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`+ {'id': 2,`
		`+ 'name': 'dst-tag',`
		`+ 'arches': 'arch1 arch2',`
		`+ 'perm_id': 1,`
		`+ 'maven_support': False,`
		`+ 'maven_include_all': True,`
		`+ 'locked': False,`
		`+ 'extra': {}}]`
		`handle_clone_tag(self.options, self.session, args)`
		`self.activate_session.assert_called_once()`
		`self.session.assert_has_calls([call.hasPerm('admin'),`
		`call.getTag('src-tag', event=None),`
		`call.getTag('dst-tag'),`
		`+ call.editTag2(2, arches='arch1 arch2',`
		`+ extra={}, locked=False,`
		`+ maven_include_all=True,`
		`+ maven_support=False,`
		`+ parent=None, perm=1),`
		`+ call.getTag(2, strict=True),`
		`call.listPackages(event=None,`
		`inherited=True,`
		`tagID=1),`
		`@@ -641,14 +660,24 @@`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False},`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`{'id': 2,`
		`'name': 'dst-tag',`
		`'arches': 'arch1 arch2',`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False}]`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`+ {'id': 2,`
		`+ 'name': 'dst-tag',`
		`+ 'arches': 'arch1 arch2',`
		`+ 'perm_id': 1,`
		`+ 'maven_support': False,`
		`+ 'maven_include_all': True,`
		`+ 'locked': False,`
		`+ 'extra': {}}]`
		`handle_clone_tag(self.options, self.session, args)`
		`self.activate_session.assert_called_once()`
		`self.assert_console_message(stdout, """`
		`@@ -718,14 +747,24 @@`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False},`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`{'id': 2,`
		`'name': 'dst-tag',`
		`'arches': 'arch1 arch2',`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False}]`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`+ {'id': 2,`
		`+ 'name': 'dst-tag',`
		`+ 'arches': 'arch1 arch2',`
		`+ 'perm_id': 1,`
		`+ 'maven_support': False,`
		`+ 'maven_include_all': True,`
		`+ 'locked': False,`
		`+ 'extra': {}}]`
		`handle_clone_tag(self.options, self.session, args)`
		`self.activate_session.assert_called_once()`
		`self.assert_console_message(stdout, """`
		`@@ -788,14 +827,24 @@`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False},`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`{'id': 2,`
		`'name': 'dst-tag',`
		`'arches': 'arch1 arch2',`
		`'perm_id': 1,`
		`'maven_support': False,`
		`'maven_include_all': True,`
		`- 'locked': False}]`
		`+ 'locked': False,`
		`+ 'extra': {}},`
		`+ {'id': 2,`
		`+ 'name': 'dst-tag',`
		`+ 'arches': 'arch1 arch2',`
		`+ 'perm_id': 1,`
		`+ 'maven_support': False,`
		`+ 'maven_include_all': True,`
		`+ 'locked': False,`
		`+ 'extra': {}}]`
		`handle_clone_tag(self.options, self.session, args)`
		`self.activate_session.assert_called_once()`
		`self.assert_console_message(stdout, """`

tkopecek commented 3 years ago

Fixes: https://pagure.io/koji/issue/2371

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

3 years ago

pretty please pagure-ci rebuild

3 years ago

julian8628 commented 3 years ago

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

tkopecek commented 3 years ago

yes, it makes sense

tkopecek commented 3 years ago

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?

tkopecek commented 3 years ago

@julian8628 @cobrien ^ ?

julian8628 commented 3 years ago

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

cobrien commented 3 years ago

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.

Edited 3 years ago by cobrien

cobrien commented 3 years ago

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.

julian8628 commented 3 years ago

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.

cobrien commented 3 years ago

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.

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

3 years ago

2 new commits added

cli: taginfo displays inherited extra
hub: getBuildConfig - return inheritance info

3 years ago

rebased onto ef96a0b93a6c1ecfe64ba44c302f7318112fccf4

3 years ago

rebased onto 8e5f9476b5945e8ff5694e093094aaa399af1a7c

3 years ago

tkopecek commented 3 years ago

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

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

3 years ago

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

3 years ago

mikem commented 3 years ago

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.

mikem commented 3 years ago

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

mikem commented 3 years ago

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"

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

3 years ago

rebased onto d5cd816

3 years ago

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

3 years ago

tkopecek commented 3 years ago

Commit 3b0e0df fixes this pull-request

Pull-Request has been merged by tkopecek

3 years ago