-
Notifications
You must be signed in to change notification settings - Fork 303
Fixes #38671 - Selectively clears build information on unregistration #11475
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideAdds a new boolean setting to control whether provisioning data is cleared on host unregistration, updates the unregistration logic to respect this setting, and provides tests covering both retention and clearing scenarios. Sequence diagram for host unregistration with build profile retention settingsequenceDiagram
actor User
participant Host
participant ContentFacet
participant Setting
User->>Host: Unregister host
Host->>Setting: Check retain_build_profile_upon_unregistration
alt Retention enabled
Host->>ContentFacet: Retain provisioning info (CV/LCE/KS/media)
else Retention disabled
Host->>ContentFacet: Clear provisioning info (CV/LCE/KS/media)
end
ContentFacet->>ContentFacet: Save changes
Class diagram for changes to Host content_facet on unregistrationclassDiagram
class Host {
+content_facet: ContentFacet
}
class ContentFacet {
+bound_repositories: Array
+applicable_errata: Array
+uuid: String
+content_view_environments: Array
+kickstart_repository_id: Integer
+content_source: SmartProxy
+mark_cves_unchanged()
+save!()
}
Host --> ContentFacet
ContentFacet --> SmartProxy
class Setting {
+retain_build_profile_upon_unregistration: Boolean
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test/services/katello/registration_manager_test.rb:360` </location>
<code_context>
+ def test_unregister_host_retains_build_profile_when_setting_enabled
</code_context>
<issue_to_address>
Missing assertion for media retention/clearing as described in the setting.
Please add assertions to check that media is retained or cleared as specified by the setting, ensuring the test fully covers the described behavior.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
96405fe
to
210fae2
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.
One thought.. Now that there's a setting that controls all build info (CV/LCE, KS repo, content source), this might be a good time to refactor the keep_kickstart_repository
stuff above, in this same file. I'm thinking we don't need to pass that around as much any more. Worth some investigation.
I ve added a commit. Let me know if this looks better. I ve tried to remove the keep_kickstart_repository option |
Yes I like this better. 👍 cc @ianballou as well |
8b1aa31
to
c59b27e
Compare
I recommend we run some provisioning PRT test against this. I just want to make sure there's no worry about having that Medium missing from Host error pop up again. It's a regression that popped up one of the last times we touched this code. |
Introduces new option to retain build_profile information on unregistration Removing unnecessary keep_kickstart_repository Verify content sources
a2bbdb3
to
ed98a50
Compare
Introduces new option to retain build_profile information on unregistration
What are the changes introduced in this pull request?
A configurable option
retain_build_profile_upon_unregistration
to allow users to choose whether provisioning information like CV/LCE/KS/media is retained or cleared upon host unregistration.Considerations taken when implementing this change?
Katello typically cleared the content view environment information of a host on unregistration. However some customers want to preserve content view environments and KS repository ids so that they have these options available when build is set to false (see the redmine for more info).
What are the testing steps for this pull request?
Checkout the PR
Administrations -> Settings -> Content
Now play with the settings register and unregister a host couple of times
For retain_build_profile_upon_unregistration = no
For retain_build_profile_upon_unregistration = yes
Summary by Sourcery
Introduce a new setting to conditionally retain provisioning information when unregistering hosts and update the unregistration process accordingly.
New Features:
retain_build_profile_upon_unregistration
boolean setting to control retention of build profile data on host unregistrationEnhancements:
RegistrationManager#remove_host_artifacts
to clear or preserve Content View environments and kickstart repository based on the new settingTests: