-
Notifications
You must be signed in to change notification settings - Fork 291
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 #37599 - ensure KS repo remains when reregistering host #11049
Fixes #37599 - ensure KS repo remains when reregistering host #11049
Conversation
@@ -128,6 +128,7 @@ def joined_hostnames(hosts) | |||
def unregister_host(host, options = {}) | |||
organization_destroy = options.fetch(:organization_destroy, false) | |||
unregistering = options.fetch(:unregistering, false) | |||
reregistering = options.fetch(:reregistering, false) |
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 be good to add reregistering
to the comment-documentation 4 lines above here
@@ -139,6 +140,8 @@ def unregister_host(host, options = {}) | |||
|
|||
if unregistering | |||
remove_host_artifacts(host) | |||
elsif reregistering | |||
remove_host_artifacts(host, kickstart_repository_id: host.content_facet.kickstart_repository_id) |
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.
can never be too cautious 😄
remove_host_artifacts(host, kickstart_repository_id: host.content_facet.kickstart_repository_id) | |
remove_host_artifacts(host, kickstart_repository_id: host.content_facet&.kickstart_repository_id) |
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.
Should we place a logger.info or debug incase this happens again we can track down the params and host_artifacts for satellite installs?
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 we do add a debug log I think the most useful place would be at the top of remove_host_artifacts
with all the options values that were passed in (including unregistering
and reregistering
.)
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.
+1
@@ -128,6 +128,7 @@ def joined_hostnames(hosts) | |||
def unregister_host(host, options = {}) | |||
organization_destroy = options.fetch(:organization_destroy, false) | |||
unregistering = options.fetch(:unregistering, false) | |||
reregistering = options.fetch(:reregistering, false) |
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.
Is reregistering
an accurate name? Has the host been registered before at this point?
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 suppose I could change it to something like keep_kickstart_repo
.
I named it reregistering
originally because it communicates that the unregister code is running before being expected to register again later.
2136806
to
be4f29c
Compare
unregister_host(host, :unregistering => true) | ||
# Keep the kickstart repository ID so the host's Medium isn't unset | ||
# Important for registering a host during provisioning | ||
unregister_host(host, :reregistering => true, :keep_kickstart_repository => true) |
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.
Shouldn't this be unregistering
?
unregister_host(host, :reregistering => true, :keep_kickstart_repository => true) | |
unregister_host(host, :unregistering => true, :keep_kickstart_repository => true) |
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.
yep indeed
Looks like there are some related test failures. |
be4f29c
to
860a9a8
Compare
Tests should be fixed now. |
Unfortunately the RHEL 9 packit build seems to have failed to produce any RPMs even though it reported success. Edit: it seems the RPMs are there, but they don't show up in my browser on the repo page. Odd! |
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.
Approved pending a final functional test 👍
Merging - I was able to provision RHEL 8 and RHEL 9 with hosts using the kt_activation_keys parameter. |
What are the changes introduced in this pull request?
Makes sure that the KS repo doesn't become
nil
for reregistering hosts that have a KS repo. Otherwise, the Host record will become invalid with a missing Medium.Considerations taken when implementing this change?
Do we need to clear any host artifacts when reregistering?
What are the testing steps for this pull request?