-
Notifications
You must be signed in to change notification settings - Fork 44
Allow hosts controller to find hosts by Subscription UUID #1043
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: develop
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR enables host lookup by Subscription UUID by introducing a custom controller extension that overrides the File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
app/controllers/foreman_rh_cloud/concerns/api/v2/hosts_controller_extensions.rb
Outdated
Show resolved
Hide resolved
app/controllers/foreman_rh_cloud/concerns/api/v2/hosts_controller_extensions.rb
Outdated
Show resolved
Hide resolved
fb8986b
to
1eaa925
Compare
1eaa925
to
20aecae
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.
Hey @jeremylenz - 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> `lib/foreman_rh_cloud/engine.rb:90` </location>
<code_context>
+ # API controller extensions
+ ::Api::V2::HostsController.include ::ForemanRhCloud::Concerns::Api::V2::HostsControllerExtensions
+
+ # Controller extensions
+ ::HostsController.include ::ForemanRhCloud::Concerns::Api::V2::HostsControllerExtensions
end
</code_context>
<issue_to_address>
Including the same concern in both API and non-API controllers may introduce unintended behavior.
The concern's logic seems designed for API controllers, particularly regarding parameter handling. Please verify it works correctly with both API and non-API controllers to avoid potential issues.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@vkrizan I didn't realize before that Insights UUID and Subscription UUID were not the same. I changed it to use Subscription UUID. (Now I have no idea what the Insights facet UUID is for.. 🤷 ) |
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 tested the code and confirmed that it's working as expected. The host is successfully resolved via the subscription UUID.
However, when I click on the host in the UI, it redirects to the old Host Details page instead of the new one.
You mean the link from Vuln? The link would need to use |
Yes, when I click on one of the hosts from the https://sat.example.com/foreman_rh_cloud/insights_vulnerability/CVE-ID page. I'm redirected to the old host details page: |
@nofaralfasi @ShimShtein please coordinate the required URLs/paths with @leSamo. Thanks! |
Opened #1048 as an alternate solution. |
What are the changes introduced in this pull request?
Override the
find_resource
method of Foreman'sApi::V2::HostsController
andHostsController
, so that you can access hosts byInsights UUIDSubscription UUID.Considerations taken when implementing this change?
We already have a gem in place, FriendlyId, that allows us to use host FQDNs as IDs. Sadly, though, that gem does not allow using more than one field name as an ID. So I had to develop this custom solution.
Originally I had only extended
Api::V2::HostsController
, but the Host Statuses card was not working. I realized that makes a call to/hosts/:id/statuses
(note the lack ofapi/v2
) so I had to extend the other controller as well.What are the testing steps for this pull request?
Try to access the host details page via Subscription UUID in the url, such as
Summary by Sourcery
Override the find_resource method in Foreman’s API V2 and UI HostsController to enable fetching hosts by Subscription UUID
Enhancements: