Fixes #19552 - Fix search for hosts without arf report #258
Conversation
| .where(cond) | ||
| .where.not(:assetable_id => host_ids_from_arf_of_policy) | ||
| .pluck(:assetable_id) | ||
| { :conditions => "hosts.id IN (#{ result.empty? ? 'NULL' : result.join(',')})" } |
There was a problem hiding this comment.
Space inside string interpolation detected.
|
[test] |
1 similar comment
|
[test] |
|
[test] |
|
Failures unrelated, the branch does not contain e1d06d9 |
| WHERE foreman_openscap_arf_reports.asset_id = foreman_openscap_assets.id | ||
| AND foreman_openscap_arf_reports.policy_id = foreman_openscap_policies.id)") | ||
| .pluck(:id)).to_sql} | ||
| host_ids_from_arf_of_policy = ForemanOpenscap::ArfReport.joins(:policy).where(cond).pluck(:host_id).uniq |
There was a problem hiding this comment.
This seems to only search for direct Host - Policy associations, if the host has policy because it's assigned to host group, it's not in the list. It seems it didn't work previously either, could it be incorporated in the patch?
There was a problem hiding this comment.
It was working when I tested, the policy is never connected directly to the host - it is linked through Asset or ArfReport. It does not matter if a certain policy comes from a hostgroup - it is still linked to a host through associated asset.
|
It seems to work well, just one comment that would be nice to fix too. |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
| test "should find hosts with inherited policy that were never audited" do | ||
| policy = FactoryGirl.create(:policy) | ||
| parent = FactoryGirl.create(:hostgroup) | ||
| child = FactoryGirl.create(:hostgroup, :ancestry => "#{parent.id}") |
There was a problem hiding this comment.
Prefer to_s over string interpolation.
xprazak2
force-pushed
the
missing-arf
branch
2 times, most recently
from
9c29ff3
to
4b3064a
Compare
|
It should be working for policies inherited from hostgroups now. |
|
[test] |
|
Counts should also change at https://github.com/theforeman/foreman_openscap/blob/master/app/services/foreman_openscap/policy_dashboard/data.rb#L25-L26, we're ignoring host associated through host group there. It should probably use the same scope |
| .joins(:policies) | ||
| .where(condition) | ||
| .pluck(:assetable_id) | ||
| descendant_ids = Hostgroup.where(:id => hostgroup_with_policy_ids).flat_map(&:descendant_ids).uniq |
There was a problem hiding this comment.
&:subtree_ids contains also the record itself so we'd avoid hostgroup_with_policy_ids + descendant_ids on line below
| { :conditions => Host::Managed.arel_table[:id].in(Host::Managed.select(Host::Managed.arel_table[:id]).joins(:policies).where(cond).pluck(:id)).to_sql } | ||
|
|
||
| host_group_host_ids = policy_assigned_using_hostgroup_host_ids cond, [] | ||
| if host_group_host_ids.any? |
There was a problem hiding this comment.
Use the return of the conditional for variable assignment and comparison.
|
I made changes as suggested. |
|
Works nicely, thanks @xprazak2, merging! |
I think these queries would deserve some tests. The problem is, they need joins across multiple models which would mean setting up associations in our factories and make changes in majority of our tests.