[Pulp-dev] Thoughts needed on Yet Another Advisory-Merge Oddness

[Grant Gainey]

Hey gang,

In the process of fixing issue 7282 <https://pulp.plan.io/issues/7282>, I
have encountered a behavior from advisory-merge that appears to be the
deliberately-desired behavior - and I'm not sure it's really what we want
to do? I need some thoughts on this. Figured I'd start in email, because
it's advisory-merge, and complicated, and trying to follow a verbal
description is confusing. More confusing.

The PR with the proposed fix is https://github.com/pulp/pulp_rpm/pull/1973

The scenario is as follows:

1) Upload an advisory that has a package-list with one RPM, say 'bear', to
a repository.
2) Upload an *identical* advisory, except for having a different package
list with one RPM, say 'camel', to the same repository.
3) The resulting advisory-conflict-resolution falls into the "date and
version identical, pkg-list disjoint" case, which calls for an advisory
merge here:
  https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/advisory.py#L222

4) advisory_merge() finds two 'collections' - one for each pkglist.packages
in each advisory, neither of which is named.
5) advisory_merge() results in an advisory with two collections. One has no
name, and is a package-list containing 'bear'. The other is named
"None_0"[0], and is a package-list containing "camel".

You can see where this decision is made here:

https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/advisory.py#L273-L284

That result was surprising to me. My assumption going in was, if we were
merging two advisories, we would merge their same-named collections into
one collection with elements from both. But we're very carefully choosing
to not-do that.

So, my question here is, can someone explain to me the rationale for the
current behavior? I'm sure it has something to do with modularity, because
half of the weird things I run into in RPM-land are because of modularity
:) But I'd like to understand why we're doing what we're doing, so I can
add some commentary in the test or in the merge_advisories() code itself.

If this turns out to be undesirable emergent behavior, then I can address
it as part of this PR.

The test I added for all this fun is here:

https://github.com/pulp/pulp_rpm/pull/1973/files#diff-781abba15eb7e61b9ae644ae112bf1292dfd7eb78e7162b74830d4ddb43e1868R274

Thoughts very much appreciated!
G

[0] "None_0" is Rude. Even if this is the right behavior, we prob could
stand to notice 'unnamed' collections and do something a little less
painful...
-- 
Grant Gainey
Principal Software Engineer, Red Hat System Management Engineering
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20210420/d7a5f485/attachment.htm>