12.2.13 #1128
Merged
12.2.13 #1128
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…1112) * Add tests for 5 Dynflow actions and fix facet count bug This commit adds comprehensive tests for the 5 Dynflow actions introduced in commit 596699a. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> (cherry picked from commit f6422b4)
* Clear host from HBI on unregister (cherry picked from commit b2abedd)
Reviewer's GuideRefactors async job tests to use Dynflow testing factories instead of ForemanTasks.sync_task, adds comprehensive test coverage for new inventory report-related async jobs and registration behavior, introduces HBI host deletion on host unregister in IoP mode with robust error handling, fixes Insights facets counting/logging, extends Foreman inventory URL helpers, and bumps the plugin version to 12.2.13. Sequence diagram for host unregister with HBI deletion in IoP modesequenceDiagram
actor User
participant Katello_RegistrationManager
participant ForemanRhCloud_RegistrationManagerExtensions
participant ForemanInventoryUpload
participant HBI_Service
participant Rails_logger
User->>Katello_RegistrationManager: unregister_host(host, options)
activate Katello_RegistrationManager
Note over Katello_RegistrationManager,ForemanRhCloud_RegistrationManagerExtensions: singleton_class.prepend routes call to extension
Katello_RegistrationManager->>ForemanRhCloud_RegistrationManagerExtensions: unregister_host(host, options)
activate ForemanRhCloud_RegistrationManagerExtensions
ForemanRhCloud_RegistrationManagerExtensions->>host: reload
alt IoP_mode_and_not_organization_destroy_and_host_has_insights_uuid
ForemanRhCloud_RegistrationManagerExtensions->>ForemanRhCloud_RegistrationManagerExtensions: hbi_host_destroy(host)
activate ForemanRhCloud_RegistrationManagerExtensions
ForemanRhCloud_RegistrationManagerExtensions->>Rails_logger: debug Unregistering host uuid from HBI
ForemanRhCloud_RegistrationManagerExtensions->>ForemanInventoryUpload: host_by_id_url(uuid)
ForemanInventoryUpload-->>ForemanRhCloud_RegistrationManagerExtensions: hbi_host_url
ForemanRhCloud_RegistrationManagerExtensions->>HBI_Service: DELETE hbi_host_url
alt HBI_host_not_found
HBI_Service-->>ForemanRhCloud_RegistrationManagerExtensions: RestClient::NotFound
ForemanRhCloud_RegistrationManagerExtensions->>Rails_logger: warn Attempted to destroy HBI host uuid but does not exist
else HBI_delete_error
HBI_Service-->>ForemanRhCloud_RegistrationManagerExtensions: StandardError
ForemanRhCloud_RegistrationManagerExtensions->>Rails_logger: error Failed to destroy HBI host uuid message
else HBI_delete_success
HBI_Service-->>ForemanRhCloud_RegistrationManagerExtensions: 204 No Content
end
deactivate ForemanRhCloud_RegistrationManagerExtensions
else Hosted_mode_or_organization_destroy_or_no_insights_uuid
ForemanRhCloud_RegistrationManagerExtensions-->>ForemanRhCloud_RegistrationManagerExtensions: skip_hbi_host_destroy
end
ForemanRhCloud_RegistrationManagerExtensions->>host: insights.destroy!
ForemanRhCloud_RegistrationManagerExtensions->>Katello_RegistrationManager: super(host, options)
deactivate ForemanRhCloud_RegistrationManagerExtensions
Katello_RegistrationManager-->>User: unregister_result
deactivate Katello_RegistrationManager
Updated class diagram for registration manager extensions and inventory URL helpersclassDiagram
class Katello_RegistrationManager {
+unregister_host(host, options)
}
class ForemanRhCloud_RegistrationManagerExtensions {
+logger()
+unregister_host(host, options)
+hbi_host_destroy(host)
}
class ForemanRhCloud_CertAuth {
}
class ForemanInventoryUpload {
+inventory_base_url()
+inventory_self_url()
+host_by_id_url(host_uuid)
+hosts_by_ids_url(host_uuids)
}
class ForemanRhCloud_Engine {
+register_scheduled_task(task_class, cronline)
}
Katello_RegistrationManager <|.. ForemanRhCloud_RegistrationManagerExtensions
ForemanRhCloud_RegistrationManagerExtensions ..> ForemanRhCloud_CertAuth : includes
ForemanRhCloud_RegistrationManagerExtensions ..> ForemanInventoryUpload : uses
ForemanRhCloud_Engine ..> Katello_RegistrationManager : extends_registration
ForemanRhCloud_Engine ..> ForemanRhCloud_RegistrationManagerExtensions : prepends_singleton_class
ForemanInventoryUpload ..> ForemanRhCloud_Engine : loaded_by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In ExponentialBackoffTest, the last test no longer asserts on the failure outcome (it just runs the action without checking the error or state), which weakens the test compared to the previous behavior that verified the aggregated message; consider restoring explicit assertions about the failure condition.
- In ForemanInventoryUpload, hosts_by_ids_url and the new host_by_id_url now have overlapping responsibilities; consider implementing hosts_by_ids_url in terms of host_by_id_url (or vice versa) to avoid duplicating URL construction logic.
- In ForemanRhCloud::RegistrationManagerExtensions, logging sometimes uses the local logger method and sometimes Rails.logger directly; consider using a single logging entry point for consistency (e.g., always use the logger method).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In ExponentialBackoffTest, the last test no longer asserts on the failure outcome (it just runs the action without checking the error or state), which weakens the test compared to the previous behavior that verified the aggregated message; consider restoring explicit assertions about the failure condition.
- In ForemanInventoryUpload, hosts_by_ids_url and the new host_by_id_url now have overlapping responsibilities; consider implementing hosts_by_ids_url in terms of host_by_id_url (or vice versa) to avoid duplicating URL construction logic.
- In ForemanRhCloud::RegistrationManagerExtensions, logging sometimes uses the local logger method and sometimes Rails.logger directly; consider using a single logging entry point for consistency (e.g., always use the logger method).
## Individual Comments
### Comment 1
<location> `app/controllers/concerns/foreman_rh_cloud/registration_manager_extensions.rb:9-18` </location>
<code_context>
+ Rails.logger
+ end
+
+ def unregister_host(host, options = {})
+ organization_destroy = options.fetch(:organization_destroy, false)
+
+ # Reload to ensure we have fresh association data
+ host.reload
+
+ # Only delete from HBI in IoP mode (hosted mode uses async job for cleanup)
+ hbi_host_destroy(host) if ForemanRhCloud.with_iop_smart_proxy? && !organization_destroy && host.insights_facet&.uuid&.presence
+ host.insights&.destroy!
+ super(host, options)
+ end
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Reloading the host without handling RecordNotFound can cause unregister to fail unexpectedly.
If the host has been deleted or become stale, `host.reload` will raise `ActiveRecord::RecordNotFound` and prevent the rest of `unregister_host` (including `super`) from running. If unregister can be called when the host might already be removed, consider either rescuing `RecordNotFound` around `reload` and continuing with best-effort cleanup, or only reloading when you know the host still exists.
</issue_to_address>
### Comment 2
<location> `app/controllers/concerns/foreman_rh_cloud/registration_manager_extensions.rb:32-37` </location>
<code_context>
+ super(host, options)
+ end
+
+ def hbi_host_destroy(host)
+ uuid = host.insights_facet.uuid
+ logger.debug "Unregistering host #{uuid} from HBI"
+ execute_cloud_request(
+ organization: host.organization,
+ method: :delete,
+ url: ForemanInventoryUpload.host_by_id_url(uuid),
+ headers: {
+ content_type: :json,
+ }
+ )
+ rescue RestClient::NotFound
+ Rails.logger.warn(_("Attempted to destroy HBI host %s, but host does not exist in HBI") % uuid)
+ rescue StandardError => e
+ # TODO: Improve error handling - don't break registration if HBI delete fails
+ Rails.logger.error(format(_("Failed to destroy HBI host %s: %s"), uuid, e.message))
+ end
+ end
</code_context>
<issue_to_address>
**suggestion:** Use the injected logger consistently instead of hardcoding Rails.logger.
In this module, the `logger` method already wraps `Rails.logger` to allow future overrides. In `hbi_host_destroy`, please use `logger.warn` and `logger.error` instead of calling `Rails.logger` directly to keep logging consistent and maintain that indirection.
```suggestion
rescue RestClient::NotFound
logger.warn(_("Attempted to destroy HBI host %s, but host does not exist in HBI") % uuid)
rescue StandardError => e
# TODO: Improve error handling - don't break registration if HBI delete fails
logger.error(format(_("Failed to destroy HBI host %s: %s"), uuid, e.message))
end
```
</issue_to_address>
### Comment 3
<location> `test/jobs/exponential_backoff_test.rb:42` </location>
<code_context>
TestAction.any_instance.stubs(:poll_intervals).returns([0, 0, 0])
- TestAction.any_instance.expects(:action_callback).raises(::Foreman::Exception.new('Foo')).times(3)
+ TestAction.any_instance.expects(:action_callback).raises(StandardError.new('Foo')).at_least_once
- ForemanTasks.sync_task(TestAction)
</code_context>
<issue_to_address>
**issue (testing):** The exponential backoff failure test no longer asserts that the action ultimately fails with the expected error/message.
Previously this test verified that the task failed after three attempts and that the aggregated message included "Foo" by rescuing ForemanTasks::TaskError and asserting on ex.aggregated_message. After the refactor to use create_and_plan_action/run_action, it just calls run_action(action) without asserting the failure type or message. Please add explicit checks again, e.g.:
- wrap run_action(action) in assert_raises with the expected error, and
- assert that the error message (or relevant action state/retry count) reflects the intended backoff behavior.
This keeps the test validating both the retry logic and the final failure mode.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+9
to
+18
| def unregister_host(host, options = {}) | ||
| organization_destroy = options.fetch(:organization_destroy, false) | ||
|
|
||
| # Reload to ensure we have fresh association data | ||
| host.reload | ||
|
|
||
| # Only delete from HBI in IoP mode (hosted mode uses async job for cleanup) | ||
| hbi_host_destroy(host) if ForemanRhCloud.with_iop_smart_proxy? && !organization_destroy && host.insights_facet&.uuid&.presence | ||
| host.insights&.destroy! | ||
| super(host, options) |
There was a problem hiding this comment.
issue (bug_risk): Reloading the host without handling RecordNotFound can cause unregister to fail unexpectedly.
If the host has been deleted or become stale, host.reload will raise ActiveRecord::RecordNotFound and prevent the rest of unregister_host (including super) from running. If unregister can be called when the host might already be removed, consider either rescuing RecordNotFound around reload and continuing with best-effort cleanup, or only reloading when you know the host still exists.
Comment on lines
+32
to
+37
| rescue RestClient::NotFound | ||
| Rails.logger.warn(_("Attempted to destroy HBI host %s, but host does not exist in HBI") % uuid) | ||
| rescue StandardError => e | ||
| # TODO: Improve error handling - don't break registration if HBI delete fails | ||
| Rails.logger.error(format(_("Failed to destroy HBI host %s: %s"), uuid, e.message)) | ||
| end |
There was a problem hiding this comment.
suggestion: Use the injected logger consistently instead of hardcoding Rails.logger.
In this module, the logger method already wraps Rails.logger to allow future overrides. In hbi_host_destroy, please use logger.warn and logger.error instead of calling Rails.logger directly to keep logging consistent and maintain that indirection.
Suggested change
| rescue RestClient::NotFound | |
| Rails.logger.warn(_("Attempted to destroy HBI host %s, but host does not exist in HBI") % uuid) | |
| rescue StandardError => e | |
| # TODO: Improve error handling - don't break registration if HBI delete fails | |
| Rails.logger.error(format(_("Failed to destroy HBI host %s: %s"), uuid, e.message)) | |
| end | |
| rescue RestClient::NotFound | |
| logger.warn(_("Attempted to destroy HBI host %s, but host does not exist in HBI") % uuid) | |
| rescue StandardError => e | |
| # TODO: Improve error handling - don't break registration if HBI delete fails | |
| logger.error(format(_("Failed to destroy HBI host %s: %s"), uuid, e.message)) | |
| end |
chris1984
approved these changes
Nov 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
Cherrypick f6422b4 and b2abedd
Considerations taken when implementing this change?
What are the testing steps for this pull request?
Summary by Sourcery
Update inventory and insights integration to improve host facet handling and HBI cleanup, while modernizing Dynflow-based job tests and bumping the plugin version to 12.2.13.
New Features:
Enhancements:
Tests: