fleetctl/generate-gitops: warn on missing MaintainedApp instead of aborting#43837
Open
SAY-5 wants to merge 1 commit intofleetdm:mainfrom
Open
fleetctl/generate-gitops: warn on missing MaintainedApp instead of aborting#43837SAY-5 wants to merge 1 commit intofleetdm:mainfrom
SAY-5 wants to merge 1 commit intofleetdm:mainfrom
Conversation
…orting When a patch-software policy references a maintained app id that is no longer in the datastore (the policy was authored before the app got cleaned up), generatePolicies returned the `Resource Not Found: MaintainedApp was not found in the datastore` error straight up to the top-level command, killing the whole `fleetctl generate-gitops` export for every team - not just the team with the stale reference. No files are produced and the error message doesn't identify the offending policy (fleetdm#43770). Check for fleet.IsNotFound explicitly: log a per-policy warning that names the policy and the unresolved maintained-app id, omit the fleet_maintained_app_slug field from the generated spec, and keep processing. Any other error still bubbles up unchanged so genuine datastore failures don't silently mask data loss. Signed-off-by: SAY-5 <[email protected]> Fixes fleetdm#43770
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThe pull request modifies 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/fleetctl/fleetctl/generate_gitops.go`:
- Around line 1515-1528: In the switch handling the lookup of the MaintainedApp
(the block that currently sets policySpec["fleet_maintained_app_slug"] when err
== nil and emits a warning on fleet.IsNotFound(err)), always emit the
fleet_maintained_app_slug key even on the not-found branch: set
policySpec["fleet_maintained_app_slug"] to an explicit empty string (or
placeholder) inside the fleet.IsNotFound(err) case, keep the existing warning to
ErrWriter, and retain the default branch to return other errors; this ensures
policySpec contains the key for discovery while preserving current error
handling for fma lookups (referencing policySpec, fleet_maintained_app_slug,
fma, and cachedSWTitle.MaintainedAppID).
🪄 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: 3ee8507e-ecc9-47e4-96d2-df947d5f4d9e
📒 Files selected for processing (2)
changes/43770-generate-gitops-missing-maintained-appcmd/fleetctl/fleetctl/generate_gitops.go
Comment on lines
+1515
to
1528
| switch { | ||
| case err == nil: | ||
| policySpec["fleet_maintained_app_slug"] = fma.Slug | ||
| case fleet.IsNotFound(err): | ||
| // The policy references a MaintainedApp that no longer exists | ||
| // (e.g. deleted after the policy was authored). Warn and keep | ||
| // generating the rest of the export instead of aborting the | ||
| // whole `fleetctl generate-gitops` run (#43770); the operator | ||
| // can clean the stale reference up post-export. | ||
| fmt.Fprintf(cmd.CLI.App.ErrWriter, | ||
| "Warning: policy %q references maintained app %d which is no longer in the datastore; omitting fleet_maintained_app_slug.\n", | ||
| policy.Name, cachedSWTitle.MaintainedAppID) | ||
| default: | ||
| return nil, err |
Contributor
There was a problem hiding this comment.
Keep emitting fleet_maintained_app_slug in the stale-reference case.
The new not-found branch avoids aborting, but omitting the field hides the required GitOps key from the generated policy. The provided context notes fleet_maintained_app_slug is part of PolicySpec in server/fleet/policies.go:481-485 and required by patch-policy validation, so leaving a TODO/blank field is safer for discoverability and follow-up fixes.
Proposed adjustment
switch {
case err == nil:
policySpec["fleet_maintained_app_slug"] = fma.Slug
case fleet.IsNotFound(err):
// The policy references a MaintainedApp that no longer exists
// (e.g. deleted after the policy was authored). Warn and keep
// generating the rest of the export instead of aborting the
// whole `fleetctl generate-gitops` run (`#43770`); the operator
// can clean the stale reference up post-export.
+ policySpec["fleet_maintained_app_slug"] = cmd.AddComment(filePath,
+ fmt.Sprintf("TODO: Resolve missing Fleet-maintained app %d for policy %q", cachedSWTitle.MaintainedAppID, policy.Name))
fmt.Fprintf(cmd.CLI.App.ErrWriter,
- "Warning: policy %q references maintained app %d which is no longer in the datastore; omitting fleet_maintained_app_slug.\n",
+ "Warning: policy %q references maintained app %d which is no longer in the datastore; leaving fleet_maintained_app_slug as a TODO.\n",
policy.Name, cachedSWTitle.MaintainedAppID)
default:
return nil, err
}Based on learnings, The generate-gitops command in fleetctl should always emit all fields, even when they are empty, so that users can see what configuration options are available for discovery purposes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| switch { | |
| case err == nil: | |
| policySpec["fleet_maintained_app_slug"] = fma.Slug | |
| case fleet.IsNotFound(err): | |
| // The policy references a MaintainedApp that no longer exists | |
| // (e.g. deleted after the policy was authored). Warn and keep | |
| // generating the rest of the export instead of aborting the | |
| // whole `fleetctl generate-gitops` run (#43770); the operator | |
| // can clean the stale reference up post-export. | |
| fmt.Fprintf(cmd.CLI.App.ErrWriter, | |
| "Warning: policy %q references maintained app %d which is no longer in the datastore; omitting fleet_maintained_app_slug.\n", | |
| policy.Name, cachedSWTitle.MaintainedAppID) | |
| default: | |
| return nil, err | |
| switch { | |
| case err == nil: | |
| policySpec["fleet_maintained_app_slug"] = fma.Slug | |
| case fleet.IsNotFound(err): | |
| // The policy references a MaintainedApp that no longer exists | |
| // (e.g. deleted after the policy was authored). Warn and keep | |
| // generating the rest of the export instead of aborting the | |
| // whole `fleetctl generate-gitops` run (`#43770`); the operator | |
| // can clean the stale reference up post-export. | |
| policySpec["fleet_maintained_app_slug"] = cmd.AddComment(filePath, | |
| fmt.Sprintf("TODO: Resolve missing Fleet-maintained app %d for policy %q", cachedSWTitle.MaintainedAppID, policy.Name)) | |
| fmt.Fprintf(cmd.CLI.App.ErrWriter, | |
| "Warning: policy %q references maintained app %d which is no longer in the datastore; leaving fleet_maintained_app_slug as a TODO.\n", | |
| policy.Name, cachedSWTitle.MaintainedAppID) | |
| default: | |
| return nil, err |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 1515 - 1528, In the
switch handling the lookup of the MaintainedApp (the block that currently sets
policySpec["fleet_maintained_app_slug"] when err == nil and emits a warning on
fleet.IsNotFound(err)), always emit the fleet_maintained_app_slug key even on
the not-found branch: set policySpec["fleet_maintained_app_slug"] to an explicit
empty string (or placeholder) inside the fleet.IsNotFound(err) case, keep the
existing warning to ErrWriter, and retain the default branch to return other
errors; this ensures policySpec contains the key for discovery while preserving
current error handling for fma lookups (referencing policySpec,
fleet_maintained_app_slug, fma, and cachedSWTitle.MaintainedAppID).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43837 +/- ##
==========================================
- Coverage 66.91% 66.91% -0.01%
==========================================
Files 2601 2601
Lines 208982 208998 +16
Branches 9191 9191
==========================================
+ Hits 139842 139848 +6
- Misses 56398 56408 +10
Partials 12742 12742
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #43770.
When a patch-software policy references a
MaintainedAppid that is no longer in the datastore (the policy was authored before the app got cleaned up),generatePoliciesreturns theResource Not Found: MaintainedApp was not found in the datastoreerror straight up to the top-level command. That kills the wholefleetctl generate-gitopsexport for every team - not just the team with the stale reference - and no files are produced. The error message doesn't name the offending policy either, so operators have no way to identify what to clean up.Fix
Check for
fleet.IsNotFoundexplicitly in thePatchSoftwarebranch:fleet_maintained_app_slugfrom the generated spec for that policyAny other error still bubbles up unchanged so genuine datastore failures don't silently mask data loss.
Test plan
go build ./cmd/fleetctl/fleetctl/go test ./cmd/fleetctl/fleetctl/ -run "TestGenerateGitops|TestPolicies|TestGitops" -count=1(all pass)Added the change file under
changes/43770-generate-gitops-missing-maintained-app.Summary by CodeRabbit
Bug Fixes
fleetctl generate-gitopsnow warns and continues when encountering policies referencing missing maintained apps, instead of aborting the export.Documentation