Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30295 +/- ##
==========================================
+ Coverage 62.31% 64.20% +1.88%
==========================================
Files 1837 1871 +34
Lines 174894 183178 +8284
Branches 5315 5315
==========================================
+ Hits 108980 117603 +8623
+ Misses 57399 56337 -1062
- Partials 8515 9238 +723
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:
|
…verified erroneously
| nvq.active = 1 | ||
| AND nvq.id = ? | ||
| AND nvq.request_type = ? | ||
| AND status != 'Acknowledged'` |
There was a problem hiding this comment.
I'm still a bit confused, the name of the function is GetAcknowledgedMDMCommandsByHost but it returned commands that are not acknowledged?
Do you want to instead get the commands where status is failed (i.e. fleet.MDMAppleStatusError, fleet.MDMAppleStatusCommandFormatError)?
|
|
||
| activateNext := vppAct.Status != string(fleet.SoftwareInstalled) | ||
| if vppPtrAct != nil { | ||
| activateNext = vppPtrAct.Status != string(fleet.SoftwareInstalled) |
There was a problem hiding this comment.
Double-checking, we want to block all other upcoming activities until we are able to verify the InstalledApplications list?
| InstallCommandUUID string `db:"command_uuid"` | ||
| InstallCommandAckAt *time.Time `db:"ack_at"` | ||
| HostID uint `db:"host_id"` | ||
| InstallCommandStatus string `db:"install_command_status"` |
There was a problem hiding this comment.
Is there an enum for this?
| // Check if this is a result of a "refetch" command sent to iPhones/iPads | ||
| // to fetch their device information periodically. | ||
| if strings.HasPrefix(cmdResult.CommandUUID, fleet.RefetchBaseCommandUUIDPrefix) { | ||
| if strings.HasPrefix(cmdResult.CommandUUID, fleet.RefetchBaseCommandUUIDPrefix) && !strings.HasPrefix(cmdResult.CommandUUID, fleet.RefetchVPPAppInstallsCommandUUIDPrefix) { |
There was a problem hiding this comment.
Is there a reason to lump the verify prefix in with the refetch base? Do you currently need to match by prefix for verification (or are you more concerned about future needs)?
There was a problem hiding this comment.
@gillespi314 I was more just trying to follow the patterns; I went back and forth about making a dedicated prefix without the refetch base. Chose the refetch base because it's technically refetching, but open to making it its own thing!
| commander *apple_mdm.MDMAppleCommander, | ||
| logger kitlog.Logger, | ||
| verifyTimeout, verifyRequestDelay time.Duration, | ||
| ) func(ctx context.Context, commandResults fleet.MDMCommandResults) error { |
There was a problem hiding this comment.
Do we have a definition for the return type (e.g., MDMCommandResultsFunc)?
| RefetchDeviceCommandUUIDPrefix = RefetchBaseCommandUUIDPrefix + "DEVICE-" | ||
| RefetchAppsCommandUUIDPrefix = RefetchBaseCommandUUIDPrefix + "APPS-" | ||
| RefetchCertsCommandUUIDPrefix = RefetchBaseCommandUUIDPrefix + "CERTS-" | ||
| RefetchVPPAppInstallsCommandUUIDPrefix = RefetchBaseCommandUUIDPrefix + "VPP-INSTALLS-" |
There was a problem hiding this comment.
Maybe we could use a different prefix for verify commands (e.g., "VERIFY-" rather than "REFETCH-")
| return nil, nil | ||
| } | ||
|
|
||
| type InstalledApplicationListResult interface { |
There was a problem hiding this comment.
Would it make sense for these types to be in the fleet package? Or perhaps we could be consolidate things under server/mdm/apple in a separate file (e.g., vpp_verifier similar to profile_verifier)?
| "github.com/google/uuid" | ||
| ) | ||
|
|
||
| const VPPVerificationJobName = "vpp_verification" |
There was a problem hiding this comment.
Nit: Maybe the job is apple_software and the task is verify_vpp_installs since the worker/job is concerned primarily with sending the list applications command?
|
Feel free to address my comments as you see fit in your follow up PRs |
> Fixes #29851 > Fixes #29902 > Mainly followups from #30295, plus improved integration testing # Checklist for submitter If some of the following don't apply, delete the relevant line. <!-- Note that API documentation changes are now addressed by the product design team. --> - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated automated tests - [x] Manual QA for all new/changed functionality
4 participants
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)COLLATE utf8mb4_unicode_ci).