Fix is_highest logic on resyncs

[gerrod3]

fixes: #1547

[mdellweg]

Content in Pulp is meant to be immutable.
is_highest does not even look right to me when a collection version is in two different repositories.

[mdellweg]

What is this save trying to tell us?

[gerrod3]

What do you mean? I explained the bug in the issue. This if save is to just be consistent with how the function can be called.

[mdellweg]

I see if <condition> or save : in other places of this function. I'm really confused whether this flag should tell us to always save the unit, or whether it allows us to.

[gerrod3]

That previous condition is for an odd scenario where none of the collection versions for a collection had is_highest set. The reason why save is conditional is for an optimization since this function is called to update a collection version before it is saved in the CollectionSaver stage or the import_collection task, there is no point in saving the collection twice in a row.

[mdellweg]

I assume this fixes the issue at hand then.
Can you add some of the explanations as comments to the code, please?

[mdellweg]

I'm really not happy, we ignore that content must be immutable.
What happens if we remove the uniqueness constraint on the is_latest and specifically allow it to be a bit fuzzy at times? A mutable field within a uniqueness constraint seems to be additionally painful. Also what is true for a collection in one repository version does not necessarily be as true in another. Could we move that information elsewhere?

[mdellweg]

I assume this fixes the issue at hand then.
Can you add some of the explanations as comments to the code, please?

[gerrod3]

I'm really not happy, we ignore that content must be immutable. What happens if we remove the uniqueness constraint on the is_latest and specifically allow it to be a bit fuzzy at times? A mutable field within a uniqueness constraint seems to be additionally painful. Also what is true for a collection in one repository version does not necessarily be as true in another. Could we move that information elsewhere?

This is how it has been for years... Yes we should refactor it to better be align with Pulp philosophy, not sure how though. For AAH it has been fine to have this field calculated on the model since they only cared about one repository, but with the introduction of repo management it might be time to look at this field again. I think we should file an issue for this and put the design discussion there.

[patchback]

Backport to 0.17: 💚 backport PR created

✅ Backport PR branch: patchback/backports/0.17/6f7f087aca62923870d7d408bdcdd25fa63d39d7/pr-1548

Backported as #1552

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to 0.16: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 6f7f087 on top of patchback/backports/0.16/6f7f087aca62923870d7d408bdcdd25fa63d39d7/pr-1548

Backporting merged PR #1548 into main

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/pulp/pulp_ansible.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/0.16/6f7f087aca62923870d7d408bdcdd25fa63d39d7/pr-1548 upstream/0.16

4. Now, cherry-pick PR Fix is_highest logic on resyncs #1548 contents into that branch:

   $ git cherry-pick -x 6f7f087aca62923870d7d408bdcdd25fa63d39d7

   If it'll yell at you with something like fatal: Commit 6f7f087aca62923870d7d408bdcdd25fa63d39d7 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x 6f7f087aca62923870d7d408bdcdd25fa63d39d7

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Fix is_highest logic on resyncs #1548 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/0.16/6f7f087aca62923870d7d408bdcdd25fa63d39d7/pr-1548

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[mdellweg]

Manual cherry-pick:
#1984