feat: filter host software by label scoping #24801
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat-labels-scoped-software #24801 +/- ##
==============================================================
Coverage ? 63.60%
==============================================================
Files ? 1603
Lines ? 151900
Branches ? 3857
==============================================================
Hits ? 96620
Misses ? 47600
Partials ? 7680
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // We should have [installerID1] | ||
| software, _, err = ds.ListHostSoftware(ctx, host, opts) | ||
| require.NoError(t, err) | ||
| require.Len(t, software, 1) |
There was a problem hiding this comment.
here and elsewhere I could probably tighten up the checks by validating the IDs returned. Since there is other testing fixup to do, I will tackle this in a separate PR.
There was a problem hiding this comment.
Probably best to add it to the catch-all ticket too so we don't forget about this.
| sil.software_installer_id = si.id | ||
| AND sil.exclude = 1 | ||
| HAVING | ||
| count_installer_labels > 0 AND count_host_labels = 0 |
There was a problem hiding this comment.
One thing I did not think of mentioning earlier, the exclude_any case will suffer from the same issue as for the configuration profiles - for dynamic labels (based on a query result), all hosts appear to not be members until we ingest the first result for that host. So this could show an installer as "available" for a host even though once we ingest the dynamic label query results, it should've been excluded. This was fixed with timestamp checks for config profiles.
I think it'd be fine to merge as-is and worry about that exclude-any corner case later on, maybe as a "holistic" approach for the whole story? Would you mind adding it to the catch-all ticket if that approach make sense to you?
There was a problem hiding this comment.
aha, that's a good point. I can add it to the catch all.
| // We should have [installerID1] | ||
| software, _, err = ds.ListHostSoftware(ctx, host, opts) | ||
| require.NoError(t, err) | ||
| require.Len(t, software, 1) |
There was a problem hiding this comment.
Probably best to add it to the catch-all ticket too so we don't forget about this.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)