<div dir="ltr"><div>Hey Grant, </div><div><br></div><div>Let's start with thoughts without modularity :)</div><div>I think the main reasons for the existing behavior are semantical and also aim to preserve collections as they are.</div><div>The situation you describe (when I change only pkglist and nothing else at all) usually means one of the following:</div><div> 1. A user wants to combine the same advisory which came from different repos, e.g RHEL base and RHEL debuginfo.</div><div> 2. A user tries to fix an advisory in a bad way. This should  <strike>never</strike> rarely happen. If they change advisory, they need to update the `updated` timestamp and or version, and no merge will be done.</div><div><br></div><div>For situation #1, if it happens that collection names are the same (which is often not the case in the wild), we are not modifying the existing one, and you can clearly see that it was combined, it also preserves grouping of packages in it which might be relevant, e.g. debuginfo vs others.</div><div>For situation #2, I agree it looks a bit weird but they should not do it in the first place.</div><div><br></div><div>Having said that, there is no problem functionality wise, one or two collections will work fine if their names are different. </div><div>I personally would leave it as is. It gives me more confidence that we are not modifying a collection package list but just adding a new one and feeding it to createrepo_c as is. Plus if there is a problem, it's easier to see that it was a merge.</div><div><br></div><div>Now the modularity part :)</div><div>This might require some experimenting with dnf or just asking them how the situation with multiple collections for the same modules is handled.</div><div>Here is how modular collection looks:</div><div><br></div><div>Collection</div><div>    - name_0</div><div>    - module NSVCA_0</div><div>    - list of packages</div><div><br></div><div>If we look at 2 modular collections with the same name but different modules, those can not be merged. </div><div>Current behaviour covers that case. If we choose to put all packages into one collection at the merge time, we need to check the modularity aspect of the collection.</div><div><br></div><div>If we look at 2 modular collections with the same name and same modules, we technically can merge them. </div><div>And here is the unknown part for me, if we leave 2 collections with the same modules and make names different, will dnf handle it correctly, or does it expect only one collection per module.</div><div>I'd think that dnf doesn't care for module uniqueness if the collection names are different but it's worth checking.</div><div><br></div><div>Thanks for sorting it all out,</div><div>Tanya</div><div><br></div><div>P.S. +1 to give a better name to the unnamed collections, fwiw, in pulp 2 it was something like "collection_0", collection_1", etc.</div><div><br></div><div><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr 20, 2021 at 3:03 PM Grant Gainey <<a href="mailto:ggainey@redhat.com">ggainey@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hey gang,<div><br></div><div>In the process of fixing issue <a href="https://pulp.plan.io/issues/7282" target="_blank">7282</a>, 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.</div><div><br></div><div>The PR with the proposed fix is <a href="https://github.com/pulp/pulp_rpm/pull/1973" target="_blank">https://github.com/pulp/pulp_rpm/pull/1973</a></div><div><br></div><div>The scenario is as follows:</div><div><br></div><div>1) Upload an advisory that has a package-list with one RPM, say 'bear', to a repository.</div><div>2) Upload an *identical* advisory, except for having a different package list with one RPM, say 'camel', to the same repository.</div><div>3) The resulting advisory-conflict-resolution falls into the "date and version identical, pkg-list disjoint" case, which calls for an advisory merge here:</div><div>  <a href="https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/advisory.py#L222" target="_blank">https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/advisory.py#L222</a></div><div><br></div><div>4) advisory_merge() finds two 'collections' - one for each pkglist.packages in each advisory, neither of which is named.</div><div>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".</div><div><br></div><div>You can see where this decision is made here:<div>   <a href="https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/advisory.py#L273-L284" target="_blank">https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/advisory.py#L273-L284<br></a></div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>If this turns out to be undesirable emergent behavior, then I can address it as part of this PR.</div><div><br></div><div>The test I added for all this fun is here:</div><div>  <a href="https://github.com/pulp/pulp_rpm/pull/1973/files#diff-781abba15eb7e61b9ae644ae112bf1292dfd7eb78e7162b74830d4ddb43e1868R274" target="_blank">https://github.com/pulp/pulp_rpm/pull/1973/files#diff-781abba15eb7e61b9ae644ae112bf1292dfd7eb78e7162b74830d4ddb43e1868R274</a></div><div><br></div><div>Thoughts very much appreciated!</div><div>G</div><div><br></div><div>[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...</div>-- <br><div dir="ltr"><div dir="ltr"><div><div dir="ltr"><div>Grant Gainey</div><div>Principal Software Engineer, Red Hat System Management Engineering</div></div></div></div></div></div></div>
</blockquote></div></div>