-
Notifications
You must be signed in to change notification settings - Fork 300
Fixes #38440 - Preserve candlepin content when deleting rolling repo clone with structured apt #11399
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 #38440 - Preserve candlepin content when deleting rolling repo clone with structured apt #11399
Conversation
Reviewer's GuideThis PR tightens the repository destroy action so that structured APT clones in rolling content views no longer trigger ContentDestroy, and adds unit tests to verify both the removal and retention behaviors. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
d014706
to
299c3b5
Compare
Added test coverage. |
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.
Hey @quba42 - I've reviewed your changes - here's some feedback:
- The combined condition in handle_custom_content is a bit dense; consider extracting
(repository.deb_using_structured_apt? && !repository.content_view.rolling?)
into a well-named predicate method to improve readability. - There's a typo in the test helper method
refute_action_planed
(should berefute_action_planned
), which could lead to confusion or silent failures. - Consider adding a test for a structured APT repo in a rolling view when
remove_from_content_view_versions
is explicitly set to true to ensure the removal flag overrides the rolling logic as expected.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
299c3b5
to
6455dc7
Compare
Fixed rubocop/lint error. |
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.
LGTM
action.stubs(:action_subject).with(clone) | ||
action.expects(:plan_self) | ||
plan_action action, clone | ||
refute_action_planed action, ::Actions::Katello::Product::ContentDestroy |
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.
There's a typo in the test helper method refute_action_planed (should be refute_action_planned), which could lead to confusion or silent failures.
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 found several more instances of refute_action_planed
in the existing tests in this file. For now I have decided to fix them all as part of my commit.
Btw: Fixing these typos had no apparent effect on running the tests locally (still green and same number of passed assetions as before). It looks like both spellings are somehow understood?
If I should only fix the typo in my new test (but not the existing tests in the file) instead let me know.
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.
Left a few comments
6455dc7
to
fa3118c
Compare
fa3118c
to
faf4738
Compare
faf4738
to
4d5e2b7
Compare
…clone with structured apt
4d5e2b7
to
5d47997
Compare
In the hope of unblocking this PR: My gut feeling is that it is better to drop the fixup commit from this branch. My argument is mainly that I think this will make the history more readable (if we need to change the relevant lines 6 Months from now). That being said, I don't feel very strongly about this, so if others disagree, I am fine with squishing things together and going with this version as well. |
5d47997
to
ce5846d
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.
Comments have been addressed and code looks good to me.
@chris1984, @sjha4 any things left preventing this from being merged?
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 👍🏼
Draft PR until I have added test coverage.
Summary by Sourcery
Prevent ContentDestroy for rolling structured APT repository clones and ensure content removal only applies to non-rolling structured APT or last-instance repos.
Bug Fixes:
Tests: