Allow multiple packages - GitOps#48710
Conversation
Parse a package YAML file as a list of packages (a lone object is a one-element list); self_service, categories, and labels are per-package while setup_experience stays fleet-level, and the multi-package field rules only apply when the file holds more than one package. Apply upserts each package by dedup_token so first-added ordering is stable, enforces the per-title hash/limit/FMA-mix rules, and removes packages dropped from the YAML. Generate emits a title's packages to their own package YAML file.
Parse tests for the per-package field-placement rules (gated on multiple packages) and list/object equivalence, datastore tests for multi-package upsert ordering, the hash/limit/FMA-mix rules, FMA/custom transitions, mixed-title and interleaved package files, and a generate test plus a full generate-to-parse round-trip.
Collapse the duplicated label-to-gitops mapping in generate into a single scopeLabels helper used by the single-package, VPP, and multi-package paths.
Extract the per-installer delete-and-side-effects logic into a deleteInstallerInBatch datastore method, and move the source-of-truth and stale-custom SELECTs into top-of-function consts to match the rest of BatchSetSoftwareInstallers.
Add a real-datastore gitops round-trip integration test (apply, generate, re-apply, no diff) and extend the batch datastore test to cover the multi-package lifecycle, removed-package side effects, and mixed-title routing in one place. Serve a ruby.deb variant so two distinct packages resolve to one title, and drop the now-redundant generate-to-parse round-trip test.
Merge the mixed-title batch case into the multi-package datastore test and replace its title-counting loop with direct per-title assertions. Build the field-placement fixtures from raw-string YAML templates. Assert the gitops round-trip produces no diff by regenerating from the re-applied state and comparing the output byte for byte, and ride a per-package self_service flag through the trip.
When a custom package is removed in a batch, re-point its policies to the first-added surviving package of the same title instead of unsetting them, which matches the existing version-replace behavior. Reuse GetCategoriesForSoftwareInstallers instead of a private helper, and rename the dropped-title list for clarity.
Assert a dropped package's policy re-points to the first-added surviving package, and move the gitops round-trip test to the end of its file.
GetSoftwarePackagesByTeamAndTitleID no longer populates categories; the caller that needs them already does so via GetCategoriesForSoftwareInstallers. Reword two comments to avoid a stale term and an identifier reference.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/28108-multiple-custom-packages #48710 +/- ##
======================================================================
Coverage ? 68.09%
======================================================================
Files ? 3680
Lines ? 234436
Branches ? 12454
======================================================================
Hits ? 159631
Misses ? 60446
Partials ? 14359
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:
|
The dedup_token match added for multiple custom packages also changed the FMA path, so a rollback diffed against the version being activated instead of the previously-active one and left its stale pending install queued. Match custom packages by dedup_token and FMAs by the currently-active version.
![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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis PR adds support for multiple custom packages under a single software title across the GitOps pipeline. Parsing ( Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 `@server/fleet/errors.go`:
- Around line 45-47: These format-string identifiers are currently declared as
variables, which causes go vet printf checks to fail on Go 1.24+. Move
SoftwareSelfServiceCategoriesConflictMessage,
SoftwareSetupExperienceFleetLevelOnlyMessage, and
SoftwareLabelsPackageLevelOnlyMessage into the existing const block in errors.go
so they remain compile-time constants for fmt.Errorf/fmt.Sprintf usage.
🪄 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: fa4436f8-7d96-4897-85d1-2afc80e1eb8b
📒 Files selected for processing (10)
cmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/testing_utils/testing_utils.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.gopkg/spec/gitops.gopkg/spec/gitops_test.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_installers_test.goserver/fleet/errors.goserver/fleet/software_installer.go
|
@claude review once |
| func validatePackageFieldPlacement(teamLevel SoftwarePackage, pkg *fleet.SoftwarePackageSpec, multiple bool) error { | ||
| if (teamLevel.SelfService && pkg.SelfService) || | ||
| (len(teamLevel.Categories.Value) > 0 && len(pkg.Categories.Value) > 0) { | ||
| return fmt.Errorf(fleet.SoftwareSelfServiceCategoriesConflictMessage, pkg.URL) | ||
| } | ||
| if multiple && pkg.InstallDuringSetup.Valid { | ||
| return fmt.Errorf(fleet.SoftwareSetupExperienceFleetLevelOnlyMessage, pkg.URL) | ||
| } | ||
| if multiple && (len(teamLevel.LabelsIncludeAny) > 0 || len(teamLevel.LabelsExcludeAny) > 0 || len(teamLevel.LabelsIncludeAll) > 0) { | ||
| return fmt.Errorf(fleet.SoftwareLabelsPackageLevelOnlyMessage, pkg.URL) |
There was a problem hiding this comment.
🟡 The new multi-package validation errors in validatePackageFieldPlacement (pkg/spec/gitops.go:337-346) have two UX rough edges: (1) all three error messages format pkg.URL with %q, but hash-only packages (a supported config) have an empty URL, so users see Couldn't add software (""). ... with no identifier. (2) The labels rule (rule 3) only depends on file-scope state (multiple + team-level labels), yet it's checked inside the per-spec loop in parseSoftware, so a file with N packages emits N copies of the same error — and when URLs are empty, they're byte-identical. Consider falling back to hash_sha256 (or list index / referenced YAML path) when URL is empty, and hoisting the labels-only rule outside the per-package loop.
Extended reasoning...
Empty identifier for hash-only packages
The three new error message constants added in server/fleet/errors.go:
SoftwareSelfServiceCategoriesConflictMessageSoftwareSetupExperienceFleetLevelOnlyMessageSoftwareLabelsPackageLevelOnlyMessage
all use %q and are called from validatePackageFieldPlacement in pkg/spec/gitops.go:337-346 with fmt.Errorf(msg, pkg.URL). A hash-only package (no url:, just hash_sha256:) is a valid config — later in parseSoftware the check is "at least one of hash_sha256 or url is required". For such a package pkg.URL == "", so the emitted error reads:
Couldn't add software (""). ...
Bare empty quotes are not something a user can grep for or use to find the offending entry in a list.
Labels-rule error duplicated per package
validatePackageFieldPlacement is called inside the per-spec loop in parseSoftware and appends to multiError on each violation. Of the three rules:
- Rule 1 (self_service / categories) gates on
pkg.SelfService/pkg.Categories— legitimately per-package. - Rule 2 (setup_experience) gates on
pkg.InstallDuringSetup.Valid— also per-package; fires only for the offender. - Rule 3 (labels) gates on
multiple+teamLevel.Labels*, which are file-scope. Every package in the file trips it identically.
So a multi-package file with N packages and team-level labels emits N copies of "Labels can be specified only in the package-level file when adding multiple packages of the same software." Combined with the empty-URL issue above, the messages are literally byte-identical for hash-only packages. The PR's own test scenario in TestMultiPackageFieldPlacement (labels in the fleet-level file) uses two hash-only entries and would hit this, but the assertion is ErrorContains(substring), which passes on any occurrence.
Step-by-step
Given this multi-package file, valid apart from the fleet-level labels_include_all:
software:
packages:
- path: software/pkgs.yml
labels_include_all: [macOS]# software/pkgs.yml
- hash_sha256: aaaa...aaaa
- hash_sha256: bbbb...bbbbTrace:
parseSoftwarereads the list, setsmultiple := true.- Loop iteration 1: calls
validatePackageFieldPlacement(teamLevel, spec[0], true).teamLevel.LabelsIncludeAllis non-empty andmultipleis true → returnsfmt.Errorf(SoftwareLabelsPackageLevelOnlyMessage, "")=Couldn't add software (""). Labels can be specified only in the package-level file .... - Loop iteration 2:
teamLevelunchanged,multipleunchanged → returns the exact same string. Both are appended tomultiError.
User sees the same message twice with an empty identifier in each — no way to tell which package the tool is complaining about, and no reason to think there are two problems rather than one.
Impact and fix
Not a correctness bug: validation still fails and the surface message still explains the issue. Marking as a nit — worth cleaning up but not blocking merge. Fixes:
- Prefer
hash_sha256(or list index /ReferencedYamlPath) overpkg.URLfor the%qidentifier when URL is empty. A helper likepackageIdent(pkg)keeps the call sites clean. - Hoist rule 3 outside the per-spec loop — check it once with the file-scope state before iterating, and keep rules 1 and 2 in the loop.
Identify a package by its hash when the URL is empty so hash-only packages no longer report an empty name, and report the fleet-level labels error once per file instead of once per package.
| // Inherit team-level fields the package didn't set. Labels, self_service, and | ||
| // categories are per-package for multiple packages, or inherited from the | ||
| // fleet-level file for a single package. | ||
| if !packageLevel.SelfService { |
There was a problem hiding this comment.
The comment says self_service and categories are "per-package for multiple packages," but this block still inherits them from the fleet level whenever a package omits them. And unlike labels (forbidden at fleet level when multiple), there's no restriction stopping that. So a 2-package file with self_service: true at fleet level applies it to both packages. Is inherit-to-all the intended behavior for multi-package files? If so, can we fix the comment; if not, should self_service/categories be restricted the same way labels are?
There was a problem hiding this comment.
Based on the new docs it looks like it shouldn't be an error to have self_service or categories at the fleet level even for a multiple packages file, unlike labels/setup_experience. So I think updating the comment (and making sure this is tested) would work.
| id := pkg.URL | ||
| if id == "" { | ||
| id = pkg.SHA256 | ||
| } |
There was a problem hiding this comment.
This rejects self_service/categories at both fleet and package level, but there's no equivalent conflict check for labels, so package labels silently win. Is this intentional, or should labels get the same "either/or, not both" validation?
There was a problem hiding this comment.
The check for that is in line 2774
The reason it's there is to avoid repeating the same error multiple times, since labels should not be allowed at the fleet-level at all for a multiple-packages file. I can add a comment to clarify .
Edit: actually, in case there's a single package defined in a .yml file right now we can still get this conflict, I'll update.
| item["uninstall_script"] = map[string]any{"path": writeSideFile("scripts", prefix+"-uninstall", pkg.UninstallScript)} | ||
| } | ||
| if pkg.PreInstallQuery != "" { | ||
| item["pre_install_query"] = map[string]any{"path": writeSideFile("queries", prefix+"-preinstallquery.yml", []map[string]any{{"query": pkg.PreInstallQuery}})} |
There was a problem hiding this comment.
Nothing exercises this branch end-to-end (the round-trip test uses ruby.deb with no pre-install query, and the generate unit test doesn't set one). Could we add a package with a pre_install_query to the round-trip case so the generated side-file format is verified on re-apply?
| } | ||
|
|
||
| func validatePackageFieldPlacement(teamLevel SoftwarePackage, pkg *fleet.SoftwarePackageSpec, multiple bool) error { | ||
| id := pkg.URL |
There was a problem hiding this comment.
If a package has neither url nor hash_sha256, id stays "" and the error renders as Couldn't add software (""). Is one of those guaranteed present by this point, or should we fall back to the filename/path so the message is always meaningful?
There was a problem hiding this comment.
It looks like either hash_sha256 or url being set is only validated later in the code, so It's a good idea to add that 👍
Error when labels are set at both the fleet and package level in a single-package file, matching the either/or rule for self_service and categories. Identify a package in placement errors by url, then hash, then the package file path so the message is never empty. Reword the inherit comment to describe what the block actually does.
Cover fleet-level inherit, categories/labels/self_service conflicts, the hash and file-path identifier fallbacks, and a single-package label conflict. Ride pre_install_query, post_install_script, labels, categories, and an icon through the gitops round-trip so every generated per-package field is verified on re-apply.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
jkatz01
merged commit 7124b27
into
feat/28108-multiple-custom-packages
2 participants
Related issue: Resolves #48399
Summary:
generate-gitopsgenerates a file like this if multiple installers are available per titleChecklist 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
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
New Features
Bug Fixes