Conversation
cmd/fleetctl/fleetctl/templates/new/.github/fleet-gitops/action.template.yml
Outdated
Show resolved
Hide resolved
cmd/fleetctl/fleetctl/templates/new/.github/workflows/workflow.template.yml
Show resolved
Hide resolved
cmd/fleetctl/fleetctl/templates/new/fleets/personal-mobile-devices.template.yml
Show resolved
Hide resolved
cmd/fleetctl/fleetctl/templates/new/.github/workflows/workflow.template.yml
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe PR introduces the Possibly related PRs
🚥 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)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/spec/gitops.go (1)
1189-1205:⚠️ Potential issue | 🟠 Major
pathentries currently bypassAllowedExtensionschecks.
pathsglob expansion filters by extension, but on Line 1189+ directpathitems skip that validation. This allows unsupported file types through and makes behavior inconsistent.💡 Suggested patch
case hasPath: if containsGlobMeta(*baseItem.Path) { errs = append(errs, fmt.Errorf(`%s "path" %q contains glob characters; use "paths" for glob patterns`, entityType, *baseItem.Path)) continue } resolved := resolveApplyRelativePath(baseDir, *baseItem.Path) + ext := strings.ToLower(filepath.Ext(resolved)) + if !opts.AllowedExtensions[ext] { + errs = append(errs, fmt.Errorf( + `%s "path" %q has unsupported extension %q`, + entityType, *baseItem.Path, ext, + )) + continue + } // Check for duplicate filenames if requested. if opts.RequireUniqueBasenames { base := filepath.Base(resolved) if existing, ok := seenBasenames[base]; ok {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/spec/gitops.go` around lines 1189 - 1205, The code path under the hasPath case bypasses AllowedExtensions validation (while the paths glob branch performs extension filtering); modify the hasPath handling to validate the resolved path's file extension against the same AllowedExtensions logic used for glob expansion: after resolveApplyRelativePath(baseDir, *baseItem.Path) and before checking RequireUniqueBasenames or appending the entity, check whether the resolved filename's extension is permitted (reuse the same extension-check helper or the same list from opts.AllowedExtensions), and if not append an error to errs and continue; keep the existing duplicate basename logic (seenBasenames) and then call PT(&entity).SetBaseItem and append to result only when the extension is allowed.
🧹 Nitpick comments (1)
pkg/spec/gitops_test.go (1)
2155-2274: Add a direct-pathextension regression test for profile parsing.These tests cover extension filtering for
pathsglobs well, but not for single-filepathentries. A small test for invalid directpath(for example Android.xml) would lock in expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/spec/gitops_test.go` around lines 2155 - 2274, The tests in TestGitOpsGlobProfiles exercise extension filtering for globbed "paths" but don't assert behavior for explicit single-file "path" entries; add a new subtest inside TestGitOpsGlobProfiles that creates a profiles dir with a single-file (e.g., an Android .xml), writes a gitops YAML using a direct "path: profiles/foo.xml" under android_settings.configuration_profiles, calls GitOpsFromFile, and asserts that result.Controls.AndroidSettings (AndroidSettings.CustomSettings) either excludes the invalid extension or reports expected invalid/empty Value—use the same patterns and helpers already in the test (dir := t.TempDir(), require.NoError, GitOpsFromFile, assertions against androidSettings.CustomSettings.Value and .Valid) to lock in regression behavior. Ensure the new subtest name indicates it's for direct-path extension validation and mirrors existing test structure (t.Run, t.Parallel()).
🤖 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/new.go`:
- Around line 56-60: The npm version lookup currently uses exec.Command which
can hang; change it to use exec.CommandContext with a context created by
context.WithTimeout (e.g., 3–5s) so the call is cancelled on slow networks, and
handle the context timeout error the same as the existing fallback to "latest";
update the block that currently calls exec.Command("npm", "view", "fleetctl",
"version") to create ctx, defer cancel, call exec.CommandContext(ctx, ...), then
keep the existing logic that trims output and checks semverPattern before
returning the version.
In `@cmd/fleetctl/fleetctl/templates/new/default.template.yml`:
- Line 50: Replace the typo "single-sign on" in the comment in
default.template.yml with the correct phrasing "single sign-on" so the end-user
guidance reads "Uncomment to use single sign-on (SSO) to authenticate". Locate
the comment containing the string "single-sign on" and update only the wording,
preserving the rest of the comment formatting and context.
In `@server/fleet/mdm.go`:
- Around line 681-682: MDMProfileSpecsMatch currently keys profile specs using
only the Path field causing collisions when Paths is used; update the matching
logic in MDMProfileSpecsMatch to derive a stable key that uses Paths when
non-empty (e.g., sort and join the Paths slice into a single string) else falls
back to Path, and use that key for map lookups and comparisons so entries with
empty Path and non-empty Paths no longer collide.
---
Outside diff comments:
In `@pkg/spec/gitops.go`:
- Around line 1189-1205: The code path under the hasPath case bypasses
AllowedExtensions validation (while the paths glob branch performs extension
filtering); modify the hasPath handling to validate the resolved path's file
extension against the same AllowedExtensions logic used for glob expansion:
after resolveApplyRelativePath(baseDir, *baseItem.Path) and before checking
RequireUniqueBasenames or appending the entity, check whether the resolved
filename's extension is permitted (reuse the same extension-check helper or the
same list from opts.AllowedExtensions), and if not append an error to errs and
continue; keep the existing duplicate basename logic (seenBasenames) and then
call PT(&entity).SetBaseItem and append to result only when the extension is
allowed.
---
Nitpick comments:
In `@pkg/spec/gitops_test.go`:
- Around line 2155-2274: The tests in TestGitOpsGlobProfiles exercise extension
filtering for globbed "paths" but don't assert behavior for explicit single-file
"path" entries; add a new subtest inside TestGitOpsGlobProfiles that creates a
profiles dir with a single-file (e.g., an Android .xml), writes a gitops YAML
using a direct "path: profiles/foo.xml" under
android_settings.configuration_profiles, calls GitOpsFromFile, and asserts that
result.Controls.AndroidSettings (AndroidSettings.CustomSettings) either excludes
the invalid extension or reports expected invalid/empty Value—use the same
patterns and helpers already in the test (dir := t.TempDir(), require.NoError,
GitOpsFromFile, assertions against androidSettings.CustomSettings.Value and
.Valid) to lock in regression behavior. Ensure the new subtest name indicates
it's for direct-path extension validation and mirrors existing test structure
(t.Run, t.Parallel()).
🪄 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: 40d739b0-676f-4e00-8d62-fb97b894ee61
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
changes/41345-add-fleetctl-newcmd/fleetctl/fleetctl/fleetctl.gocmd/fleetctl/fleetctl/new.gocmd/fleetctl/fleetctl/new_test.gocmd/fleetctl/fleetctl/templates/new/.github/fleet-gitops/action.template.ymlcmd/fleetctl/fleetctl/templates/new/.github/fleet-gitops/gitops.shcmd/fleetctl/fleetctl/templates/new/.github/workflows/workflow.template.ymlcmd/fleetctl/fleetctl/templates/new/.gitignorecmd/fleetctl/fleetctl/templates/new/.gitlab-ci.template.ymlcmd/fleetctl/fleetctl/templates/new/README.mdcmd/fleetctl/fleetctl/templates/new/default.template.ymlcmd/fleetctl/fleetctl/templates/new/fleets/personal-mobile-devices.template.ymlcmd/fleetctl/fleetctl/templates/new/fleets/workstations.template.ymlcmd/fleetctl/fleetctl/templates/new/labels/apple-silicon-macos-hosts.template.ymlcmd/fleetctl/fleetctl/templates/new/labels/arm-based-windows-hosts.template.ymlcmd/fleetctl/fleetctl/templates/new/labels/debian-based-linux-hosts.template.ymlcmd/fleetctl/fleetctl/templates/new/labels/x86-based-windows-hosts.template.ymlcmd/fleetctl/fleetctl/templates/new/platforms/all/icons/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/all/policies/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/all/reports/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/android/configuration-profiles/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/android/managed-app-configurations/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/ios/configuration-profiles/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/ios/declaration-profiles/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/ipados/configuration-profiles/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/ipados/declaration-profiles/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/linux/policies/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/linux/reports/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/linux/scripts/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/linux/software/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/macos/commands/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/macos/configuration-profiles/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/macos/declaration-profiles/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/macos/enrollment-profiles/automatic-enrollment.dep.jsoncmd/fleetctl/fleetctl/templates/new/platforms/macos/policies/all-software-updates-installed.template.ymlcmd/fleetctl/fleetctl/templates/new/platforms/macos/reports/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/macos/scripts/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/macos/software/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/windows/configuration-profiles/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/windows/policies/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/windows/reports/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/windows/scripts/.gitkeepcmd/fleetctl/fleetctl/templates/new/platforms/windows/software/.gitkeepgo.modpkg/spec/gitops.gopkg/spec/gitops_test.goserver/fleet/mdm.goserver/fleet/spec.gotools/cloner-check/generated_files/appconfig.txttools/cloner-check/generated_files/mdmprofilespec.txttools/cloner-check/generated_files/teamconfig.txttools/cloner-check/generated_files/teammdm.txtwebsite/.sailsrcwebsite/generators/gitops/README.mdwebsite/generators/gitops/index.js
💤 Files with no reviewable changes (2)
- website/generators/gitops/README.md
- website/generators/gitops/index.js
|
@getvictor Nice example of different review bots finding different things. |
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Reviewed alphabetically through everything in cmd. Will pick up later.
cmd/fleetctl/fleetctl/new.go
Outdated
| // Escape for safe inclusion in double-quoted YAML strings. | ||
| yamlOrgName := strings.ReplaceAll(orgName, `\`, `\\`) | ||
| yamlOrgName = strings.ReplaceAll(yamlOrgName, `"`, `\"`) |
There was a problem hiding this comment.
This feels like we should be using a templating/escaping library for this. Maybe this isn't possible because Go doesn't have one that operates like test/html template stdlib, and YAML escapers expect you to serialize (which we can't do because comments and field order). But this has my spidey senses tingling.
There was a problem hiding this comment.
I could remove the quotes from the template and just do:
org_name: <%= org_name %>
and then use the YAML serializer on them if that feels better. I don't think any template library is gonna do it for us.
There was a problem hiding this comment.
Done. One nit is it uses single quotes (when it uses quotes at all), where we default to double-quotes in all our examples. I could get around this by using JSON marshaling instead, but then it'd double-quote everything and do unnecessary escaping. So I'm leaving as-is.
cmd/fleetctl/fleetctl/new_test.go
Outdated
| outDir := filepath.Join(dir, "ctrl-only") | ||
| _, err := runNewCommand(t, "--org-name", "\x01\x02\x03", "--dir", outDir) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "organization name is required") |
There was a problem hiding this comment.
This feels like we could do table driven tests, including an assertion that the dir wasn't created etc., or was created with the correct org name.
| @@ -0,0 +1,27 @@ | |||
| package fleet | |||
There was a problem hiding this comment.
Nit: Feels moving this into the fleet megapackage is a (smallish) step backwards, but I understand why you did it (mdm.go). Probably more trouble than it's worth to extract it into its own thing.
There was a problem hiding this comment.
I don't disagree. Maybe pkg/glob?
There was a problem hiding this comment.
then again it's 30 lines of code and it'll probably never be more than that 🤔 . I think I'll leave it for now, and if this becomes a reason why we can't drop the fleet import from something we can move it out.
There was a problem hiding this comment.
and i couldn't think of a good name for it
Related issue: Resolves #41345
Details
This PR:
fleetctl newcommand which creates a starter GitOps repo file structureconfiguration_profiles:key in GitOps, to support its use in thefleetctl newtemplates. This involved moving theBaseItemtype andSupportsFileIncludeinterface into thefleetpackage so that theMDMProfileSpectype could implement the interface and do glob expansion.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
fleetctl newfleetctl newwith no args prompted for org name and created a newit-and-securityfolder under current folder w/ correct filesfleetctl new --dir /tmp/testnewcreated correct files under/tmp/testnewfleetctl new --dir /tmp/testexisting --forcewith an existing/tmp/testexistingfolder created correct files under/tmp/testexistingfleetctl new --org-name=foocreated correct files underit-and-securitywithout prompting for org namepaths:inconfiguration_profilespicks up multiple matching profilespaths:+path:inconfiguration_profileswill error if the same profile is picked up twiceSummary by CodeRabbit
New Features
fleetctl newcommand to initialize GitOps repository structure via CLI.configuration_profilesfield, enabling flexible profile selection.Chores