Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing issue where deleted profiles were being sent to devices. #25095

Merged
merged 6 commits into from
Jan 6, 2025

Conversation

getvictor
Copy link
Member

@getvictor getvictor commented Jan 2, 2025

#24804

Checklist for submitter

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • Added/updated tests
  • If database migrations are included, checked table schema to confirm autoupdate
  • Manual QA for all new/changed functionality

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

Attention: Patch coverage is 71.79487% with 33 lines in your changes missing coverage. Please review.

Project coverage is 63.83%. Comparing base (4c463b6) to head (edec508).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
server/mdm/nanomdm/storage/mysql/queue.go 71.79% 7 Missing and 4 partials ⚠️
...les/20250102121439_AddIgnoreErrorToHostProfiles.go 54.54% 4 Missing and 1 partial ⚠️
server/mdm/nanomdm/storage/allmulti/allmulti.go 0.00% 5 Missing ⚠️
server/datastore/mysql/apple_mdm.go 85.00% 2 Missing and 1 partial ⚠️
server/datastore/mysql/common_mysql/batch.go 78.57% 2 Missing and 1 partial ⚠️
server/mdm/nanomdm/storage/file/file.go 0.00% 3 Missing ⚠️
server/service/apple_mdm.go 84.21% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25095      +/-   ##
==========================================
+ Coverage   54.30%   63.83%   +9.52%     
==========================================
  Files        1615     1617       +2     
  Lines      153594   153709     +115     
  Branches     3987     3987              
==========================================
+ Hits        83416    98123   +14707     
+ Misses      63296    47776   -15520     
- Partials     6882     7810     +928     
Flag Coverage Δ
backend 64.70% <71.79%> (+10.41%) ⬆️

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.

@getvictor getvictor changed the title Fixing issue where deleted profiles being sent to devices. Fixing issue where deleted profiles were being sent to devices. Jan 2, 2025
@getvictor getvictor marked this pull request as ready for review January 2, 2025 21:20
@getvictor getvictor requested a review from a team as a code owner January 2, 2025 21:20
Copy link
Member

@mna mna left a comment

Choose a reason for hiding this comment

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

LGTM, profile reconciliation is tricky logic but the guard you added make sense to me.


// BatchProcessSimple is a simple utility function to batch process a slice of payloads.
// Provide a slice of payloads, a batch size, and a function to execute on each batch.
func BatchProcessSimple[T any](
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@@ -260,3 +261,54 @@ WHERE
)
return err
}

// BulkDeleteHostUserCommandsWithoutResults deletes all commands without results for the given host/user IDs.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is for the given "host/ command IDs", right? I see you use hostUserID elsewhere, so looks like this is deliberate, what is a hostUserID? Looks like this refers to nano's device ID?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just future-proofing. The nano ID is either a device ID or user ID. I assume we will support user enrollments in the future.

@getvictor getvictor merged commit 7e1a808 into main Jan 6, 2025
25 checks passed
@getvictor getvictor deleted the victor/24804-deleted-profiles branch January 6, 2025 19:16
getvictor added a commit that referenced this pull request Jan 6, 2025
#24804

# Checklist for submitter

- [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/Committing-Changes.md#changes-files)
for more information.
- [x] Added/updated tests
- [x] If database migrations are included, checked table schema to
confirm autoupdate
- [x] Manual QA for all new/changed functionality

(cherry picked from commit 7e1a808)
getvictor added a commit that referenced this pull request Jan 6, 2025
…) (#25177)

Cherry pick.

For #24804

# Checklist for submitter

- [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/Committing-Changes.md#changes-files)
for more information.
- [x] Added/updated tests
- [x] If database migrations are included, checked table schema to
confirm autoupdate
- [x] Manual QA for all new/changed functionality

(cherry picked from commit 7e1a808)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants