-
Notifications
You must be signed in to change notification settings - Fork 289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #25452 - Duplicate repo in CCV causes mismatch #7836
Conversation
Issues: #25452 |
To test:
This commit doesn't add the ability to use exclude filter in composite content views for duplicate repos, but rather fixes a mismatch between the filesystem and repo metadata |
Going to try and figure out a good way to write tests for this, but my initial thought it is going to be pretty hard to test a mismatch between the filesystem and what pulp tells us. It's more an order-of-operations fix. |
a6d207d
to
8f421b7
Compare
343551a
to
df36470
Compare
A composite content view containing two content views will only use one filter for the repo and cause a mismatch between the published repo on the filesystem and what pulp/katello displays.
df36470
to
69acdf9
Compare
repositories.each do |repository| | ||
if new_repository.yum? | ||
# If there is more than one repository passed here, that means that there are duplicate repos in a composite content view. | ||
# We skip generating metadata in this case and generate it later to prevent conflicting data, such as filters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the metadata generation gets skipped here, where does it happen later on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this step plan_action(Katello::Repository::MetadataGenerate, new_repository) if repositories.length > 1
https://github.com/Katello/katello/pull/7836/files#diff-516603408c98ec56e67c629e9de94587R28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, didnt see that even though it was right on the screen 👓
it's correct that in the case of incremental, no metadata is regenerated? The skip_metadata
has two checks, but the regen at the end only validates the latter check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hrmmm... good point. Do we do incremental versions for composite content views?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, at least based on the text for the param in the api:
Content View Version Ids to perform an incremental update on. May contain composites as well as one or more components to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there is a better way to display the logic, this was the most readable way I could figure out, but I'm open to suggestions 📂
The original functionality should be working as is. Here are some simplified scenarios
# incremental publish and normal CV
2.5.1 :002 > incremental = true; repositories = ['foo']
=> ["foo"]
# skip metadata
2.5.1 :003 > incremental || repositories.length > 1
=> true
# incremental and CVV
2.5.1 :004 > incremental = true; repositories = ['foo', 'bar']
=> ["foo", "bar"]
2.5.1 :005 > incremental || repositories.length > 1
=> true
# normal publish and CVV
2.5.1 :006 > incremental = false; repositories = ['foo', 'bar']
=> ["foo", "bar"]
2.5.1 :007 > incremental || repositories.length > 1
=> true
# normal publish and CV
2.5.1 :008 > incremental = false; repositories = ['foo']
=> ["foo"]
# here we don't skip metadata regenerating in the task
2.5.1 :009 > incremental || repositories.length > 1
=> false
If any CV publish is incremental, we are still skipping metadata, the addition is we are skipping metadata if its a CVV with duplicate repos as well
code looks ok and tested ok for me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK from me, based on beav's testing
@beav I tested out incremental updates (using hammer) on both content views and composite content views without any issues. Feel free to test any additional scenarios. |
A composite content view containing two content views will only use
one filter for the repo and cause a mismatch between the published
repo on the filesystem and what pulp/katello displays.