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

Resend Windows profiles on change #27308

Merged
merged 10 commits into from
Mar 20, 2025
Merged

Conversation

getvictor
Copy link
Member

@getvictor getvictor commented Mar 19, 2025

For #25030

This PR includes the bug fix and tests.

It also includes the secrets_updated_at columns needed for story #27351. These columns are currently unused and always NULL.

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.
  • If database migrations are included, checked table schema to confirm autoupdate
  • For database migrations:
    • Checked schema for all modified table for columns that will auto-update timestamps during migration.
    • Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
    • Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).
  • Added/updated automated tests
  • A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it)
  • Manual QA for all new/changed functionality

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 88.15789% with 9 lines in your changes missing coverage. Please review.

Project coverage is 63.89%. Comparing base (c15d535) to head (0a61ce2).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
...18165922_AddChecksumAndSecretsToWindowsProfiles.go 71.42% 6 Missing and 2 partials ⚠️
server/service/microsoft_mdm.go 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #27308    +/-   ##
========================================
  Coverage   63.89%   63.89%            
========================================
  Files        1731     1733     +2     
  Lines      164610   165049   +439     
  Branches     4412     4412            
========================================
+ Hits       105175   105457   +282     
- Misses      51267    51384   +117     
- Partials     8168     8208    +40     
Flag Coverage Δ
backend 64.72% <88.15%> (-0.01%) ⬇️

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.

@getvictor getvictor marked this pull request as ready for review March 20, 2025 18:04
@getvictor getvictor requested a review from a team as a code owner March 20, 2025 18:04
MigrationClient.AddMigration(Up_20250318165922, Down_20250318165922)
}

func Up_20250318165922(tx *sql.Tx) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double-checking as it is already included in the PR checklist, but have you tested if/how this migration will any impact existing time-based comparisons related to profile delivery, reconciliation, verification, etc.?

For example, in the UPDATE statement should we preserve the hmwp.updated_at timestamp in line 31?

I don't recall offhand whether ALTER statements themselves causes ON UPDATE to fire so it may be worth a quick test to confirm that as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

ALTER statements don't cause ON UPDATE to fire. I checked this before.

The updated_at column is new here, and is only here for debug/auditing. It is not used in the code. So, it is OK for it to be updated when setting checksum.

@@ -1127,6 +1127,174 @@ func (s *integrationMDMTestSuite) TestWindowsProfileRetries() {
})
}

// TestWindowsProfileResent verifies that a Windows profile is resend when its contents have been modified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
// TestWindowsProfileResent verifies that a Windows profile is resend when its contents have been modified.
// TestWindowsProfileResent verifies that a Windows profile is resent when its contents have been modified.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Will add to next PR.

@getvictor getvictor merged commit 93bdb43 into main Mar 20, 2025
35 checks passed
@getvictor getvictor deleted the victor/25030-windows-profiles-resend branch March 20, 2025 19:43
TsekNet pushed a commit to TsekNet/fleet that referenced this pull request Mar 21, 2025
For fleetdm#25030 

This PR includes the bug fix and tests.

It also includes the `secrets_updated_at` columns needed for story
fleetdm#27351. These columns are currently unused and always NULL.

# 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] If database migrations are included, checked table schema to
confirm autoupdate
- For 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.
- [x] Ensured the correct collation is explicitly set for character
columns (`COLLATE utf8mb4_unicode_ci`).
- [x] Added/updated automated tests
- [x] A detailed QA plan exists on the associated ticket (if it isn't
there, work with the product group's QA engineer to add it)
- [x] Manual QA for all new/changed functionality
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.

2 participants