Rename mdm profiles `updated_at` to `uploaded_at` and remove automatic setting #16425

mna · 2024-01-29T19:27:08Z

Checklist for submitter

Changes file added for user-visible changes in changes/ or orbit/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)
Added/updated tests
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.

mna · 2024-01-29T20:18:56Z

server/fleet/apple_mdm.go

-	UpdatedAt time.Time                   `db:"updated_at" json:"updated_at"`
+	Labels     []ConfigurationProfileLabel `db:"labels" json:"labels,omitempty"`
+	CreatedAt  time.Time                   `db:"created_at" json:"created_at"`
+	UploadedAt time.Time                   `db:"uploaded_at" json:"updated_at"` // TODO: should we update the JSON rendering?


I don't think we should, that would be an API breaking change (documented e.g. here: https://fleetdm.com/docs/rest-api/rest-api#default-response61)

100% agree with this, I would just leave a comment so a good intended soul doesn't change it later 😅

👍 sounds good, I'll remove the TODO and add that note.

mna · 2024-01-29T20:19:20Z

server/fleet/mdm.go

@@ -364,7 +364,7 @@ type MDMConfigProfilePayload struct {
 	Identifier  string                      `json:"identifier,omitempty" db:"identifier"` // only set for macOS
 	Checksum    []byte                      `json:"checksum,omitempty" db:"checksum"`     // only set for macOS
 	CreatedAt   time.Time                   `json:"created_at" db:"created_at"`
-	UpdatedAt   time.Time                   `json:"updated_at" db:"updated_at"`
+	UploadedAt  time.Time                   `json:"updated_at" db:"uploaded_at"` // TODO: should we change the JSON rendering?


Same here, would be a breaking change.

mna · 2024-01-29T20:19:33Z

server/fleet/windows_mdm.go

@@ -37,7 +37,7 @@ type MDMWindowsConfigProfile struct {
 	SyncML      []byte                      `db:"syncml" json:"-"`
 	Labels      []ConfigurationProfileLabel `db:"labels" json:"labels,omitempty"`
 	CreatedAt   time.Time                   `db:"created_at" json:"created_at"`
-	UpdatedAt   time.Time                   `db:"updated_at" json:"updated_at"`
+	UploadedAt  time.Time                   `db:"uploaded_at" json:"updated_at"` // TODO: should we update the JSON rendering?


Same here, would be a breaking change.

roperzh

LGTM, I think the only thing to change would be the addition of the w prefix since that's being worked on separately, but maybe you're chatting with Jahziel about this.

server/datastore/mysql/apple_mdm.go

server/datastore/mysql/microsoft_mdm.go

server/datastore/mysql/microsoft_mdm_test.go

server/datastore/mysql/migrations/tables/20240129095928_AddMdmProfilesUploadedAt.go

roperzh · 2024-01-30T14:45:51Z

server/fleet/apple_mdm.go

-	UpdatedAt time.Time                   `db:"updated_at" json:"updated_at"`
+	Labels     []ConfigurationProfileLabel `db:"labels" json:"labels,omitempty"`
+	CreatedAt  time.Time                   `db:"created_at" json:"created_at"`
+	UploadedAt time.Time                   `db:"uploaded_at" json:"updated_at"` // TODO: should we update the JSON rendering?


100% agree with this, I would just leave a comment so a good intended soul doesn't change it later 😅

codecov · 2024-01-30T16:16:47Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (9069850) 65.61% compared to head (8985c3c) 65.46%.

Files	Patch %	Lines
.../tables/20240205095928_AddMdmProfilesUploadedAt.go	66.66%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16425      +/-   ##
==========================================
- Coverage   65.61%   65.46%   -0.15%     
==========================================
  Files        1133     1135       +2     
  Lines       99105    99325     +220     
  Branches     2448     2448              
==========================================
- Hits        65030    65026       -4     
- Misses      29196    29402     +206     
- Partials     4879     4897      +18

Flag	Coverage Δ
backend	`66.52% <86.66%> (-0.17%)`	⬇️

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.

roperzh

✨

mna · 2024-02-05T14:33:10Z

The Go test failure is network-related, unrelated to the changes. It could take a while to resolve given we ping an external server, I'll retry once and merge anyway if that's the only failure:

=== RUN   TestVulnerabilityDataStream
    nettest.go:33: network test start: TestVulnerabilityDataStream
    vulnerability_data_stream_test.go:41: 
        	Error Trace:	/home/runner/work/fleet/fleet/cmd/fleetctl/vulnerability_data_stream_test.go:41
        	Error:      	Received unexpected error:
        	            	bzip2 data invalid: bad magic value
        	Test:       	TestVulnerabilityDataStream
    nettest.go:36: network test done: TestVulnerabilityDataStream
--- FAIL: TestVulnerabilityDataStream (96.34s)

…c setting (#16425)

mna added 4 commits January 29, 2024 10:45

Add migration and test to use uploaded_at for mdm profiles

228d880

Existing datastore tests pass

6387abe

Fix integration tests

15614ae

Test that uploaded_at does not change in bulk-set

6a7e36c

mna commented Jan 29, 2024

View reviewed changes

mna added 5 commits January 29, 2024 16:02

Test uploaded_at behaviour for upsert on apple profiles

f158eb8

Test uploaded_at behaviour for upsert on microsoft profiles

008062f

Formatting

2da55d7

Add changes file

61b8802

Merge main

893afa6

mna temporarily deployed to Docker Hub January 29, 2024 21:48 — with GitHub Actions Inactive

mna marked this pull request as ready for review January 29, 2024 21:59

mna requested a review from a team as a code owner January 29, 2024 21:59

roperzh reviewed Jan 30, 2024

View reviewed changes

Add note to explain json field naming

e181fab

mna temporarily deployed to Docker Hub January 30, 2024 16:04 — with GitHub Actions Inactive

roperzh previously approved these changes Jan 30, 2024

View reviewed changes

Regenerate schema

8985c3c

mna dismissed roperzh’s stale review via 8985c3c February 5, 2024 14:19

mna temporarily deployed to Docker Hub February 5, 2024 14:19 — with GitHub Actions Inactive

roperzh approved these changes Feb 5, 2024

View reviewed changes

mna merged commit 9abb572 into main Feb 5, 2024

mna deleted the mna-16051-mdm-profiles-uploaded-at branch February 5, 2024 15:01

georgekarrv pushed a commit that referenced this pull request Feb 9, 2024

Rename mdm profiles updated_at to uploaded_at and remove automati…

a8ec3e5

…c setting (#16425)

lukeheath added this to the 4.44.1 milestone Feb 13, 2024

georgekarrv pushed a commit that referenced this pull request Feb 13, 2024

Rename mdm profiles updated_at to uploaded_at and remove automati…

d8f9fbd

…c setting (#16425)

lukeheath removed this from the 4.44.1 milestone Feb 13, 2024

georgekarrv pushed a commit that referenced this pull request Feb 13, 2024

Rename mdm profiles updated_at to uploaded_at and remove automati…

918ab68

…c setting (#16425)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename mdm profiles `updated_at` to `uploaded_at` and remove automatic setting #16425

Rename mdm profiles `updated_at` to `uploaded_at` and remove automatic setting #16425

mna commented Jan 29, 2024 •

edited

Loading

mna Jan 29, 2024

roperzh Jan 30, 2024

mna Jan 30, 2024

mna Jan 29, 2024

mna Jan 29, 2024

roperzh left a comment

roperzh Jan 30, 2024

codecov bot commented Jan 30, 2024 •

edited

Loading

roperzh left a comment

mna commented Feb 5, 2024

Rename mdm profiles updated_at to uploaded_at and remove automatic setting #16425

Rename mdm profiles updated_at to uploaded_at and remove automatic setting #16425

Conversation

mna commented Jan 29, 2024 • edited Loading

Checklist for submitter

mna Jan 29, 2024

Choose a reason for hiding this comment

roperzh Jan 30, 2024

Choose a reason for hiding this comment

mna Jan 30, 2024

Choose a reason for hiding this comment

mna Jan 29, 2024

Choose a reason for hiding this comment

mna Jan 29, 2024

Choose a reason for hiding this comment

roperzh left a comment

Choose a reason for hiding this comment

roperzh Jan 30, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

roperzh left a comment

Choose a reason for hiding this comment

mna commented Feb 5, 2024

Rename mdm profiles `updated_at` to `uploaded_at` and remove automatic setting #16425

Rename mdm profiles `updated_at` to `uploaded_at` and remove automatic setting #16425

mna commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading