Skip VPP label validation in dry runs - #46106
Conversation
| } | ||
|
|
||
| // test vpp apps and packages dont validate labels on a dry run (issue #45844) | ||
| func (s *enterpriseIntegrationGitopsTestSuite) TestGitOpsTeamLabelAndSoftwareSameApply() { |
There was a problem hiding this comment.
I'm conflicted on if this test is actually necessary or not, but decided to include it and also test custom packages here.
There was a problem hiding this comment.
I think this is fine to have to catch this issue in the future if we end up removing it by accident etc.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46106 +/- ##
==========================================
- Coverage 66.82% 66.82% -0.01%
==========================================
Files 2754 2754
Lines 220158 220160 +2
Branches 10996 10996
==========================================
+ Hits 147131 147132 +1
Misses 59733 59733
- Partials 13294 13295 +1
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:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughThis PR resolves an issue where 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/server/service/vpp.go`:
- Around line 335-340: The current gating of ValidateSoftwareLabels behind if
!dryRun skips structural validation (mutual-exclusivity of
payload.LabelsIncludeAny / LabelsExcludeAny / LabelsIncludeAll) during dry-run;
change the call so structural validation always runs: either call
ValidateSoftwareLabels unconditionally (keeping the existing ctx, svc, teamID,
payload.* and assigning validatedLabels) or refactor ValidateSoftwareLabels to
accept a flag (e.g., skipDBChecks bool) and invoke it with skipDBChecks=true for
dryRun so DB existence checks are skipped but mutual-exclusivity and other
structural checks still run; keep the existing error wrapping (ctxerr.Wrap) and
return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48467db9-31ea-47ae-b24e-9b5ec1acf4a2
📒 Files selected for processing (3)
changes/45844-gitops-vpp-dry-run-label-validationcmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.goee/server/service/vpp.go
| if !dryRun { | ||
| validatedLabels, err = ValidateSoftwareLabels(ctx, svc, teamID, payload.LabelsIncludeAny, payload.LabelsExcludeAny, payload.LabelsIncludeAll) | ||
| if err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "validating software labels for batch adding vpp app") | ||
| } | ||
| } |
There was a problem hiding this comment.
Dry-run now skips structural label validation, not just DB existence checks.
On Line 335, gating the full ValidateSoftwareLabels call behind !dryRun also skips the mutual-exclusivity check (labels_include_any vs labels_exclude_any vs labels_include_all). That means invalid payloads can pass dry-run but fail on real apply.
Suggested fix
var validatedLabels *fleet.LabelIdentsWithScope
- if !dryRun {
+ if dryRun {
+ // Keep dry-run fidelity for payload shape while skipping DB existence checks.
+ sets := 0
+ if len(payload.LabelsIncludeAny) > 0 {
+ sets++
+ }
+ if len(payload.LabelsExcludeAny) > 0 {
+ sets++
+ }
+ if len(payload.LabelsIncludeAll) > 0 {
+ sets++
+ }
+ if sets > 1 {
+ return nil, &fleet.BadRequestError{
+ Message: `Only one of "labels_include_all", "labels_include_any" or "labels_exclude_any" can be included.`,
+ }
+ }
+ } else {
validatedLabels, err = ValidateSoftwareLabels(ctx, svc, teamID, payload.LabelsIncludeAny, payload.LabelsExcludeAny, payload.LabelsIncludeAll)
if err != nil {
return nil, ctxerr.Wrap(ctx, err, "validating software labels for batch adding vpp app")
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ee/server/service/vpp.go` around lines 335 - 340, The current gating of
ValidateSoftwareLabels behind if !dryRun skips structural validation
(mutual-exclusivity of payload.LabelsIncludeAny / LabelsExcludeAny /
LabelsIncludeAll) during dry-run; change the call so structural validation
always runs: either call ValidateSoftwareLabels unconditionally (keeping the
existing ctx, svc, teamID, payload.* and assigning validatedLabels) or refactor
ValidateSoftwareLabels to accept a flag (e.g., skipDBChecks bool) and invoke it
with skipDBChecks=true for dryRun so DB existence checks are skipped but
mutual-exclusivity and other structural checks still run; keep the existing
error wrapping (ctxerr.Wrap) and return behavior.
There was a problem hiding this comment.
This seems to also be the case in BatchSetSoftwareInstallers, maybe ValidateSoftwareLabels should take in a dry_run parameter. Not addressing in this PR though.
2 participants
Related issue: Resolves #45844
Skips label validation against the database in dry runs, because if new ones are being applied in the same run then they wouldnt be in the db ahead of time.
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.
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
Release Notes
Bug Fixes