Don't validate software/profile labels in dry run mode #28201
Conversation
| validatedLabels, err := ValidateSoftwareLabels(ctx, svc, payload.LabelsIncludeAny, payload.LabelsExcludeAny) | ||
| if err != nil { | ||
| return "", err | ||
| if !dryRun { | ||
| validatedLabels, err := ValidateSoftwareLabels(ctx, svc, payload.LabelsIncludeAny, payload.LabelsExcludeAny) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| payload.ValidatedLabels = validatedLabels | ||
| } | ||
| payload.ValidatedLabels = validatedLabels |
There was a problem hiding this comment.
payload.ValidatedLabels is a pointer, and there's nil checks for it, so it's ok if this isn't populated in dry run mode.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #28201 +/- ##
==========================================
+ Coverage 63.97% 63.99% +0.02%
==========================================
Files 1779 1780 +1
Lines 170499 170731 +232
Branches 4952 4952
==========================================
+ Hits 109078 109261 +183
- Misses 52830 52855 +25
- Partials 8591 8615 +24
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:
|
| ds.LabelIDsByNameFunc = func(ctx context.Context, labels []string) (map[string]uint, error) { | ||
| m := map[string]uint{} | ||
| for _, label := range labels { | ||
| labelID++ | ||
| m[label] = labelID | ||
| if label != "baddy" { | ||
| labelID++ | ||
| m[label] = labelID | ||
| } | ||
| } | ||
| return m, nil | ||
| } |
There was a problem hiding this comment.
this mock just spit out IDs for any label you asked for; adding a carveout for the "baddy" label so we can test what happens when an unknown label is supplied when adding a profile
| {"team BitLocker profile", 1, | ||
| { | ||
| "team BitLocker profile", 1, | ||
| `<Replace><Item><Target><LocURI>./Device/Vendor/MSFT/BitLocker/AllowStandardUserEncryption</LocURI></Target></Item></Replace>`, true, | ||
| syncml.DiskEncryptionProfileRestrictionErrMsg}, | ||
| syncml.DiskEncryptionProfileRestrictionErrMsg, | ||
| }, |
There was a problem hiding this comment.
this and the above are just formatting changes from prettier
The issue affects dry-run only, because the labels aren't actually created in the run (nothing is created, because it's a dry run). But fleet/server/service/client.go Lines 2145 to 2155 in 11e8ed2 |
Thanks, updated! |
sgress454
deleted the
sgress454/28154-dont-validate-labels-in-dry-run-mode
branch
For #28154
This PR fixes a bug where GitOps dry runs would fail when software installers or profiles referenced labels that were created in the same run. The issue is that GitOps utilizes the real APIs for batch software/profile creation for validation, sending a
dryRunflag to prevent those APIs from actually writing data. In dry run mode, no labels are actually created, so validation checks for "don't use labels that don't exist" will always fail when new labels are referenced. Recent updates to GitOps have given it the ability to validate the labels itself, removing the need to use the API for this check.I added a new test for this in the mdm profiles tests. The test suite for software installers is a little more challenging to update for this case, and since it's not a happy path test I'm not prioritizing it, but will try to add one time permitting.