Skip to content

Conversation

@RachelElysia
Copy link
Member

@RachelElysia RachelElysia commented Jun 12, 2025

Issue

Closes #28687

Description

  • When uploading a new package to a FMA, turn the FMA into a custom package

Screenrecording of fix

Screen.Recording.2025-06-12.at.9.52.42.AM.mov

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 for more information.
  • Added/updated automated tests
  • Manual QA for all new/changed functionality

@codecov
Copy link

codecov bot commented Jun 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.11%. Comparing base (e44f454) to head (46ab2c0).
Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #29959      +/-   ##
==========================================
- Coverage   64.18%   64.11%   -0.07%     
==========================================
  Files        1859     1863       +4     
  Lines      181882   182317     +435     
  Branches     5331     5331              
==========================================
+ Hits       116734   116892     +158     
- Misses      55965    56224     +259     
- Partials     9183     9201      +18     
Flag Coverage Δ
backend 65.05% <100.00%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@RachelElysia RachelElysia marked this pull request as ready for review June 12, 2025 17:22
@RachelElysia RachelElysia requested a review from a team as a code owner June 12, 2025 17:22
@RachelElysia
Copy link
Member Author

@iansltx -- is there something I'm missing to make the last two tests pass?

Copy link
Member

@iansltx iansltx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the feedback here (which should drastically shrink the PR), we should cover this with tests. With the suggested implementation, a data store test should be sufficient as long as you can get an FMA set up in the "arrange" part of the test.

dirty["Package"] = true

// Clear the fleet_maintained_app_id because the installer file changed
if err := svc.ds.ClearFleetMaintainedAppID(ctx, existingInstaller.InstallerID); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be around L480, or probably part of ds.SaveInstallerUpdates, specifically the touchUploaded override. I believe that turns this change into a one-liner (since we can unconditionally null out FMA ID there), plus tests.

@iansltx
Copy link
Member

iansltx commented Jun 12, 2025

IIRC docs are broken here because a PR upstream hasn't been merged due to lack of approver availability, so not concerned about that since the other tests are passing.

require.NoError(t, err)

// Set fleet_maintained_app_id manually (simulate maintained app)
_, err = ds.writer(ctx).ExecContext(ctx, `UPDATE software_installers SET fleet_maintained_app_id = 42 WHERE id = ?`, installerID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing this is breaking tests. FMA table needs to have something in it to satisfy the foreign key constraint.

@RachelElysia RachelElysia requested a review from iansltx June 12, 2025 21:08
@iansltx
Copy link
Member

iansltx commented Jun 12, 2025

@RachelElysia I confirmed the test failure was what I expected it to be, so that needs to be fixed (so tests pass) before this merges.

=== RUN   TestSoftwareInstallers/SaveInstallerUpdatesClearsFleetMaintainedAppID
    software_installers_test.go:3117: 
        	Error Trace:	/Volumes/Code/CH/fleet/server/datastore/mysql/software_installers_test.go:3117
        	            				/Volumes/Code/CH/fleet/server/datastore/mysql/software_installers_test.go:57
        	Error:      	Received unexpected error:
        	            	Error 1452 (23000): Cannot add or update a child row: a foreign key constraint fails ("server_datastore_mysql_TestSoftwareInstallers"."software_installers", CONSTRAINT "software_installers_ibfk_1" FOREIGN KEY ("fleet_maintained_app_id") REFERENCES "fleet_maintained_apps" ("id"))
        	Test:       	TestSoftwareInstallers/SaveInstallerUpdatesClearsFleetMaintainedAppID
--- FAIL: TestSoftwareInstallers/SaveInstallerUpdatesClearsFleetMaintainedAppID (0.92s)

Copy link
Member

@iansltx iansltx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tweaks!

@RachelElysia RachelElysia merged commit b763b68 into main Jun 13, 2025
37 of 39 checks passed
@RachelElysia RachelElysia deleted the 28687-fma-to-custom branch June 13, 2025 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🪲Package edits to FMA software still indicate that the software is Fleet-maintained

2 participants