fix gitops relative paths for unassigned and org_settings#47512
fix gitops relative paths for unassigned and org_settings#47512MagnusHJensen wants to merge 1 commit into
Conversation
WalkthroughThis PR fixes GitOps relative path resolution failures when custom setup profiles are added to unassigned.yml. The fix introduces path-anchoring helpers in 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
pkg/spec/gitops_test.go (1)
1371-1464: ⚡ Quick winAdd coverage for nested
yara_rules[].pathre-anchoring.
reanchorOrgSettingsPathsalso rewritesorg_settings.yara_rules[].path, but this regression test currently validates onlyorg_infoandmdmpath fields. Adding one focused subtest here would lock the new branch.🧪 Suggested test addition
+ t.Run("yara_rules paths resolve relative to nested file", func(t *testing.T) { + gitops, tmpDir := setup(t, ` +server_settings: + server_url: https://fleet.example.com +org_info: + contact_url: https://example.com/contact + org_name: Test Org +yara_rules: + - path: ../yara/rule.yar +secrets: +`) + rules := gitops.OrgSettings["yara_rules"].([]any) + rule := rules[0].(map[string]any) + assert.Equal(t, filepath.Join(tmpDir, "yara/rule.yar"), rule["path"]) + })🤖 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 `@pkg/spec/gitops_test.go` around lines 1371 - 1464, Add a new subtest inside TestGitOpsOrgSettingsNestedPathResolution that verifies reanchoring for org_settings.yara_rules[].path: create nested settings via the existing setup helper with an org.yml containing yara_rules: - path: ../yara/rule.yar, call GitOpsFromFile through setup, then extract gitops.OrgSettings["yara_rules"] (assert cast to []any -> map[string]any) and assert the "path" value equals filepath.Join(tmpDir, "yara", "rule.yar"). This mirrors the existing org_info/mdm checks and locks the reanchorOrgSettingsPaths behavior for yara_rules[].path.
🤖 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.
Nitpick comments:
In `@pkg/spec/gitops_test.go`:
- Around line 1371-1464: Add a new subtest inside
TestGitOpsOrgSettingsNestedPathResolution that verifies reanchoring for
org_settings.yara_rules[].path: create nested settings via the existing setup
helper with an org.yml containing yara_rules: - path: ../yara/rule.yar, call
GitOpsFromFile through setup, then extract gitops.OrgSettings["yara_rules"]
(assert cast to []any -> map[string]any) and assert the "path" value equals
filepath.Join(tmpDir, "yara", "rule.yar"). This mirrors the existing
org_info/mdm checks and locks the reanchorOrgSettingsPaths behavior for
yara_rules[].path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f865dfb-5702-48fd-a79f-6aa4c846f38e
📒 Files selected for processing (5)
changes/45661-fix-gitops-relative-pathscmd/fleetctl/fleetctl/gitops.gocmd/fleetctl/fleetctl/gitops_test.gopkg/spec/gitops.gopkg/spec/gitops_test.go
There was a problem hiding this comment.
Pull request overview
Fixes GitOps relative path resolution edge cases where file-path fields defined in unassigned.yml / no-team.yml (and nested org_settings.path) were later re-resolved against a different base directory, causing “no such file or directory” failures.
Changes:
- Reanchors relative paths inside nested
org_settings.pathcontent so they resolve from the nested file’s directory. - Adds
GitOpsControls.ResolveFilePathsAbsand applies it when extracting unassigned/no-team controls so apply-time path resolution doesn’t double-anchor. - Adds unit/integration test coverage for issue #45661 and a user-visible change entry.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/spec/gitops.go | Reanchors nested org_settings path fields and introduces absolute-path resolution for certain controls fields. |
| pkg/spec/gitops_test.go | Adds focused tests for nested org_settings path reanchoring and no-team controls path behavior. |
| cmd/fleetctl/fleetctl/gitops.go | Resolves no-team/unassigned controls’ apply-time file paths to absolute paths during extraction. |
| cmd/fleetctl/fleetctl/gitops_test.go | Adds end-to-end regression tests reproducing the unassigned/no-team relative path failures. |
| changes/45661-fix-gitops-relative-paths | Documents the user-visible GitOps fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| c.MacOSSetup.MacOSSetupAssistant.Value = absPathFrom(baseDir, c.MacOSSetup.MacOSSetupAssistant.Value) | ||
| c.MacOSSetup.Script.Value = absPathFrom(baseDir, c.MacOSSetup.Script.Value) |
| // ../ from fleets/ climbs out of fleets/, so the result must not contain it. | ||
| assert.NotContains(t, got, "fleets", "path should not retain the fleets/ segment: %q", got) | ||
| } | ||
| assert.True(t, strings.HasSuffix(controls.MacOSSetup.MacOSSetupAssistant.Value, "lib/no-team/macos_enrollment.json")) |
| assert.True(t, filepath.IsAbs(got), "expected absolute path, got %q", got) | ||
| assert.True(t, strings.HasSuffix(got, "generated/lib/no-team/macos_enrollment.json"), "got %q", got) | ||
| assert.Equal(t, 1, strings.Count(got, "generated/lib"), "the generated/ segment must appear once, not be double-anchored: %q", got) |
| @@ -0,0 +1 @@ | |||
| - Fixed GitOps relative path lookup for controls.setup_experience.(apple_setup_assistant, macos_script, software.package_path) in unassigned.yml, and org_logo_paths under org_settings. No newline at end of file | |||
| assert.True(t, filepath.IsAbs(got), "expected absolute path, got %q", got) | ||
| // ../ from fleets/ climbs out of fleets/, so the result must not contain it. | ||
| assert.NotContains(t, got, "fleets", "path should not retain the fleets/ segment: %q", got) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47512 +/- ##
=======================================
Coverage 67.19% 67.19%
=======================================
Files 3489 3489
Lines 228536 228612 +76
Branches 11872 11872
=======================================
+ Hits 153557 153622 +65
- Misses 61154 61164 +10
- Partials 13825 13826 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Related issue: Resolves #45661
I couldn't really find another good solution that would solve it all, as the path resolution is spread out, plus unassigned merging into global config definitely makes it more complex (root cause of the issue).
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary by CodeRabbit