-
Notifications
You must be signed in to change notification settings - Fork 284
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 #33939 - support mirroring policy on repositories #9834
Conversation
Issues: #33939 |
071a344
to
4d3735d
Compare
[test katello] |
Katello::RootRepository.where(:content_type => 'yum').where(:mirror_on_sync => true).update_all(:mirroring_policy => ::Katello::RootRepository::MIRRORING_POLICY_COMPLETE) | ||
Katello::RootRepository.where.not(:content_type => 'yum').where(:mirror_on_sync => true).update_all(:mirroring_policy => ::Katello::RootRepository::MIRRORING_POLICY_COMPLETE) |
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.
Would Katello::RootRepository.where(:mirror_on_sync => true).update_all(:mirroring_policy => ::Katello::RootRepository::MIRRORING_POLICY_COMPLETE)
get the same result as these two lines?
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.
was this supposed to be for making red hat repos mirror_complete
and other repos content_only
?
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.
ah yes, i meant to make custom repos content_only!!
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.
After migrating, if I try to update any repository I get: An error occurred saving the Repository: can't write unknown attribute mirror_on_sync
. I think this is caused by removing the mirror_on_sync
column in the migration?
On a similar note, should that column be removed if the issue says "old option should still 'work' via controller code"? Nevermind, I see what this means now.
@rverdile hrmm, i will retry that, did you refresh your UI after checking out the PR? |
@jlsherrill yeah I refreshed the page. The more precise error message is I might be missing something, but I think it's because |
0136790
to
1fbadc3
Compare
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.
Code and functionality looks good to me!. Migration works both ways and I'm able to create/update repositories' mirroring policies.
What are the changes introduced in this pull request?
Prior to pulp 3.14 (including our time with pulp2), mirror on sync meant that if content was removed from an upstream repository, it would be removed from the local repository at sync time. If this was disabled, stuff would just be continually added to the repository and never removed. This is implemented by us calling sync in pulp3 with mirror=true
As part of 3.14 pulp added a 'metadata mirroring' support and reused the same option to now trigger metadata mirroring in addition to content mirroring. It turns out that some repositories cannot actually have their metadata mirrored due to various reasons, and so pulp started throwing an error suggesting that the user not use the mirror option.
Now pulp supports a new option 'sync_policy' that lets the user decide between:
This exposes this option as a 'mirroring policy'.
On upgrade, we migrate all redhat repos set to 'mirror_on_sync' to mirror_complete, since all of them can have their metadata mirrored.
For all custom repos we migrate anything with mirror_on_sync set to true to mirror_content_only
If mirror_on_sync is not true, we migrate it to additive.
The mirror_on_sync option is also handled via the repo create/update api for backwards compatibility.
Considerations taken when implementing this change?
What are the testing steps for this pull request?