Add ability to block and unblock pkg listings

[rhartman93]

Added 'blocked' and 'unblocked' states, requiring separate argument "blocked_packages" which is a list as opposed to the current "packages" dict.

[ktdreyer]

Thanks @rhartman93 . I'm reviewing this and my notes at #2

[rhartman93]

Ah so 2 failures I see, 1) I have a bad call for the test, extra parameter, I'll remove that. For the test-collection test, it's failing because of a non standard definition of "Required" now that there are conditional requirements. Is this test a standard ansible role test? In which case maybe I can find suggested syntax for this situation. Otherwise, is there a preferable way for me to record this in the doc notes? Or do you have a disagreement with the approach to have these conditionally linked parameters in the module?

[ktdreyer]

Thank you for writing this PR. I'm still reviewing and testing it on my local Koji instance. Here are my initial thoughts...

It looks like Koji always requires an owner for a package, even when blocking it. blocked is just a boolean in the tag_packages table. In order to add a package entry to this table, an admin must add the package (with an owner) to a tag first, and then they can set that blocked boolean to true or false in the DB.

As this PR is currently written, playbook authors don't need to provide owners for blocked packages. The important thing is that this is how most users use Koji. Once an admin has "blocked" a package in a tag, an admin does not really care who owns it. I'm guessing this is why you've implemented this PR with a simple list?

Here's a corner-case. What should the expected behavior be in the following scenario?

1. A package does not yet exist in a tag at all
2. A koji_tag_packages task tells Ansible to block the package from the tag

Should Ansible add the package that tag to the package and block it (with what user account?) Or show an error to the user (like the koji block-pkg CLI does?)

I'd like to know more about how you plan to use this feature, so I can know more about how we should implement this so that it's easy to use for most cases.

---

Some other initial thoughts:

Let's remove the blocked and unblocked states. This is additional complexity that's confusing and complex to test. Let's make it so that "present" should mean the blocks are present, and "absent" should mean the blocks are absent. In other words, to unblock a list of packages, playbook authors would use blocked_packages + state: absent.

Regarding documentation: If we get rid of blocked and unblocked for state, you can just delete the required: true line for packages in the documentation and delete required=True from the packages parameter in module_args.

Avoid calling ensure_logged_in() until you're truly ready to send the RPCs that modify data. This allows Ansible to check the state without logging in (think unprivileged check mode), and Ansible will run faster when it finds no changes to make. You can move those ensure_logged_in() calls into the if not check_mode conditionals. #200 is an older ticket where I had a similar problem. (I've been thinking of generalizing this in #205 , but I'm on the fence as to whether that would be simpler or more complex to maintain.)

[rhartman93]

Gotcha, so my reasoning behind excluding owners was, I was struggling to think of a use case where you'd be adding a package block to a tag, that doesn't already have the package in it's listing either explicitly in the specified tag, or via inheritance, otherwise, why would you need to block it as it couldn't be tagged in the first place. But there's an argument to be made that maybe this module should provide the ability to sort of preemptively block packages, in which case, your suggestion to add and then block would make sense. I also like the idea of not adding additional states and rather just making the logic driven by which field happens to be populated. I think it will make sense to make them mutually exclusive in that case as well. I'll hopefully be able to make all these updates later on this week

[ktdreyer]

More discussion in internal ticket CWFCONF-1083

[ktdreyer]

Here's the use-case from CWFCONF-1083. Engineers want to block a rapidly-changing package from their -container-build tag. Tag Inheritance looks like this (edited):

 cnv-4.8-rhel-8-container-build 
  └─cnv-4.8-rhel-8-override
     └─cnv-4.8-rhel-8-candidate

• cnv-4.8-rhel-8-candidate includes not-yet-signed "kubevirt" builds.

• cnv-4.8-rhel-8-container-build should not include unsigned "kubevirt" builds.

Thinking holistically, it's unclear to me if cnv-4.8-rhel-8-container-build really ought to inherit from the -candidate tag, but let's say for the moment that will not change.

In this case, the koji administrator has manually run the following command:

koji block-pkg cnv-4.8-rhel-8-container-build kubevirt

They run this every product version, and want to capture this in Ansible.

Note, the admin did not mention having to run add-pkg prior to block-pkg. TODO: empirically test whether Koji requires that or not. Maybe if the package is inherited, Koji does not require it? Is there any error message if a package is absent from a tag and parents?

I'm leaning more towards just punting on the ownership-of-blocked-packages corner-cases. If Ansible cannot block a package (either because it's not in that tag's direct package list, or if it's not inherited), we'll raise an error to the user explaining that they must add the package to a tag or that tag's parents first.

[ktdreyer]

Here are the changes from our call yesterday: rhartman93#1

[ktdreyer]

Dumping some of my own notes here:

Given the following tags:

koji list-tag-inheritance cnv-4.8-rhel-8-container-build 
     cnv-4.8-rhel-8-container-build
....  └─cnv-4.8-rhel-8-candidate

And the package in the parent tag:

koji add-pkg --owner kdreyer cnv-4.8-rhel-8-candidate kubevirt

I can run block-pkg without adding the package to the child tag:

koji block-pkg cnv-4.8-rhel-8-container-build kubevirt

It then shows up as an entry in the listPackages RPC.

koji call listPackages tagID=cnv-4.8-rhel-8-container-build
[{'blocked': True,
  'extra_arches': '',
  'owner_id': 1,
  'owner_name': 'kdreyer',
  'package_id': 2,
  'package_name': 'kubevirt',
  'tag_id': 3,
  'tag_name': 'cnv-4.8-rhel-8-container-build'}]

Once I unblock it, it is no longer present in listPackages:

koji unblock-pkg cnv-4.8-rhel-8-container-build kubevirt
koji call listPackages tagID=cnv-4.8-rhel-8-container-build
[]

[ktdreyer]

I've opened #242 to track adding this feature to the koji_tag module.