Modify public REST API to get profiles for a host #79
Conversation
|
Another thing that seems obvious but may be incorrect: we don't need to make a request to the systems API to see if the system exists if we just request this profiles API. Systems registered with old insights would not be in our compliance systems response anyway. |
quarckster
force-pushed
the
master
branch
5 times, most recently
from
ded0853 to
25f9004
Compare
There was a problem hiding this comment.
@akofink I agree with the direction of the PR, can you include some tests so we don't go down on code coverage? It's true we don't have any subman-id, however, the ID of the host is === to the ID of the host in the inventory.
https://github.com/RedHatInsights/compliance-backend/blob/master/app/services/host_inventory_api.rb makes sure that when a report is uploaded, we make sure to get the ID from the inventory if the host is not there, or create it there with an ID of our choosing.
| .paginate(page: params[:page], per_page: params[:per_page]) | ||
| .sort_by(&:score) | ||
| ) | ||
| end | ||
|
|
||
| def index_relation | ||
| relation = Profile.includes(:rules, :hosts) | ||
| unless profile_index_params[:hostname].nil? |
There was a problem hiding this comment.
nitpick
return (relation = Profile.includes(:rules, :hosts) if profile_index_params[:hostname].blank?
relation.where(hosts: {name: profile_index_params[:hostname]})There was a problem hiding this comment.
I have two issues with this:
- It's not extensible. In the future if we decide we want the ability to filter on other columns, we'd have to modify this code rather than adding code. I certainly wanted to write it like this, but see the next point about line length:
relation.where(hosts: {name: profile_index_params[:hostname]}) unless profile_index_params[:hostname].nil?- It's not as readable (and it goes over the 80 character line limit - I think rubocop is enforcing that default)
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
==========================================
- Coverage 69.72% 69.28% -0.45%
==========================================
Files 40 39 -1
Lines 555 547 -8
==========================================
- Hits 387 379 -8
Misses 168 168
Continue to review full report at Codecov.
|
| def index_relation | ||
| relation = Profile.includes(:rules, :hosts) | ||
| relation.where(hosts: { name: profile_index_params[:hostname] }) \ | ||
| unless profile_index_params[:hostname].nil? |
There was a problem hiding this comment.
@dLobatog how about this? It's kind of a compromise. 🤷♂️
akofink
changed the title
|
Updated with tests. Rubocop doesn't like multiline trailing conditionals, so I had to use the traditional |
|
This PR is out of date. We should use scoped search for this. |
@dLobatog WIP for feedback: This endpoint is required for the client.
Should we be using subscription-manager ID or host fqdn (or allow either)? There's also anit seems we don't store sub-man ID or insights_id anywhere, so hostname is the only identifier for hosts. Please correct me if that's not the case.insights_idsupplied by the canonical facts, but I'm not sure if we're storing that anywhere?(Also WIP for tests)