IHA: add support for in-house apps to list host software - #34588
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat-in-house-apps #34588 +/- ##
=====================================================
Coverage ? 64.37%
=====================================================
Files ? 2065
Lines ? 209506
Branches ? 7032
=====================================================
Hits ? 134878
Misses ? 64078
Partials ? 10550
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // TODO: should an in-house app fill the InstallUUID instead? For frontend? | ||
| require.Equal(t, sw[2].SoftwarePackage.LastInstall.CommandUUID, inhouse1InstallCmd) |
There was a problem hiding this comment.
Thinking especially of when clicking on details in the software library for the last install status? Won't that try to open an installer's script outputs vs an MDM command results? rachaelshaw (meant @RachelElysia )
There was a problem hiding this comment.
Actually, it currently reports the in-house app UUID in the command_uuid field, not the install_uuid, just wondering if that's ok for the frontend - i.e. it should open an MDM command result modal when clicking on details in list host software library, not the installer's results.
There was a problem hiding this comment.
@mna I think this is right. VPP & in-house apps want command_uuid for the MDM command results modal.
|
|
||
| // create some in-house apps for no-team (this creates both iOS and iPadOS, | ||
| // but returns the iOS ids) | ||
| inHouseID1, inHouseTitleID1, err := ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ |
There was a problem hiding this comment.
This turned out ok for this test, but it's a bit awkward/error-prone API to create 2 entries for the installer payload but return (~arbitrarily) the iOS in-house and title ids.
| require.Len(t, sw, 4) | ||
| require.Equal(t, []string{"inhouse1", "inhouse2", "inhouse3", "inhouseincl"}, pluckSoftwareNames(sw)) | ||
|
|
||
| // Useful for debugging: |
There was a problem hiding this comment.
I'm thinking of leaving it there to help future us adding cases or debugging issues, unless y'all hate it.
| DumpTable(t, tx, "nano_commands") | ||
| DumpTable(t, tx, "nano_command_results") | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
It has passed the last few times, and locally too, but I'd leave this here so if we do see it failing before we merge the feature branch, it might help with debugging. If not, we can always remove it before merging to main (or leave it there a while longer).
| for _, s := range installedInHouseAppIDs { | ||
| if s.InHouseAppID != nil { | ||
| if tmpByInHouseID[*s.InHouseAppID] == nil { | ||
| // inventoried by osquery, but not installed by fleet |
There was a problem hiding this comment.
nit: in-house apps can't be inventoried by osquery rn, because they're mobile device only
There was a problem hiding this comment.
ah, true, I was caught up in duplicating the most accurately possible the VPP logic. Are we confident enough in the comment so that I remove this part?
There was a problem hiding this comment.
actually it may just be the comment that need adjusted, and the code needs to stay there (i.e. inventoried by MDM command, but not installed by Fleet)...
There was a problem hiding this comment.
I'll update it this PM.
|
this lgtm overall! |
| // TODO: should an in-house app fill the InstallUUID instead? For frontend? | ||
| require.Equal(t, sw[2].SoftwarePackage.LastInstall.CommandUUID, inhouse1InstallCmd) |
There was a problem hiding this comment.
@mna I think this is right. VPP & in-house apps want command_uuid for the MDM command results modal.
Related issue: (partially) Resolves #34001
Checklist for submitter
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing