Allow multiple custom packages per software title#49157
Open
cdcme wants to merge 9 commits into
Open
Conversation
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #48396 # 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/` 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. - [ ] 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 - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually ## Database migrations - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - shouldn't update timestamps unless the installer was a duplicate - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). ## New Fleet configuration settings - [ ] Setting(s) is/are explicitly excluded from GitOps If you didn't check the box above, follow this checklist for GitOps-enabled settings: - [ ] Verified that the setting is exported via `fleetctl generate-gitops` - [ ] 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) - [ ] 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) - [ ] Verified that any relevant UI is disabled when GitOps mode is enabled <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Software titles can now include multiple custom packages without duplicating the title in lists. * Added clearer package details when viewing software tied to a title or team. * **Bug Fixes** * Improved installer conflict handling so uploads now show more specific error messages. * Prevented duplicate package entries from creating extra title rows or incorrect counts. * Fixed package counting and selection so the first-added package is used consistently when multiple exist. * **Chores** * Added database changes to enforce the new package-deduplication behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
**Related issue:** Resolves #48397 ## Summary Adds the REST API layer for multiple custom packages per software title (backend only): - `GET /software/titles` and `GET /software/titles/:id` return a new `packages[]` array; `software_package` is retained as the first-added package for backwards compatibility. - `POST /software/package` adds a package to an existing title and returns the added package. - `PATCH /software/titles/:id/package` targets a specific `installer_id` and rejects a replacement whose hash matches a sibling package (409). - `DELETE /software/titles/:id/available_for_install?installer_id=` deletes one package; omitting `installer_id` deletes them all. Builds on the data-model foundation (#48396). Install-time precedence and the host-software endpoint are out of scope (#48398). # Checklist for submitter - [ ] Changes file added for user-visible changes in `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). - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually
**Related issue:** Resolves #48398 Applies first-added-wins install precedence everywhere a host can match more than one package of a software title: manual install, self-service, policy auto-install, setup experience, and the host-software read all resolve the first-added package the host is in label scope for. Policy automations can now target a specific package (defaulting to first-added), and deleting a package re-points its automations to the first-added surviving package. # Checklist for submitter - [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] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #48399 Summary: - Allows multiple installers for the same title to be defined in a yaml file - `generate-gitops` generates a file like this if multiple installers are available per title - Allows labels, self_service, categories keys to be defined per package - Inherits fleet-level keys only if they are not set at the package-level - Repoints policies.software_installer_id for a deleted installer to either the first added installer for that title, or NULL if none are available # 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/` 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. - [ ] 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 - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * GitOps output now supports software titles that contain multiple packages, generating a dedicated package file plus related assets. * Software imports and updates now preserve package order and handle multi-package titles more consistently. * **Bug Fixes** * Improved inheritance and validation for software fields so package-level settings are respected and conflicting settings are flagged. * Fixed installer batch updates to better handle added, removed, and reordered packages without disrupting related policies or pending installs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ctivity (#49085) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves # - Adds hash_sha256 field to host installer result and install software activity so they can be used in combination to cover past activities with existing installers + all new activities even if their installer will get deleted # Checklist for submitter ## Testing - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Software installation results now include the installer’s SHA-256 hash when available. * Installed-software activity records now include the corresponding `hash_sha256` value. * **Bug Fixes** * Hash values are retained in activity history after an installer is deleted, while live results correctly show the value as unavailable. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ience, install-details hash (#49079)
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThis change adds support for up to 10 custom packages per software title. Packages can be independently targeted, edited, deleted, exported, and selected for installation, with first-added precedence when multiple packages match. APIs, datastore queries, policy automation, setup experience, GitOps YAML parsing/generation, and software management UI now expose package-level data. Install results and activities also preserve package SHA-256 hashes. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for multiple custom software packages per software title (up to 10) within a team, while preserving the legacy software_package (first-added) field for backwards compatibility and applying “first-added-wins” precedence consistently.
Changes:
- Backend: extend software title/package models, list/detail query paths, policy automation pinning (
software_installer_id), and setup-experience dedupe to support multi-package titles. - GitOps: allow multi-package YAML parsing + stable generation/round-trip for multiple packages per title.
- Frontend: add multi-package Library UX (add/edit/delete per package), policy automation “select package” picker, setup experience tooltip copy, and install-details hash display.
Reviewed changes
Copilot reviewed 92 out of 94 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/testing_client_test.go | Adds installer_id multipart field support in test client update helper. |
| server/service/team_policies.go | Threads software_installer_id through policy create/modify and validates chosen installer against title packages. |
| server/service/team_policies_test.go | Extends policy automation population test to assert installer pin behavior. |
| server/service/software_titles.go | Populates packages[] and keeps software_package as first-added for detail responses; per-package categories/policies. |
| server/service/software_installers.go | Adds installer targeting on update and optional per-installer deletion on delete endpoint. |
| server/service/software_installers_test.go | Updates auth test to reflect new package lookup + delete signature. |
| server/service/orbit.go | Includes hash_sha256 in installed software activity details. |
| server/service/integration_vpp_install_test.go | Updates expected conflict messages for VPP vs installer conflicts. |
| server/service/integration_mdm_setup_experience_test.go | Extends setup experience activity assertions to include hash_sha256. |
| server/mock/service/service_mock.go | Updates mock service interface for delete signature including optional installer id. |
| server/mock/datastore_mock.go | Adds mocks for installer-scoped categories and multi-package fetch helpers. |
| server/fleet/software.go | Adds packages to title/list shapes; clarifies back-compat software_package semantics. |
| server/fleet/software_installer.go | Adds hash_sha256 to host installer results; introduces package list item type + validation helpers. |
| server/fleet/service.go | Updates service interface for installer-targeted delete. |
| server/fleet/policies.go | Adds software_installer_id to policy payloads and response shape for pinned package selection. |
| server/fleet/errors.go | Adds new conflict/validation messages for multi-package + GitOps validations. |
| server/fleet/datastore.go | Adds datastore APIs to fetch packages per title(s) and installer-scoped categories. |
| server/fleet/api_policies.go | Adds software_installer_id to team policy request payload. |
| server/fleet/activities.go | Adds hash_sha256 to installed-software activity detail. |
| server/datastore/mysql/software_titles.go | Prevents duplicate title rows via MIN(installer_id) join; adds packages[] fetch per title; adds GetSoftwarePackagesForTitles. |
| server/datastore/mysql/software_titles_test.go | Adds regression tests for multi-package title dedupe + per-installer policy dispatch. |
| server/datastore/mysql/software_test.go | Adds host software list precedence test for multi-package label scoping + first-added-wins. |
| server/datastore/mysql/setup_experience.go | Dedupes setup queue to first-added installer per title (no double-queue). |
| server/datastore/mysql/setup_experience_test.go | Adds test ensuring only first-added package is queued during setup. |
| server/datastore/mysql/policies.go | Makes GitOps policy resolution deterministic via first-added installer; adds installer join key. |
| server/datastore/mysql/policies_test.go | Adds tests for installer join key and GitOps first-added resolution behavior. |
| server/datastore/mysql/migrations/tables/20260702232839_MultipleCustomPackagesPerTitle.go | Migration adds dedup_token and new unique key; dedupes existing rows and repoints FKs before deletion. |
| server/datastore/mysql/in_house_apps.go | Removes installer-by-name existence helper (now multi-package aware paths elsewhere). |
| pkg/spec/gitops.go | Parses package YAML as list; validates fleet/package-level field placement; hydrates inheritance rules. |
| pkg/spec/gitops_test.go | Adds tests for multi-package field placement rules + inheritance behaviors. |
| frontend/test/handlers/software-handlers.ts | Adds MSW handler to return install results with hash_sha256. |
| frontend/services/entities/team_policies.ts | Threads software_installer_id through team policy create/update API calls. |
| frontend/services/entities/software.ts | Adds software_title_id to add-package flow; adds installer_id to edit/delete package calls. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/ViewYamlModal.tsx | Removes View YAML modal (flow removed). |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/index.ts | Removes View YAML modal export. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/helpers.tsx | Removes YAML-generation helper. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/helpers.tests.ts | Removes YAML-generation helper tests. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/_styles.scss | Removes View YAML modal styles. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsx | Adjusts chips/actions for multi-package titles; pluralizes “Custom package(s)”. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/LibraryItemAccordion/LibraryItemAccordion.tsx | Adds per-row self-service/auto-install indicator icons + GitOps delete lock for multi-package custom titles. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/LibraryItemAccordion/LibraryItemAccordion.tests.tsx | Adds tests for multi-package row icon behavior and Latest badge gating. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/helpers.tests.ts | Updates helper tests for new installer_id and packages nullability. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/EditSoftwareModal.tsx | Supports per-installer editing + “Edit package” title; uses shared navigation-block hook. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/EditSoftwareModal.tests.tsx | Adds tests for “Edit package” title and installerId prop acceptance. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsx | Supports per-installer deletion + “Delete package” title changes. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tests.tsx | Adds tests for multi-package delete title + warning suppression. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/index.ts | Exports new AddPackageModal. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/helpers.ts | Adds file-type restriction helper for adding packages to an existing title. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/helpers.tests.ts | Tests file-type restriction helper. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/AddPackageModal.tsx | New modal for adding a package to an existing multi-package title. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/AddPackageModal.tests.tsx | Tests AddPackageModal behavior in standard and GitOps mode. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/_styles.scss | Adds layout styling for new library description row. |
| frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/SoftwareCustomPackage.tsx | Introduces shared GitOps banner component; switches to navigation-block hook. |
| frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/SoftwareCustomPackage.tests.tsx | Tests shared GitOps banner content/link. |
| frontend/pages/SoftwarePage/helpers.tsx | Makes target helpers accept undefined installer data. |
| frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx | Adds multi-package context banner + file accept restriction + GitOps banner reuse; adjusts save button text consistency. |
| frontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsx | Collapses Actions into single Edit button when multi-package gate is enabled. |
| frontend/pages/policies/hooks/useUpdatePolicyAutomations.ts | Includes software_installer_id in policy automation update type. |
| frontend/pages/policies/helpers.ts | Adds multi-package help text, package help text, and “first-added package” resolver. |
| frontend/pages/policies/helpers.tests.tsx | Adds tests for multi-package “N versions” help text behavior. |
| frontend/pages/policies/edit/components/PolicyForm/_styles.scss | Adds responsive layout tweaks for dual dropdowns when schema sidebar is open. |
| frontend/pages/policies/components/PolicyAutomationsFields/PolicyAutomationsFields.tsx | Adds per-package dropdown + auto-select/validation logic + payload includes installer id. |
| frontend/pages/policies/components/PolicyAutomationsFields/_styles.scss | Adds layout styling for dual dropdowns and tighter table layout. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareTable/InstallSoftwareTableConfig.tsx | Adds tooltip to Version column describing first-added behavior for custom packages. |
| frontend/interfaces/software.ts | Adds installer_id, packages, max-packages constant, and install result hash field. |
| frontend/interfaces/policy.ts | Adds software_installer_id to policy software automation shape. |
| frontend/hooks/useBlockNavigation.ts | New shared beforeunload blocking hook for uploads/patch flows. |
| frontend/hooks/useBlockNavigation.tests.ts | Adds hook tests for add/remove handler behavior. |
| frontend/docs/patterns.md | Documents useBlockNavigation hook. |
| frontend/components/InfoBanner/InfoBanner.tsx | Adds optional leading icon support to InfoBanner component. |
| frontend/components/InfoBanner/_styles.scss | Styles InfoBanner layout when leading icon is present. |
| frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx | Adds ariaLabel prop and ensures combobox always has an accessible name. |
| frontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsx | Adds guarded “Package SHA-256 hash” row + copy button. |
| frontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tests.tsx | Adds tests for hash row presence/absence based on payload. |
| frontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/_styles.scss | Styles hash row to keep copy button visible with long hashes. |
| frontend/mocks/softwareMock.ts | Updates software mocks with installer_id and packages fields. |
| ee/server/service/software_installers_test.go | Updates EE install/uninstall tests for precedence resolver using packages list. |
| cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go | Adds enterprise integration test covering multi-package apply/generate/reapply stability. |
| cmd/fleetctl/fleetctl/testing_utils/testing_utils.go | Updates installer test server to serve distinct-hash variant package content. |
| cmd/fleetctl/fleetctl/get_test.go | Updates get output tests to include packages: null in shapes. |
| cmd/fleetctl/fleetctl/generate_gitops.go | Generates multi-package package YAML side file and references from fleet file; refactors label scoping helper. |
| cmd/fleetctl/fleetctl/generate_gitops_test.go | Adds unit test verifying multi-package generate output structure + side files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+695
to
+699
| if !p.SoftwareTitleID.Set && p.SoftwareInstallerID.Set && p.SoftwareInstallerID.Value != 0 { | ||
| return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{ | ||
| Message: "software_installer_id can only be set together with software_title_id", | ||
| }) | ||
| } |
| software_package: ISoftwarePackage | null; | ||
| /** All custom packages on this title (trimmed shape on list responses). | ||
| * `null` when the title has no custom packages. */ | ||
| packages: ISoftwarePackage[] | null; |
…ustom-packages # Conflicts: # frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/LibraryItemAccordion/LibraryItemAccordion.tsx # frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsx # pkg/spec/gitops.go # server/datastore/mysql/schema.sql # server/fleet/software_installer.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49157 +/- ##
==========================================
- Coverage 67.93% 65.72% -2.21%
==========================================
Files 3764 3768 +4
Lines 238273 239399 +1126
Branches 12452 12555 +103
==========================================
- Hits 161868 157345 -4523
- Misses 61773 67232 +5459
- Partials 14632 14822 +190
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:
|
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsx (1)
97-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a package-specific success message.
Line 105 still says “Successfully deleted software.” when multi-package mode deletes only one installer. Make the toast conditional, and include
canActivateMultiplePackagesin the callback dependencies.Proposed fix
- notify.success("Successfully deleted software."); + notify.success( + canActivateMultiplePackages + ? "Successfully deleted package." + : "Successfully deleted software." + ); @@ - }, [softwareId, teamId, installerId, onSuccess, onExit]); + }, [ + softwareId, + teamId, + installerId, + canActivateMultiplePackages, + onSuccess, + onExit, + ]);🤖 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 `@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsx` around lines 97 - 127, Update onDeleteSoftware to conditionally display a package-specific success toast when canActivateMultiplePackages is enabled, while retaining the existing message otherwise. Add canActivateMultiplePackages to the useCallback dependency array.cmd/fleetctl/fleetctl/generate_gitops.go (2)
2148-2161: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
generateMultiPackagedoesn't exportConfiguration(managed app config), unlike the single-package path.The single-package branch above unwraps and writes
softwareTitle.SoftwarePackage.Configurationto a side file (in-house app managed XML config), but the per-package loop ingenerateMultiPackagehas no equivalent — any in-house app package withConfigurationset that's part of a multi-package title will silently lose that config on GitOps export/round-trip.♻️ Suggested fix: mirror the single-package Configuration handling per item
if key, names := scopeLabels(pkg.LabelsIncludeAny, pkg.LabelsExcludeAny, pkg.LabelsIncludeAll); key != "" { item[key] = names } + if len(pkg.Configuration) > 0 { + var xmlStr string + if err := json.Unmarshal(pkg.Configuration, &xmlStr); err != nil { + return nil, err + } + if xmlStr != "" { + item["configuration"] = map[string]any{ + "path": writeSideFile("software", prefix+"-config.xml", []byte(xmlStr)), + } + } + } items = append(items, item)Also applies to: 2299-2336
🤖 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 `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 2148 - 2161, Update generateMultiPackage to mirror the single-package Configuration handling for each package item: JSON-unmarshal the package Configuration into XML, report and return decoding errors, write non-empty XML to a package-specific config file under the appropriate lib path, and set the item’s softwareSpec["configuration"]["path"] accordingly. Ensure filenames are unique per package and apply the same logic to the additional code path around the referenced package loop.
1971-2015: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the policy’s installer ID when exporting
install_software. Incmd/fleetctl/fleetctl/generate_gitops.go:1655,generatePolicieskeyscmd.SoftwareListbySoftwareTitleID, so a multi-package policy pinned to a non-first package is serialized with the title’s first-added hash. Fall back toSoftwareTitleIDonly whenSoftwareInstallerIDis nil.🤖 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 `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 1971 - 2015, Use the policy’s SoftwareInstallerID when looking up software entries for install_software generation, since cmd.SoftwareList is populated by installer ID in the software export logic. Update generatePolicies to key the lookup by *SoftwareInstallerID when present, falling back to SoftwareTitleID only when it is nil, ensuring multi-package policies use the pinned installer’s hash and metadata.
🧹 Nitpick comments (3)
frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tests.tsx (1)
63-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the installer-specific delete request.
Line 78 only verifies presentation; it neither supplies an
installerIdnor clicks Delete. Add a multi-package test that asserts the API receives the selected installer ID, since this is the destructive behavior introduced by the feature.🤖 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 `@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tests.tsx` around lines 63 - 85, Extend the multi-package test coverage around DeleteSoftwareModal to exercise the destructive action, not just rendered text. Render with canActivateMultiplePackages true and a specific installerId, click the Delete control, and assert the delete API/mock is called with that selected installer ID. Reuse the existing modal render helper and API mock symbols to verify the request payload.frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared banner to break the import cycle.
Line 35 imports from
SoftwareCustomPackage, while that page importsPackageForm. MoveGitOpsCustomPackageBannerinto a standalone shared module and import it from both callers; the current cycle makes module initialization and testing more brittle.🤖 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 `@frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx` at line 35, Move GitOpsCustomPackageBanner out of SoftwareCustomPackage into a standalone shared module, then update both SoftwareCustomPackage and PackageForm to import it from that module. Remove the direct PackageForm import from the page component to eliminate the circular dependency.server/service/software_titles.go (1)
229-234: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffOptional: per-package status summary is an N+1.
GetSummaryHostSoftwareInstallsis called once per package inside the loop (up to 10 round-trips per title-detail request). If a batched variant keyed byinstallerIDis (or can be) available, prefer it to collapse these into a single query.🤖 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 `@server/service/software_titles.go` around lines 229 - 234, Replace the per-package GetSummaryHostSoftwareInstalls call in the package loop with a batched lookup keyed by installer ID, using or adding a data-store method that retrieves all summaries in one query; then assign each returned summary to its corresponding pkg.Status and preserve the existing error wrapping.
🤖 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 `@frontend/pages/policies/components/PolicyAutomationsFields/_styles.scss`:
- Around line 95-102: Update the comment above the `&__row-picker` styles to
reference the actual fixed width of 225px instead of 300px.
In
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsx`:
- Around line 289-320: Wrap the callback props in renderAppStoreRow with
zero-argument functions so the click event is not passed as the installer ID:
use closures for onTrashClick, onLabelCountClick, and onLabelsClick that invoke
openDeleteModal/openEditModal with the intended app-store identifier or no
argument, matching renderPackageRows and the expected callback signatures.
In `@pkg/spec/gitops.go`:
- Around line 2267-2304: Empty package files or lists currently pass validation
without registering a package. Before the per-package loop in the
package-loading logic, add a len(softwarePackageSpecs) == 0 check and append an
appropriate validation error referencing the package path, then continue without
hydration.
In `@server/datastore/mysql/software.go`:
- Around line 3794-3908: The label CTEs in ListHostSoftware currently aggregate
all installers before applying fleet, active-status, and title filters. Add a
candidate-installer CTE (or equivalent filtered seed) using global_or_team_id,
is_active, and title_ids, then join each of no_labels, include_any, exclude_any,
and include_all to that candidate set before aggregation; retain the final
result and ordering semantics.
---
Outside diff comments:
In `@cmd/fleetctl/fleetctl/generate_gitops.go`:
- Around line 2148-2161: Update generateMultiPackage to mirror the
single-package Configuration handling for each package item: JSON-unmarshal the
package Configuration into XML, report and return decoding errors, write
non-empty XML to a package-specific config file under the appropriate lib path,
and set the item’s softwareSpec["configuration"]["path"] accordingly. Ensure
filenames are unique per package and apply the same logic to the additional code
path around the referenced package loop.
- Around line 1971-2015: Use the policy’s SoftwareInstallerID when looking up
software entries for install_software generation, since cmd.SoftwareList is
populated by installer ID in the software export logic. Update generatePolicies
to key the lookup by *SoftwareInstallerID when present, falling back to
SoftwareTitleID only when it is nil, ensuring multi-package policies use the
pinned installer’s hash and metadata.
In
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsx`:
- Around line 97-127: Update onDeleteSoftware to conditionally display a
package-specific success toast when canActivateMultiplePackages is enabled,
while retaining the existing message otherwise. Add canActivateMultiplePackages
to the useCallback dependency array.
---
Nitpick comments:
In `@frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx`:
- Line 35: Move GitOpsCustomPackageBanner out of SoftwareCustomPackage into a
standalone shared module, then update both SoftwareCustomPackage and PackageForm
to import it from that module. Remove the direct PackageForm import from the
page component to eliminate the circular dependency.
In
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tests.tsx`:
- Around line 63-85: Extend the multi-package test coverage around
DeleteSoftwareModal to exercise the destructive action, not just rendered text.
Render with canActivateMultiplePackages true and a specific installerId, click
the Delete control, and assert the delete API/mock is called with that selected
installer ID. Reuse the existing modal render helper and API mock symbols to
verify the request payload.
In `@server/service/software_titles.go`:
- Around line 229-234: Replace the per-package GetSummaryHostSoftwareInstalls
call in the package loop with a batched lookup keyed by installer ID, using or
adding a data-store method that retrieves all summaries in one query; then
assign each returned summary to its corresponding pkg.Status and preserve the
existing error wrapping.
🪄 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: 56ca2a3d-9e67-4f30-9fbc-e42eb5de7ef5
⛔ Files ignored due to path filters (2)
frontend/docs/patterns.mdis excluded by!**/*.mdserver/datastore/mysql/testdata/select_software_titles_sql_fixture.gzis excluded by!**/*.gz
📒 Files selected for processing (92)
changes/28108-multiple-custom-packagescmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/get_test.gocmd/fleetctl/fleetctl/testing_utils/testing_utils.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.goee/server/service/software_installers.goee/server/service/software_installers_test.gofrontend/__mocks__/softwareMock.tsfrontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tests.tsxfrontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsxfrontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/_styles.scssfrontend/components/InfoBanner/InfoBanner.tsxfrontend/components/InfoBanner/_styles.scssfrontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsxfrontend/hooks/useBlockNavigation.tests.tsfrontend/hooks/useBlockNavigation.tsfrontend/interfaces/policy.tsfrontend/interfaces/software.tsfrontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareTable/InstallSoftwareTableConfig.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/SoftwareCustomPackage.tests.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/SoftwareCustomPackage.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/AddPackageModal.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/AddPackageModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/helpers.tests.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/helpers.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPackageModal/index.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/EditSoftwareModal.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/EditSoftwareModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/LibraryItemAccordion/LibraryItemAccordion.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/LibraryItemAccordion/LibraryItemAccordion.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/ViewYamlModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/_styles.scssfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/helpers.tests.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/helpers.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/index.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/_styles.scssfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/helpers.tests.tsfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsxfrontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsxfrontend/pages/SoftwarePage/helpers.tsxfrontend/pages/policies/components/PolicyAutomationsFields/PolicyAutomationsFields.tests.tsxfrontend/pages/policies/components/PolicyAutomationsFields/PolicyAutomationsFields.tsxfrontend/pages/policies/components/PolicyAutomationsFields/_styles.scssfrontend/pages/policies/edit/components/PolicyForm/_styles.scssfrontend/pages/policies/helpers.tests.tsxfrontend/pages/policies/helpers.tsfrontend/pages/policies/hooks/useUpdatePolicyAutomations.tsfrontend/services/entities/software.tsfrontend/services/entities/team_policies.tsfrontend/test/handlers/software-handlers.tspkg/spec/gitops.gopkg/spec/gitops_test.goserver/datastore/mysql/in_house_apps.goserver/datastore/mysql/migrations/tables/20260702232839_MultipleCustomPackagesPerTitle.goserver/datastore/mysql/migrations/tables/20260702232839_MultipleCustomPackagesPerTitle_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/setup_experience.goserver/datastore/mysql/setup_experience_test.goserver/datastore/mysql/software.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_installers_test.goserver/datastore/mysql/software_test.goserver/datastore/mysql/software_titles.goserver/datastore/mysql/software_titles_test.goserver/fleet/activities.goserver/fleet/api_policies.goserver/fleet/datastore.goserver/fleet/errors.goserver/fleet/policies.goserver/fleet/service.goserver/fleet/software.goserver/fleet/software_installer.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/integration_enterprise_test.goserver/service/integration_mdm_setup_experience_test.goserver/service/integration_vpp_install_test.goserver/service/orbit.goserver/service/software_installers.goserver/service/software_installers_test.goserver/service/software_titles.goserver/service/team_policies.goserver/service/team_policies_test.goserver/service/testing_client_test.go
💤 Files with no reviewable changes (6)
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/_styles.scss
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/helpers.tsx
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/index.ts
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/helpers.tests.ts
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/ViewYamlModal.tsx
- server/datastore/mysql/in_house_apps.go
Comment on lines
95
to
102
| &__row-picker { | ||
| max-width: 300px; | ||
| // Pin to a fixed 300px so the control doesn't wobble when the selected | ||
| // option's label changes length (short vs long package name). | ||
| width: 225px; | ||
| max-width: 100%; // Guard against narrow containers | ||
| margin-left: auto; | ||
| text-align: left; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment references the wrong pixel value.
The comment says "Pin to a fixed 300px" but the actual value set is 225px.
📝 Fix stale comment
&__row-picker {
- // Pin to a fixed 300px so the control doesn't wobble when the selected
- // option's label changes length (short vs long package name).
+ // Pin to a fixed 225px so the control doesn't wobble when the selected
+ // option's label changes length (short vs long package name).
width: 225px;📝 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
| &__row-picker { | |
| max-width: 300px; | |
| // Pin to a fixed 300px so the control doesn't wobble when the selected | |
| // option's label changes length (short vs long package name). | |
| width: 225px; | |
| max-width: 100%; // Guard against narrow containers | |
| margin-left: auto; | |
| text-align: left; | |
| } | |
| &__row-picker { | |
| // Pin to a fixed 225px so the control doesn't wobble when the selected | |
| // option's label changes length (short vs long package name). | |
| width: 225px; | |
| max-width: 100%; // Guard against narrow containers | |
| margin-left: auto; | |
| text-align: left; | |
| } |
🤖 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 `@frontend/pages/policies/components/PolicyAutomationsFields/_styles.scss`
around lines 95 - 102, Update the comment above the `&__row-picker` styles to
reference the actual fixed width of 225px instead of 300px.
Comment on lines
+289
to
+320
| const renderAppStoreRow = () => { | ||
| if (!appStore) return null; | ||
| const { labels, kind } = pickLabels(appStore); | ||
| const isAndroidPlayStoreApp = appStore.platform === "android"; | ||
| const isIosOrIpadosApp = isIpadOrIphoneSoftwareSource(title.source); | ||
| return ( | ||
| <LibraryItemAccordion | ||
| filename={appStore.name} | ||
| version={appStore.latest_version} | ||
| addedAt={appStore.created_at} | ||
| installerType="app-store" | ||
| androidPlayStoreId={ | ||
| isAndroidPlayStoreApp ? appStore.app_store_id : undefined | ||
| } | ||
| isIosOrIpadosApp={isIosOrIpadosApp} | ||
| isActive | ||
| badgeState="latest" | ||
| labels={labels} | ||
| labelKind={kind} | ||
| canEditSoftware={canEditSoftware} | ||
| installed={appStore.status?.installed ?? 0} | ||
| pending={appStore.status?.pending ?? 0} | ||
| failed={appStore.status?.failed ?? 0} | ||
| installedPath={statusPath("installed")} | ||
| pendingPath={statusPath("pending")} | ||
| failedPath={statusPath("failed")} | ||
| onLabelCountClick={openEditModal} | ||
| onLabelsClick={openEditModal} | ||
| onTrashClick={openDeleteModal} | ||
| /> | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unwrapped callback refs risk leaking the click event as id into openDeleteModal/openEditModal.
onTrashClick={openDeleteModal}, onLabelCountClick={openEditModal}, and onLabelsClick={openEditModal} pass the functions directly instead of wrapping them (unlike renderPackageRows, which correctly uses () => openDeleteModal(pkg.installer_id) etc.). renderTrashButtonBody's <Button onClick={onTrashClick}> forwards the native click event as the first argument, so openDeleteModal/openEditModal would receive a MouseEvent as their id parameter here, not undefined.
This currently doesn't manifest as a visible bug — findSelectedPackage compares against title.packages, which is empty/null for app-store titles, so the mismatch resolves to the same fallback (null/meta.softwareInstaller) either way. Still, it's fragile and type-unsound; wrapping these consistently with the rest of the file would remove the latent risk.
🔧 Suggested fix
- onLabelCountClick={openEditModal}
- onLabelsClick={openEditModal}
+ onLabelCountClick={() => openEditModal()}
+ onLabelsClick={() => openEditModal()}
...
- onTrashClick={openDeleteModal}
+ onTrashClick={() => openDeleteModal()}📝 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
| const renderAppStoreRow = () => { | |
| if (!appStore) return null; | |
| const { labels, kind } = pickLabels(appStore); | |
| const isAndroidPlayStoreApp = appStore.platform === "android"; | |
| const isIosOrIpadosApp = isIpadOrIphoneSoftwareSource(title.source); | |
| return ( | |
| <LibraryItemAccordion | |
| filename={appStore.name} | |
| version={appStore.latest_version} | |
| addedAt={appStore.created_at} | |
| installerType="app-store" | |
| androidPlayStoreId={ | |
| isAndroidPlayStoreApp ? appStore.app_store_id : undefined | |
| } | |
| isIosOrIpadosApp={isIosOrIpadosApp} | |
| isActive | |
| badgeState="latest" | |
| labels={labels} | |
| labelKind={kind} | |
| canEditSoftware={canEditSoftware} | |
| installed={appStore.status?.installed ?? 0} | |
| pending={appStore.status?.pending ?? 0} | |
| failed={appStore.status?.failed ?? 0} | |
| installedPath={statusPath("installed")} | |
| pendingPath={statusPath("pending")} | |
| failedPath={statusPath("failed")} | |
| onLabelCountClick={openEditModal} | |
| onLabelsClick={openEditModal} | |
| onTrashClick={openDeleteModal} | |
| /> | |
| ); | |
| }; | |
| const renderAppStoreRow = () => { | |
| if (!appStore) return null; | |
| const { labels, kind } = pickLabels(appStore); | |
| const isAndroidPlayStoreApp = appStore.platform === "android"; | |
| const isIosOrIpadosApp = isIpadOrIphoneSoftwareSource(title.source); | |
| return ( | |
| <LibraryItemAccordion | |
| filename={appStore.name} | |
| version={appStore.latest_version} | |
| addedAt={appStore.created_at} | |
| installerType="app-store" | |
| androidPlayStoreId={ | |
| isAndroidPlayStoreApp ? appStore.app_store_id : undefined | |
| } | |
| isIosOrIpadosApp={isIosOrIpadosApp} | |
| isActive | |
| badgeState="latest" | |
| labels={labels} | |
| labelKind={kind} | |
| canEditSoftware={canEditSoftware} | |
| installed={appStore.status?.installed ?? 0} | |
| pending={appStore.status?.pending ?? 0} | |
| failed={appStore.status?.failed ?? 0} | |
| installedPath={statusPath("installed")} | |
| pendingPath={statusPath("pending")} | |
| failedPath={statusPath("failed")} | |
| onLabelCountClick={() => openEditModal()} | |
| onLabelsClick={() => openEditModal()} | |
| onTrashClick={() => openDeleteModal()} | |
| /> | |
| ); | |
| }; |
🤖 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
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsx`
around lines 289 - 320, Wrap the callback props in renderAppStoreRow with
zero-argument functions so the click event is not passed as the installer ID:
use closures for onTrashClick, onLabelCountClick, and onLabelsClick that invoke
openDeleteModal/openEditModal with the intended app-store identifier or no
argument, matching renderPackageRows and the expected callback signatures.
Comment on lines
2267
to
+2304
|
|
||
| softwarePackageSpecs[i].ReferencedYamlPath = resolvedPath | ||
| } | ||
| // A package YAML file is a list of packages. A file written as a single | ||
| // object is treated as a one-element list. | ||
| var singlePackageSpec fleet.SoftwarePackageSpec | ||
| listErr := YamlUnmarshal(fileBytes, &softwarePackageSpecs) | ||
| if listErr == nil { | ||
| multiError = multierror.Append(multiError, validateYAMLKeys(fileBytes, reflect.TypeFor[[]fleet.SoftwarePackageSpec](), *teamLevelPackage.Path, []string{"software", "packages"})...) | ||
| } else if err := YamlUnmarshal(fileBytes, &singlePackageSpec); err == nil { | ||
| multiError = multierror.Append(multiError, validateYAMLKeys(fileBytes, reflect.TypeFor[fleet.SoftwarePackageSpec](), *teamLevelPackage.Path, []string{"software", "packages"})...) | ||
| softwarePackageSpecs = append(softwarePackageSpecs, &singlePackageSpec) | ||
| } else { | ||
| // If we reached here, we couldn't unmarshal as either format. | ||
| multiError = multierror.Append(multiError, MaybeParseTypeError(*teamLevelPackage.Path, []string{"software", "packages"}, err)) | ||
| // couldn't unmarshal as a list or a single object | ||
| multiError = multierror.Append(multiError, MaybeParseTypeError(*teamLevelPackage.Path, []string{"software", "packages"}, listErr)) | ||
| continue | ||
| } | ||
|
|
||
| for i, spec := range softwarePackageSpecs { | ||
| softwarePackageSpec := spec.ResolveSoftwarePackagePaths(filepath.Dir(spec.ReferencedYamlPath)) | ||
| // Collect the packages that validate and hydrate, dropping any that fail. | ||
| multiple := len(softwarePackageSpecs) > 1 | ||
| // This label rule is at the fleet level, so check it once here rather than per package. | ||
| if multiple && (len(teamLevelPackage.LabelsIncludeAny) > 0 || len(teamLevelPackage.LabelsExcludeAny) > 0 || len(teamLevelPackage.LabelsIncludeAll) > 0) { | ||
| multiError = multierror.Append(multiError, fmt.Errorf(fleet.SoftwareLabelsPackageLevelOnlyMessage, *teamLevelPackage.Path)) | ||
| } | ||
| var valid []*fleet.SoftwarePackageSpec | ||
| for _, spec := range softwarePackageSpecs { | ||
| spec.ReferencedYamlPath = resolvedPath | ||
| if err := validatePackageFieldPlacement(teamLevelPackage, spec, multiple); err != nil { | ||
| multiError = multierror.Append(multiError, err) | ||
| continue | ||
| } | ||
| softwarePackageSpec := spec.ResolveSoftwarePackagePaths(filepath.Dir(resolvedPath)) | ||
| softwarePackageSpec, err = teamLevelPackage.HydrateToPackageLevel(softwarePackageSpec, ext) | ||
| if err != nil { | ||
| multiError = multierror.Append(multiError, err) | ||
| continue | ||
| } | ||
| softwarePackageSpecs[i] = &softwarePackageSpec | ||
| valid = append(valid, &softwarePackageSpec) | ||
| } | ||
| softwarePackageSpecs = valid |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate YamlUnmarshal and relevant package parsing/tests.
rg -n "func YamlUnmarshal|YamlUnmarshal\\(" -S .
rg -n "empty.*package|software package|packages.*empty|\\[\\]" pkg/spec -S
# Show the exact region around the snippet.
sed -n '2240,2325p' pkg/spec/gitops.goRepository: fleetdm/fleet
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read YamlUnmarshal implementation and tests around empty input behavior.
files=$(rg -l "func YamlUnmarshal|YamlUnmarshal\\(" -S . | tr '\n' ' ')
for f in $files; do
echo "=== $f ==="
sed -n '1,220p' "$f" | sed -n '/func YamlUnmarshal/,/}/p'
done
echo "=== tests ==="
rg -n "YamlUnmarshal|empty file|empty list|\\[\\]" -S ./*test* pkg spec 2>/dev/null || trueRepository: fleetdm/fleet
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gitops.go snippet =="
sed -n '2248,2318p' pkg/spec/gitops.go
echo
echo "== YamlUnmarshal definition(s) =="
rg -n "func YamlUnmarshal" -S .
for f in $(rg -l "func YamlUnmarshal" -S .); do
echo "--- $f ---"
line=$(rg -n "func YamlUnmarshal" "$f" | head -n1 | cut -d: -f1)
start=$((line-20)); [ "$start" -lt 1 ] && start=1
end=$((line+80))
sed -n "${start},${end}p" "$f"
done
echo
echo "== tests mentioning empty yaml/list behavior =="
rg -n "empty|\\[\\]|YamlUnmarshal|single object|package list" pkg/spec -SRepository: fleetdm/fleet
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the continuation after the snippet to see whether an empty spec slice is rejected later.
sed -n '2318,2385p' pkg/spec/gitops.go
# Also inspect any existing tests around package YAML files and empty inputs.
rg -n "softwarePackageSpecs|teamLevelPackage.Path|does not define any packages|empty" pkg/spec/*test* -SRepository: fleetdm/fleet
Length of output: 5685
🌐 Web query:
gopkg.in/yaml.v3 YAMLToJSON empty input returns nil slice or null empty document behavior
💡 Result:
In the gopkg.in/yaml.v3 package, the behavior regarding empty input depends on the specific function being called. For the primary unmarshaling function, yaml.Unmarshal, providing an empty input (e.g., an empty byte slice) results in no action being taken, and it does not return an error [1]. The target variable remains unchanged. If you are referring to the YAMLToJSON utility, it is important to note that YAMLToJSON is not a function within the gopkg.in/yaml.v3 library itself. It is a common utility function provided by separate wrapper libraries, most notably sigs.k8s.io/yaml (formerly ghodss/yaml), which uses gopkg.in/yaml.v3 internally to perform the YAML-to-JSON conversion [2][3][4][5]. When using sigs.k8s.io/yaml's YAMLToJSON with empty input: 1. It calls yaml.Unmarshal internally [2][4]. 2. Because yaml.Unmarshal treats an empty input as a no-op (performing no changes to the target), the subsequent JSON marshaling process (via encoding/json) will encounter an uninitialized or empty interface variable. 3. This typically results in the JSON output being "null" (as an empty interface in Go marshals to the JSON null value). There is no "nil slice" behavior directly associated with the empty input in the sense of returning a specific error; rather, the library gracefully handles the absence of a YAML document by effectively treating it as a null/empty document [1][6]. If you encounter behavior where an empty input returns a nil slice, it is likely a result of the surrounding application logic or the specific implementation of the converter wrapper rather than the core gopkg.in/yaml.v3 package itself [1].
Citations:
- 1: Reading empty file causes Decode to return error go-yaml/yaml#805
- 2: https://github.com/kubernetes-sigs/yaml/blob/aadfabb7d36952151672ec164d4220a6f0e31f63/yaml.go
- 3: https://github.com/kubernetes-sigs/yaml/blob/master/yaml.go
- 4: https://github.com/ghodss/yaml/blob/master/yaml.go
- 5: https://pkg.go.dev/sigs.k8s.io/yaml@v1.4.0
- 6: https://github.com/kubernetes-sigs/yaml/blob/745ef44e09d6829194f1f9752d1cc04541d5b314/yaml_test.go
Reject empty package lists in pkg/spec/gitops.go
A package file that decodes to zero entries (empty file or []) skips hydration and raises no error, so the path: entry silently registers nothing. Add an explicit len(softwarePackageSpecs) == 0 check before the per-package loop.
🤖 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.go` around lines 2267 - 2304, Empty package files or lists
currently pass validation without registering a package. Before the per-package
loop in the package-loading logic, add a len(softwarePackageSpecs) == 0 check
and append an appropriate validation error referencing the package path, then
continue without hydration.
Comment on lines
+3794
to
+3908
| include_any AS ( | ||
| SELECT | ||
| software_installers.id AS installer_id, | ||
| COUNT(*) AS count_installer_labels, | ||
| COUNT(label_membership.label_id) AS count_host_labels, | ||
| 0 AS count_host_updated_after_labels | ||
| FROM | ||
| software_installers | ||
| INNER JOIN software_installer_labels | ||
| ON software_installer_labels.software_installer_id = software_installers.id | ||
| AND software_installer_labels.exclude = 0 | ||
| AND software_installer_labels.require_all = 0 | ||
| LEFT JOIN label_membership | ||
| ON label_membership.label_id = software_installer_labels.label_id | ||
| AND label_membership.host_id = :host_id | ||
| GROUP BY | ||
| software_installers.id | ||
| HAVING | ||
| count_installer_labels > 0 AND count_host_labels > 0 | ||
| ), | ||
| exclude_any AS ( | ||
| SELECT | ||
| software_installers.id AS installer_id, | ||
| COUNT(software_installer_labels.label_id) AS count_installer_labels, | ||
| COUNT(label_membership.label_id) AS count_host_labels, | ||
| SUM( | ||
| CASE | ||
| WHEN labels.created_at IS NOT NULL AND ( | ||
| labels.label_membership_type = 1 OR | ||
| (labels.label_membership_type = 0 AND :host_label_updated_at >= labels.created_at) | ||
| ) THEN 1 | ||
| ELSE 0 | ||
| END | ||
| ) AS count_host_updated_after_labels | ||
| FROM | ||
| software_installers | ||
| INNER JOIN software_installer_labels | ||
| ON software_installer_labels.software_installer_id = software_installers.id | ||
| AND software_installer_labels.exclude = 1 | ||
| AND software_installer_labels.require_all = 0 | ||
| INNER JOIN labels | ||
| ON labels.id = software_installer_labels.label_id | ||
| LEFT JOIN label_membership | ||
| ON label_membership.label_id = software_installer_labels.label_id | ||
| AND label_membership.host_id = :host_id | ||
| GROUP BY | ||
| software_installers.id | ||
| HAVING | ||
| count_installer_labels > 0 | ||
| AND count_installer_labels = count_host_updated_after_labels | ||
| AND count_host_labels = 0 | ||
| ), | ||
| include_all AS ( | ||
| SELECT | ||
| software_installers.id AS installer_id, | ||
| COUNT(*) AS count_installer_labels, | ||
| COUNT(label_membership.label_id) AS count_host_labels, | ||
| 0 AS count_host_updated_after_labels | ||
| FROM | ||
| software_installers | ||
| INNER JOIN software_installer_labels | ||
| ON software_installer_labels.software_installer_id = software_installers.id | ||
| AND software_installer_labels.exclude = 0 | ||
| AND software_installer_labels.require_all = 1 | ||
| LEFT JOIN label_membership | ||
| ON label_membership.label_id = software_installer_labels.label_id | ||
| AND label_membership.host_id = :host_id | ||
| GROUP BY | ||
| software_installers.id | ||
| HAVING | ||
| count_installer_labels > 0 | ||
| AND count_host_labels = count_installer_labels | ||
| ) | ||
| SELECT | ||
| software_installers.id AS installer_id, | ||
| software_installers.title_id AS title_id, | ||
| software_installers.self_service AS self_service, | ||
| software_installers.platform AS platform, | ||
| software_installers.extension AS extension, | ||
| ( | ||
| no_labels.installer_id IS NOT NULL | ||
| OR include_any.installer_id IS NOT NULL | ||
| OR exclude_any.installer_id IS NOT NULL | ||
| OR include_all.installer_id IS NOT NULL | ||
| ) AS in_scope | ||
| FROM | ||
| software_installers | ||
| LEFT JOIN no_labels | ||
| ON no_labels.installer_id = software_installers.id | ||
| LEFT JOIN include_any | ||
| ON include_any.installer_id = software_installers.id | ||
| LEFT JOIN exclude_any | ||
| ON exclude_any.installer_id = software_installers.id | ||
| LEFT JOIN include_all | ||
| ON include_all.installer_id = software_installers.id | ||
| WHERE | ||
| software_installers.global_or_team_id = :global_or_team_id | ||
| AND software_installers.is_active = 1 | ||
| AND software_installers.title_id IN (:title_ids) | ||
| ORDER BY software_installers.title_id ASC, software_installers.id ASC |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Scope the label CTEs before aggregating.
Each label CTE scans and aggregates every installer across all fleets/titles, then the final query discards unrelated rows. ListHostSoftware can therefore scale with total global installer/label membership rather than this host’s relevant titles. Seed these CTEs from a candidate installer set filtered by global_or_team_id, is_active, and title_ids.
🤖 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 `@server/datastore/mysql/software.go` around lines 3794 - 3908, The label CTEs
in ListHostSoftware currently aggregate all installers before applying fleet,
active-status, and title filters. Add a candidate-installer CTE (or equivalent
filtered seed) using global_or_team_id, is_active, and title_ids, then join each
of no_labels, include_any, exclude_any, and include_all to that candidate set
before aggregation; retain the final result and ordering semantics.
Source: Path instructions
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
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.
Related issue: Resolves #28108
Adds support for uploading multiple custom packages (up to 10) for the same software title on a team — so IT admins can deploy different versions or architectures (for example, Arm vs. Intel builds or staged rollouts) to label-scoped hosts instead of splitting them across teams. The software title keeps a single first-added
software_packagefor backwards compatibility, and first-added-wins resolves overlaps consistently across self-service, manual install, policy automation, and setup experience.Feature branch combining the sub-PRs: migration (#48596), packages[] API and add/edit/delete-package endpoints (#48607), install-time precedence and setup experience (#48708), GitOps (#48710), Library and Add/Edit/Delete modals (#48520), secondary UI — policy automation, setup experience, and install-details hash (#49079), and the host install-result
hash_sha256field (#49085).Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.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.Testing
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Bug Fixes