Move software self-service, labels, categories, and setup experience to team level in GitOps - #32245
Conversation
…to team level in GitOps This also enables setup experience inclusion for FMAs. Includes checks and errors for invalid states. TODO: error tests for setup experience defined in two places, fields defined in child objects that should be defined in parent
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 30095-gitops-changes #32245 +/- ##
=======================================================
Coverage ? 62.08%
=======================================================
Files ? 1985
Lines ? 194089
Branches ? 6444
=======================================================
Hits ? 120500
Misses ? 64011
Partials ? 9578
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:
|
… for setup experience flag)
Caught in manual self-QA
Co-authored-by: Anthony Maxwell <133805840+Illbjorn@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughMoved software attributes (labels, categories, self_service, setup_experience) from package files to team-level YAML, added hydration to propagate them to package-level, and enforced mutual exclusivity of setup_experience between macos_setup and per-software entries. Updated fleetctl GitOps parsing/tests, adjusted specs, and modified server models and services accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (YAML)
participant fleetctl as fleetctl gitops
participant Spec as Spec Parser
participant Service as Service Client
participant Server as Fleet API
User->>fleetctl: Provide team YAML (labels, categories, self_service, setup_experience)
fleetctl->>Spec: Parse team-level software definitions
Spec->>Spec: Validate package files exclude team-only fields
Spec->>Spec: Hydrate package-level spec with team-level fields
Spec-->>fleetctl: Package specs (hydrated)
fleetctl->>Service: Build payloads (packages, maintained apps, VPP apps)
Service->>Service: Check setup_experience mutual exclusivity (macos_setup vs software)
alt Exclusive OK
Service->>Server: Apply team software config
Server-->>Service: 200 OK
Service-->>fleetctl: Success
else Conflict
Service-->>fleetctl: Error: setup_experience specified in both places
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
♻️ Duplicate comments (3)
server/service/client.go (2)
1112-1118: Good guard: enforce mutual exclusivity for setup experience (packages/FMA).Same suggestion to reuse a shared const for the error message to avoid drift.
- return nil, fmt.Errorf("Couldn't edit software (%s). Setup experience may only be specified directly on software or within macos_setup, but not both. See https://fleetdm.com/learn-more-about/yaml-software-setup-experience.", si.URL) + return nil, fmt.Errorf("%s", errSetupExperienceMutualExcl)
2178-2184: Good guard: enforce mutual exclusivity for setup experience (No team VPP apps).Matches team path; reuse the shared const for consistency.
- return nil, nil, errors.New("Couldn't edit app store apps. Setup experience may only be specified directly on software or within macos_setup, but not both. See https://fleetdm.com/learn-more-about/yaml-software-setup-experience.") + return nil, nil, errors.New(errSetupExperienceMutualExcl)pkg/spec/gitops.go (1)
1131-1161: Improve error message typos and clarity when reading package files.There’s a typo in the environment expansion error message.
- multiError = multierror.Append(multiError, fmt.Errorf("failed to expand environmet in file %s: %w", *teamLevelPackage.Path, err)) + multiError = multierror.Append(multiError, fmt.Errorf("failed to expand environment in file %s: %w", *teamLevelPackage.Path, err))Also, great call to gate disallowed fields in package files and then hydrate from team-level.
🧹 Nitpick comments (13)
server/fleet/teams.go (1)
194-196: Add brief doc on new field and confirm tri-state semanticsGood addition. Please add a short comment clarifying that this is optional and only meaningful for macOS setup experience; also confirm downstream code treats “unset” differently from explicit false.
Apply this diff to document intent:
// Categories is the list of names of software categories associated with this VPP app. Categories []string `json:"categories"` - InstallDuringSetup optjson.Bool `json:"setup_experience"` + // InstallDuringSetup indicates the app should be installed during macOS setup (aka setup experience). + // Optional (unset vs. false must be preserved via optjson.Bool). + InstallDuringSetup optjson.Bool `json:"setup_experience"`changes/30095-gitops (1)
1-1: Changelog entry is concise; consider adding explicit key namesAdd the literal keys for clarity (self_service, labels_include_any, labels_exclude_any, categories, setup_experience) to help users search.
pkg/spec/gitops_test.go (4)
201-210: Avoid brittle package identification; handle URL-less Teams (SHA-only) tooRelying on URL substring fails when Teams’ URL is empty (SHA-only case). Identify Teams by URL OR known SHA to keep this assertion stable across both test branches.
Apply:
- for _, pkg := range gitops.Software.Packages { - if strings.Contains(pkg.URL, "MicrosoftTeams") { + for _, pkg := range gitops.Software.Packages { + if strings.Contains(pkg.URL, "MicrosoftTeams") || pkg.SHA256 == teamsSHA { assert.Equal(t, "testdata/lib/uninstall.sh", pkg.UninstallScript.Path) assert.Contains(t, pkg.LabelsIncludeAny, "a") assert.Contains(t, pkg.Categories, "Communication") } else { assert.Empty(t, pkg.UninstallScript.Path) assert.Contains(t, pkg.LabelsExcludeAny, "a") } }Add outside the range (near the start of TestValidGitOpsYaml):
const teamsSHA = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
204-205: Strengthen assertions to catch inverted hydrationAlso assert the opposite label set is empty for Teams to prevent accidental leakage/misplacement.
assert.Equal(t, "testdata/lib/uninstall.sh", pkg.UninstallScript.Path) assert.Contains(t, pkg.LabelsIncludeAny, "a") assert.Contains(t, pkg.Categories, "Communication") + require.Empty(t, pkg.LabelsExcludeAny)
208-208: Symmetric negative assertion for non-Teams packagesMirror the check to ensure we don't have IncludeAny populated where ExcludeAny is expected.
assert.Empty(t, pkg.UninstallScript.Path) assert.Contains(t, pkg.LabelsExcludeAny, "a") + require.Empty(t, pkg.LabelsIncludeAny)
318-324: Don’t assert by index; verify install_during_setup across all packagesIndex-based checks are fragile. Loop over packages so the test remains correct if ordering changes.
require.Empty(t, gitops.Software.Packages[0].URL) - require.True(t, gitops.Software.Packages[0].InstallDuringSetup.Value) - require.True(t, gitops.Software.Packages[1].InstallDuringSetup.Value) + for i, pkg := range gitops.Software.Packages { + require.True(t, pkg.InstallDuringSetup.Value, fmt.Sprintf("package[%d] not marked install_during_setup", i)) + }server/service/integration_enterprise_test.go (1)
11690-11714: Minor DRY: reuse a const-ish helper for “true”Reduce repetition/noise by introducing a local var and using it in literals.
@@ - wantSoftwarePackages := []fleet.SoftwarePackageSpec{ + setupTrue := optjson.Bool{Set: true, Value: true} + wantSoftwarePackages := []fleet.SoftwarePackageSpec{ @@ - InstallDuringSetup: optjson.Bool{Set: true, Value: true}, + InstallDuringSetup: setupTrue, @@ - InstallDuringSetup: optjson.Bool{Set: true, Value: true}, + InstallDuringSetup: setupTrue, @@ - wantAppStoreApps := []fleet.TeamSpecAppStoreApp{ + wantAppStoreApps := []fleet.TeamSpecAppStoreApp{ @@ - InstallDuringSetup: optjson.Bool{Set: true, Value: true}, + InstallDuringSetup: setupTrue, @@ - InstallDuringSetup: optjson.Bool{Set: true, Value: true}, + InstallDuringSetup: setupTrue,cmd/fleetctl/integrationtest/gitops/software_test.go (2)
61-63: Avoid brittle match: remove leading space from expected error substringThe leading space in wantErr makes the test fragile. Drop it so ErrorContains matches regardless of incidental whitespace.
Apply this diff:
- {"testdata/gitops/team_setup_software_defined_in_conflicting_places.yml", " Setup experience may only be specified directly on software or within macos_setup, but not both."}, - {"testdata/gitops/team_setup_software_defined_in_conflicting_places_vpp.yml", " Setup experience may only be specified directly on software or within macos_setup, but not both."}, + {"testdata/gitops/team_setup_software_defined_in_conflicting_places.yml", "Setup experience may only be specified directly on software or within macos_setup, but not both."}, + {"testdata/gitops/team_setup_software_defined_in_conflicting_places_vpp.yml", "Setup experience may only be specified directly on software or within macos_setup, but not both."},
27-27: Typo in test name: “Sofware” → “Software”Keeps grepability and consistency.
Apply this diff:
-func TestGitOpsTeamSofwareInstallers(t *testing.T) { +func TestGitOpsTeamSoftwareInstallers(t *testing.T) {server/service/client.go (1)
773-779: Good guard: enforce mutual exclusivity for setup experience (VPP apps).Prevents mixing macos_setup and per-app flags in a team spec. Consider deduplicating the repeated error string into a const to keep the doc URL in one place.
Apply this minimal change locally here (calls shown below); see added const outside this hunk:
- return nil, nil, nil, nil, errors.New("Couldn't edit app store apps. Setup experience may only be specified directly on software or within macos_setup, but not both. See https://fleetdm.com/learn-more-about/yaml-software-setup-experience.") + return nil, nil, nil, nil, errors.New(errSetupExperienceMutualExcl)Additional code to add near other consts in this file:
const errSetupExperienceMutualExcl = "Couldn't edit app store apps. Setup experience may only be specified directly on software or within macos_setup, but not both. See https://fleetdm.com/learn-more-about/yaml-software-setup-experience."server/fleet/software_installer.go (1)
584-588: Package-file validation misses explicitself_service: false.IncludesFieldsDisallowedInPackageFile flags presence of setup_experience via Valid, but
self_serviceuses bool soself_service: falsein a package file won’t be detected and will slip through. If the intent is “field must not be present at package level regardless of value,” consider:
- Switching SelfService to optjson.Bool in SoftwarePackageSpec, or
- Detecting presence during YAML parsing (pkg/spec/gitops.go) by inspecting a raw map for the key and surfacing an error.
Your current approach is otherwise sound.
pkg/spec/gitops.go (2)
1174-1176: Fix “hash_256” typo in validation error.Should be “hash_sha256”.
- multiError = multierror.Append(multiError, fmt.Errorf("hash_256 value %q must be a valid lower-case hex-encoded (64-character) SHA-256 hash value", softwarePackageSpec.SHA256)) + multiError = multierror.Append(multiError, fmt.Errorf("hash_sha256 value %q must be a valid lower-case hex-encoded (64-character) SHA-256 hash value", softwarePackageSpec.SHA256))
709-750: Redundant “item := item” copies are no longer needed in Go ≥1.22.Given Fleet builds with Go 1.24, the loop-var semantics fix is active; these defensive copies can be removed to simplify code (applies similarly in policies/queries loops).
- for _, item := range labels { - item := item + for _, item := range labels {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (24)
changes/30095-gitops(1 hunks)changes/31164-gitops-generate(0 hunks)cmd/fleetctl/fleetctl/gitops.go(1 hunks)cmd/fleetctl/fleetctl/testdata/gitops/lib/software_other.yml(0 hunks)cmd/fleetctl/fleetctl/testdata/gitops/no_team_setup_software_invalid_script.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/gitops/no_team_setup_software_invalid_software_package.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/gitops/no_team_setup_software_invalid_vpp_app.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/gitops/no_team_setup_software_valid.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/gitops/team_setup_software_defined_in_conflicting_places.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/gitops/team_setup_software_defined_in_conflicting_places_vpp.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/gitops/team_setup_software_on_package.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/gitops/team_setup_software_valid.yml(1 hunks)cmd/fleetctl/integrationtest/gitops/software_test.go(1 hunks)pkg/spec/gitops.go(2 hunks)pkg/spec/gitops_test.go(2 hunks)pkg/spec/testdata/microsoft-teams.nourl.pkg.software.yml(0 hunks)pkg/spec/testdata/microsoft-teams.pkg.software.yml(1 hunks)pkg/spec/testdata/team_config.yml(2 hunks)pkg/spec/testdata/team_config_no_paths.yml(2 hunks)pkg/spec/testdata/team_config_only_sha256.yml(2 hunks)server/fleet/software_installer.go(3 hunks)server/fleet/teams.go(1 hunks)server/service/client.go(3 hunks)server/service/integration_enterprise_test.go(1 hunks)
💤 Files with no reviewable changes (3)
- cmd/fleetctl/fleetctl/testdata/gitops/lib/software_other.yml
- pkg/spec/testdata/microsoft-teams.nourl.pkg.software.yml
- changes/31164-gitops-generate
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
cmd/fleetctl/integrationtest/gitops/software_test.gocmd/fleetctl/fleetctl/gitops.gopkg/spec/gitops_test.goserver/service/client.goserver/fleet/teams.goserver/service/integration_enterprise_test.gopkg/spec/gitops.goserver/fleet/software_installer.go
🧠 Learnings (2)
📚 Learning: 2025-07-03T15:13:36.494Z
Learnt from: lucasmrod
PR: fleetdm/fleet#30550
File: server/datastore/mysql/policies_test.go:6368-6378
Timestamp: 2025-07-03T15:13:36.494Z
Learning: Go 1.22 introduced a significant change to loop variable semantics where each iteration of a for loop creates a new variable, automatically fixing the classic closure capture bug that existed in earlier Go versions. Code that captures loop variables in closures no longer needs explicit variable copying in Go 1.22+.
Applied to files:
pkg/spec/gitops.go
📚 Learning: 2025-07-03T15:13:36.494Z
Learnt from: lucasmrod
PR: fleetdm/fleet#30550
File: server/datastore/mysql/policies_test.go:6368-6378
Timestamp: 2025-07-03T15:13:36.494Z
Learning: Go 1.22 introduced a significant change to loop variable semantics where each iteration of a for loop creates a new variable, automatically fixing the classic closure capture bug that existed in earlier Go versions. Code that captures loop variables in closures no longer needs explicit variable copying in Go 1.22+. This fix is only enabled if the module's go.mod file specifies go 1.22 or higher.
Applied to files:
pkg/spec/gitops.go
🔇 Additional comments (27)
cmd/fleetctl/fleetctl/testdata/gitops/no_team_setup_software_valid.yml (1)
16-16: Team-levelself_servicemigration verified
- Confirmed
cmd/fleetctl/fleetctl/testdata/gitops/lib/software_other.ymlcontains noself_serviceentries.- All per-package
self_serviceflags have been removed as intended.Approving code changes.
pkg/spec/testdata/microsoft-teams.pkg.software.yml (1)
5-5: Removal of per-package fields aligns with hydrationOK to drop self_service and categories at package level given team-level hydration.
cmd/fleetctl/fleetctl/testdata/gitops/no_team_setup_software_invalid_software_package.yml (1)
11-11: Intentional invalid path scenario preserved; self_service placement correctThis keeps the test’s invalid package path while moving self_service to team level. LGTM.
cmd/fleetctl/fleetctl/testdata/gitops/no_team_setup_software_invalid_vpp_app.yml (1)
15-15: Verified: No duplicateself_servicedeclarations in package filesI ran a recursive search in
cmd/andpkg/forself_service:insoftware_other.ymland confirmed there are no occurrences. Theself_service: truein your testdata file is the sole declaration, so there’s no conflict during hydration/validation.pkg/spec/testdata/team_config_only_sha256.yml (3)
52-55: Label filters and setup_experience on packages: consistent with objectives.References to label a are defined above; no issues spotted.
Also applies to: 57-60
49-49: No conflicting macOS setup entries presentInspected
pkg/spec/testdata/controls.yml– themacos_setupsection hasbootstrap_package: nulland no per-package install instructions, so there’s no overlap with thesetup_experience: trueentry inteam_config_only_sha256.yml.
8-12: Confirm YAML key for label_membership_type
Top-level label definition looks correct, but the spec parser must recognize thelabel_membership_typekey (not justmembership_type). Please verify:
- Inspect the
fleet.LabelSpecstruct inserver/fleet/labels.goto ensure its LabelMembershipType field has the corresponding YAML tag.- Check the pkg/spec conversion code (
pkg/spec/gitops.go) to confirm it maps thelabel_membership_typekey into the internal model.cmd/fleetctl/fleetctl/testdata/gitops/no_team_setup_software_invalid_script.yml (1)
10-10: No duplicateself_serviceinsoftware_other.ymlRan a repository-wide search for
self_service:in allsoftware_other.ymlfixtures undercmd/andpkg/. Only one file was found:
- cmd/fleetctl/fleetctl/testdata/gitops/lib/software_other.yml — no
self_service:entriesSince there are no duplicate keys, this fixture aligns with the new model and is safe to hydrate.
pkg/spec/testdata/team_config.yml (3)
51-55: labels_include_any and categories on Teams: good coverage for hydration behavior.No action needed.
58-59: labels_exclude_any on Firefox: matches new team-level filtering approach.Looks good.
8-12: Key naming consistency confirmed. All occurrences inpkg/spec/**use thelabel_membership_typekey—no straymembership_typeentries were found. Changes can be approved.cmd/fleetctl/fleetctl/testdata/gitops/team_setup_software_valid.yml (1)
26-26: Verified noself_servicein underlying lib – approving code changes
- Searched
cmd/fleetctl/fleetctl/testdata/gitops/lib/software_other.ymlfor anyself_service:entries; none were found.- The testdata’s
self_service: trueon the second package stands alone and is a valid scenario.cmd/fleetctl/fleetctl/testdata/gitops/team_setup_software_on_package.yml (1)
17-26: Valid positive fixture for per-item setup_experienceLooks good: setup_experience is specified directly on software items (pkg and VPP), with macos_setup only providing a script. This aligns with the mutual-exclusivity rule.
cmd/fleetctl/fleetctl/testdata/gitops/team_setup_software_defined_in_conflicting_places_vpp.yml (1)
12-22: Good conflict fixture (macos_setup.software vs top-level app_store_apps)This should reliably trigger the exclusivity error path and complements the package-based conflict case.
cmd/fleetctl/fleetctl/testdata/gitops/team_setup_software_defined_in_conflicting_places.yml (1)
12-26: Conflict fixture (macos_setup.software vs package setup_experience)Appropriately exercises the mutual-exclusivity validation for setup_experience.
pkg/spec/testdata/team_config_no_paths.yml (2)
65-69: Team-level label definition is correct and referenced belowDefining label “a” here enables include/exclude scoping in the software section and supports hydration to package-level.
153-161: Package scoping via labels and categories aligns with new hydration behaviorUsing labels_include_any and categories here matches the intended move of metadata to team level; ensures downstream resolution and validation paths are exercised.
cmd/fleetctl/fleetctl/gitops.go (3)
472-484: Renames/readability: software package label checks LGTM.Variable rename improves clarity; logic unchanged and correct.
487-499: Renames/readability: App Store app label checks LGTM.Consistent error paths and identifiers; no functional change.
501-513: Renames/readability: Fleet-maintained app label checks LGTM.Consistent with other sections; identifiers (Slug) used appropriately.
server/service/client.go (1)
780-789: Payload mapping LGTM.install_during_setup is correctly propagated with labels/categories preserved.
server/fleet/software_installer.go (3)
549-558: Add setup_experience to SoftwarePackageSpec — LGTM.Field type optjson.Bool is appropriate for presence detection.
598-608: Add setup_experience to MaintainedAppSpec — LGTM.Consistent with package spec.
611-621: Propagate FMA setup_experience into package spec — LGTM.ToSoftwarePackageSpec maps all relevant fields.
pkg/spec/gitops.go (3)
164-172: Hydration helper is clean and focused — LGTM.Copies only the intended team-level fields down to package-level.
1096-1130: Maintained apps: path resolution and secret scanning LGTM.Validations (mutually exclusive label scopes) and secret gathering look correct.
1153-1160: Order of checks is sensible — LGTM.Validate disallowed fields before hydration; avoids accidental overrides from package file.
| URL: "http://foo.com", | ||
| SelfService: true, | ||
| InstallScript: fleet.TeamSpecSoftwareAsset{Path: "./foo/install-script.sh"}, | ||
| PostInstallScript: fleet.TeamSpecSoftwareAsset{Path: "./foo/post-install-script.sh"}, | ||
| PreInstallQuery: fleet.TeamSpecSoftwareAsset{Path: "./foo/query.yaml"}, | ||
| InstallDuringSetup: optjson.Bool{Set: true}, | ||
| }, | ||
| { | ||
| URL: "http://bar.com", | ||
| SelfService: false, | ||
| InstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/install-script.sh"}, | ||
| PostInstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/post-install-script.sh"}, | ||
| PreInstallQuery: fleet.TeamSpecSoftwareAsset{Path: "./bar/query.yaml"}, | ||
| URL: "http://bar.com", | ||
| SelfService: false, | ||
| InstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/install-script.sh"}, | ||
| PostInstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/post-install-script.sh"}, | ||
| PreInstallQuery: fleet.TeamSpecSoftwareAsset{Path: "./bar/query.yaml"}, | ||
| InstallDuringSetup: optjson.Bool{Set: true}, | ||
| }, | ||
| } | ||
| wantAppStoreApps := []fleet.TeamSpecAppStoreApp{ | ||
| { | ||
| AppStoreID: "1234", | ||
| AppStoreID: "1234", | ||
| InstallDuringSetup: optjson.Bool{Set: true}, | ||
| }, | ||
| { | ||
| AppStoreID: "5678", | ||
| AppStoreID: "5678", | ||
| InstallDuringSetup: optjson.Bool{Set: true}, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
optjson.Bool likely missing Value: true — tests intend “true” but encode default false
optjson.Bool generally requires both Set and Value to represent an explicit boolean. As written, optjson.Bool{Set: true} will marshal as a present field with the default zero value (false). If the intent is “true”, set Value: true as well; otherwise expectations and behavior may diverge from runtime structs.
Apply this diff:
@@
- InstallDuringSetup: optjson.Bool{Set: true},
+ InstallDuringSetup: optjson.Bool{Set: true, Value: true},
@@
- InstallDuringSetup: optjson.Bool{Set: true},
+ InstallDuringSetup: optjson.Bool{Set: true, Value: true},
@@
- InstallDuringSetup: optjson.Bool{Set: true},
+ InstallDuringSetup: optjson.Bool{Set: true, Value: true},
@@
- InstallDuringSetup: optjson.Bool{Set: true},
+ InstallDuringSetup: optjson.Bool{Set: true, Value: true},Run to confirm optjson.Bool semantics and existing usage patterns:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the type and JSON behavior
rg -nP -C3 'package\s+optjson\b' | head -n 50 || true
rg -nP -C3 'type\s+Bool\s+struct\b' || true
rg -nP -C2 'MarshalJSON|UnmarshalJSON' | sed -n '1,200p' || true
# 2) See how Bool is typically instantiated elsewhere
rg -nP -C2 'optjson\.Bool\s*{\s*Set:\s*true(?:\s*,\s*Value:\s*(true|false))?' || true
# 3) Check where InstallDuringSetup is compared or marshaled
rg -nP -C3 '\bInstallDuringSetup\b'Length of output: 70575
Ensure explicit “true” for InstallDuringSetup in integration tests
The optjson.Bool type has a Value field that defaults to false when omitted. Instantiating it with only Set: true will still represent false, not the intended true. Update the four occurrences in integration_enterprise_test.go to include Value: true:
• server/service/integration_enterprise_test.go:11695
• server/service/integration_enterprise_test.go:11703
• server/service/integration_enterprise_test.go:11709
• server/service/integration_enterprise_test.go:11713
Apply this diff:
@@ server/service/integration_enterprise_test.go:11690
- InstallDuringSetup: optjson.Bool{Set: true},
+ InstallDuringSetup: optjson.Bool{Set: true, Value: true},
@@ server/service/integration_enterprise_test.go:11698
- InstallDuringSetup: optjson.Bool{Set: true},
+ InstallDuringSetup: optjson.Bool{Set: true, Value: true},
@@ server/service/integration_enterprise_test.go:11707
- InstallDuringSetup: optjson.Bool{Set: true},
+ InstallDuringSetup: optjson.Bool{Set: true, Value: true},
@@ server/service/integration_enterprise_test.go:11711
- InstallDuringSetup: optjson.Bool{Set: true},
+ InstallDuringSetup: optjson.Bool{Set: true, Value: true},📝 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.
| URL: "http://foo.com", | |
| SelfService: true, | |
| InstallScript: fleet.TeamSpecSoftwareAsset{Path: "./foo/install-script.sh"}, | |
| PostInstallScript: fleet.TeamSpecSoftwareAsset{Path: "./foo/post-install-script.sh"}, | |
| PreInstallQuery: fleet.TeamSpecSoftwareAsset{Path: "./foo/query.yaml"}, | |
| InstallDuringSetup: optjson.Bool{Set: true}, | |
| }, | |
| { | |
| URL: "http://bar.com", | |
| SelfService: false, | |
| InstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/install-script.sh"}, | |
| PostInstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/post-install-script.sh"}, | |
| PreInstallQuery: fleet.TeamSpecSoftwareAsset{Path: "./bar/query.yaml"}, | |
| URL: "http://bar.com", | |
| SelfService: false, | |
| InstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/install-script.sh"}, | |
| PostInstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/post-install-script.sh"}, | |
| PreInstallQuery: fleet.TeamSpecSoftwareAsset{Path: "./bar/query.yaml"}, | |
| InstallDuringSetup: optjson.Bool{Set: true}, | |
| }, | |
| } | |
| wantAppStoreApps := []fleet.TeamSpecAppStoreApp{ | |
| { | |
| AppStoreID: "1234", | |
| AppStoreID: "1234", | |
| InstallDuringSetup: optjson.Bool{Set: true}, | |
| }, | |
| { | |
| AppStoreID: "5678", | |
| AppStoreID: "5678", | |
| InstallDuringSetup: optjson.Bool{Set: true}, | |
| }, | |
| URL: "http://foo.com", | |
| SelfService: true, | |
| InstallScript: fleet.TeamSpecSoftwareAsset{Path: "./foo/install-script.sh"}, | |
| PostInstallScript: fleet.TeamSpecSoftwareAsset{Path: "./foo/post-install-script.sh"}, | |
| PreInstallQuery: fleet.TeamSpecSoftwareAsset{Path: "./foo/query.yaml"}, | |
| InstallDuringSetup: optjson.Bool{Set: true, Value: true}, | |
| }, | |
| { | |
| URL: "http://bar.com", | |
| SelfService: false, | |
| InstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/install-script.sh"}, | |
| PostInstallScript: fleet.TeamSpecSoftwareAsset{Path: "./bar/post-install-script.sh"}, | |
| PreInstallQuery: fleet.TeamSpecSoftwareAsset{Path: "./bar/query.yaml"}, | |
| InstallDuringSetup: optjson.Bool{Set: true, Value: true}, | |
| }, | |
| } | |
| wantAppStoreApps := []fleet.TeamSpecAppStoreApp{ | |
| { | |
| AppStoreID: "1234", | |
| InstallDuringSetup: optjson.Bool{Set: true, Value: true}, | |
| }, | |
| { | |
| AppStoreID: "5678", | |
| InstallDuringSetup: optjson.Bool{Set: true, Value: true}, | |
| }, |
🤖 Prompt for AI Agents
In server/service/integration_enterprise_test.go around lines 11690–11714 the
tests set InstallDuringSetup using optjson.Bool{Set: true} which leaves Value
false by default; update the four occurrences at lines 11695, 11703, 11709, and
11713 to explicitly set Value: true (i.e., optjson.Bool{Set: true, Value: true})
so the flag is actually true in the test objects.
There was a problem hiding this comment.
This semantically changes the result, though what I actually wanted here was Valid: true I believe. I didn't want to turn on setup experience actually being true as part of this.
There was a problem hiding this comment.
Actually, scratch that. set: true is always true for packages from YAML, but Valid is false unless the value is provided explicitly. So Set being true here is the correct thing to expect given how optjson works, and coderabbit is off base here.
|
@iansltx I assume we don't need to apply the coderabbit suggestions? |
…to team level in GitOps (#32245) Fixes #31163. This also enables setup experience inclusion for FMAs. Includes checks and errors for invalid states. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## New Fleet configuration settings - [x] Verified that the setting is exported via `fleetctl generate-gitops` - [x] Verified the setting is documented in a separate PR to [the GitOps documentation](https://github.com/fleetdm/fleet/blob/main/docs/Configuration/yaml-files.md#L485) - [x] Verified that the setting is cleared on the server if it is not supplied in a YAML file (or that it is documented as being optional) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Team-level control of software settings in GitOps: setup experience (install during setup), self-service, labels (include/exclude), and categories for packages and App Store apps. - Team-level settings are applied to individual packages automatically. - Validation - Clear errors when setup experience is defined in multiple places (software/app vs. macOS setup). - Documentation - Removed outdated note about GitOps-generated outputs. - Tests - Added test cases covering valid team-level configuration and conflicting placement scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Anthony Maxwell <133805840+Illbjorn@users.noreply.github.com>
…pdate work (#32482) # Checklist for submitter - [x] Added/updated automated tests
3 participants
Fixes #31163.
This also enables setup experience inclusion for FMAs. Includes checks and errors for invalid states.
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)Testing
Added/updated automated tests
QA'd all new/changed functionality manually
New Fleet configuration settings
fleetctl generate-gitopsSummary by CodeRabbit