Clear label membership when label platform changes. - #31726
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31726 +/- ##
==========================================
+ Coverage 63.70% 63.74% +0.03%
==========================================
Files 1962 1962
Lines 191415 191616 +201
Branches 6219 6219
==========================================
+ Hits 121949 122146 +197
+ Misses 59913 59912 -1
- Partials 9553 9558 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes update the label management logic to clear all host-label memberships when a label's platform is changed via GitOps. This ensures that label membership reflects the new platform scope. A targeted test is added to verify that changing the platform field in a label spec clears existing memberships as expected. Changes
Sequence Diagram(s)sequenceDiagram
participant GitOps
participant Datastore
participant Database
GitOps->>Datastore: ApplyLabelSpecsWithAuthor(labelSpecs)
Datastore->>Database: Query existing labels (name, id, platform)
loop For each labelSpec
Datastore->>Database: Upsert label (insert or update)
alt Platform changed
Datastore->>Database: Delete all label_membership for label
end
alt Manual membership label
Datastore->>Database: Update manual membership
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
server/datastore/mysql/labels.go (3)
22-52: Prefetching existing labels: consider moving into the tx (writer) and deduping namesReading via ds.reader(ctx) outside the transaction can be stale under replica lag and bases the platform-change check on a potentially outdated row. It’s not fatal (worst-case is an extra or missed no-op delete), but it widens the race window. Consider querying existing labels inside withRetryTxx using the same tx (or ds.writer(ctx)) for a consistent view. Also consider deduping labelNames to shrink the IN(?) list on large specs.
100-113: Clear memberships only for non-manual labels to avoid redundant deletesThis unconditional delete also triggers for manual labels, but manual labels are cleared again later in the manual-membership path. Guarding this block to skip manual labels avoids an extra DELETE and reduces write churn.
Apply this diff here:
- if existing, ok := existingLabels[s.Name]; ok && existing.Platform != s.Platform { + if existing, ok := existingLabels[s.Name]; ok && + existing.Platform != s.Platform && + existing.LabelMembershipType != fleet.LabelMembershipTypeManual {And update the prefetch to include membership type (outside this hunk):
// Add this field: type existingLabel struct { ID uint `db:"id"` Name string `db:"name"` Platform string `db:"platform"` LabelMembershipType fleet.LabelMembershipType `db:"label_membership_type"` } // And select it: SELECT id, name, platform, label_membership_type FROM labels WHERE name IN (?)
120-131: Manual label ID lookup is fine; optional: skip double-clearBecause the platform-change cleanup may already have deleted this label’s membership, the later unconditional DELETE for manual membership results in a second delete. You can optionally track cleaned label IDs (e.g., a map[uint]struct{}) and skip the subsequent DELETE if already done. Also, you may add
LIMIT 1to theSELECT id FROM labels WHERE name = ?for clarity (name is unique, so it’s a no-op).server/datastore/mysql/labels_test.go (4)
2094-2105: Prefer datastore API over raw INSERTs for label_membership seedingUsing ds.RecordLabelQueryExecutions improves readability, enforces invariants, and reduces coupling to schema details.
Replace the raw INSERT with:
- ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { - _, err := q.ExecContext(ctx, ` - INSERT INTO label_membership (label_id, host_id) VALUES - (?, ?), (?, ?), (?, ?), (?, ?)`, - label.ID, hostDarwin1.ID, - label.ID, hostDarwin2.ID, - label.ID, hostWindows1.ID, - label.ID, hostLinux1.ID, - ) - return err - }) + require.NoError(t, ds.RecordLabelQueryExecutions(ctx, hostDarwin1, map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false)) + require.NoError(t, ds.RecordLabelQueryExecutions(ctx, hostDarwin2, map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false)) + require.NoError(t, ds.RecordLabelQueryExecutions(ctx, hostWindows1, map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false)) + require.NoError(t, ds.RecordLabelQueryExecutions(ctx, hostLinux1, map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false))
2128-2144: Strengthen Test 3 by reseeding before changing to empty platformRight now memberships are already 0 from Test 2, so this only re-asserts “still empty.” Reseeding makes the “clear on change” behavior explicit again.
Insert reseeding just before applying the empty platform:
// Test 3: Change platform to empty (all platforms) - all memberships are cleared + // Re-seed membership to ensure this step exercises clearing again + require.NoError(t, ds.RecordLabelQueryExecutions(ctx, hostDarwin1, map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false)) + require.NoError(t, ds.RecordLabelQueryExecutions(ctx, hostWindows1, map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false)) err = ds.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{Alternatively, update the comment to reflect that memberships remain empty if you prefer to keep the current flow.
2146-2155: Use datastore API for re-adding membership instead of INSERT IGNOREKeeps tests consistent and less brittle to schema changes.
Replace with:
- ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { - _, err := q.ExecContext(ctx, ` - INSERT IGNORE INTO label_membership (label_id, host_id) VALUES - (?, ?), (?, ?), (?, ?)`, - label.ID, hostDarwin1.ID, - label.ID, hostDarwin2.ID, - label.ID, hostLinux1.ID, - ) - return err - }) + for _, h := range []*fleet.Host{hostDarwin1, hostDarwin2, hostLinux1} { + require.NoError(t, ds.RecordLabelQueryExecutions(ctx, h, map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false)) + }
2112-2121: Optional: Assert platform field updated after each ApplyLabelSpecsAdd quick re-reads to ensure persisted platform matches the spec, improving signal if future regressions occur.
Example after each ApplyLabelSpecs call:
labels, err = ds.LabelsByName(ctx, []string{"platform_test_label"}) require.NoError(t, err) require.Equal(t, "windows", labels["platform_test_label"].Platform) // and "", "linux" for subsequent stepsAlso applies to: 2129-2137, 2159-2166
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changes/29596-labels-platform-change(1 hunks)server/datastore/mysql/labels.go(2 hunks)server/datastore/mysql/labels_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit Configuration File
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/labels.goserver/datastore/mysql/labels_test.go
🔇 Additional comments (2)
server/datastore/mysql/labels_test.go (1)
100-100: New test wired into suite — LGTMThe case is properly registered in TestLabels; good coverage addition.
changes/29596-labels-platform-change (1)
1-1: Changelog entry — LGTMClear, concise, and accurately reflects the fix.
| } | ||
|
|
||
| func testApplyLabelSpecsWithPlatformChange(t *testing.T, ds *Datastore) { | ||
| ctx := t.Context() |
There was a problem hiding this comment.
Compilation error: testing.T has no Context() method
Use a standard context instead; other tests in this file use context.Background().
Apply this fix:
- ctx := t.Context()
+ ctx := context.Background()📝 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.
| ctx := t.Context() | |
| ctx := context.Background() |
🤖 Prompt for AI Agents
In server/datastore/mysql/labels_test.go at line 2031, replace the call to
t.Context() with context.Background() since testing.T does not have a Context()
method. Import the context package if not already imported, and use
context.Background() to obtain a standard context as done in other tests in this
file.
There was a problem hiding this comment.
@coderabbitai Your information is out of date. t.Context() exists in recent Go version
There was a problem hiding this comment.
🧩 Analysis chain
@coderabbit-user Thanks for the correction — you’re right.
- The repository’s go.mod indicates the project targets a recent Go version, and testing.T.Context() is available in recent Go releases.
- Given that, the usage of t.Context() in testApplyLabelSpecsWithPlatformChange is correct; no change is needed. I’m withdrawing the earlier suggestion.
I’ve noted this to avoid flagging t.Context() in future Fleet PRs.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all go.mod files and print their go directives
while IFS= read -r mod; do
echo ">>> $mod"
# Print the go directive and toolchain if present
awk '/^go [0-9]+\.[0-9]+/{print "go directive:", $0} /^toolchain /{print "toolchain:", $0}' "$mod"
done < <(fd -a --full-path --hidden --exclude .git -t f '^go\.mod$')
Length of output: 190
🌐 Web query:
What Go version introduced testing.T.Context()?
💡 Result:
The method testing.T.Context() was introduced in Go 1.24[1][3][5]. This method allows tests to obtain a context that is canceled just after the test completes and before any test cleanup functions are run, enabling better management of resources in concurrent or asynchronous test code.
According to both the [Go 1.24 release notes][3] and official Go package documentation[5], T.Context and B.Context are new in Go 1.24, released in February 2025:
- Go 1.24 adds new methods T.Context, B.Context, and F.Context to the testing package, each returning a context that is canceled when the test, benchmark, or fuzz test ends[1][3][5].
- This assists in synchronizing resource cleanup for goroutines or external resources that depend on context cancellation within the test lifecycle.
Earlier Go versions, such as Go 1.23 and before, did not have the testing.T.Context() method[3][5].
Citations:
- 1: https://boldlygo.tech/archive/2025-04-09-t.context/
- 2: https://coder.com/blog/go-testing-contexts-and-t-parallel
- 3: https://tip.golang.org/doc/go1.24
- 4: https://go.dev/blog/synctest
- 5: https://pkg.go.dev/testing
- 6: https://quii.gitbook.io/learn-go-with-tests/go-fundamentals/context
- 7: testing: add T.Context() accessor golang/go#16221
@getvictor You’re right — t.Context() is a recent addition to the testing package.
- According to Go’s release notes and docs, testing.T.Context() was introduced in Go 1.24 and provides a context canceled when the test ends.
- I attempted to verify the repo’s Go/toolchain version via go.mod scanning; my first script failed due to sandbox process-substitution. I re-ran a more robust scan (see above). If toolchain or go directive in go.mod indicates 1.24+, then t.Context() is fully valid in this codebase.
Given that, I’m withdrawing the earlier suggestion to replace t.Context() with context.Background().
I’ll remember this for future reviews of this repo.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for go.mod files..."
mods="$(fd -H -t f go.mod || true)"
if [ -z "${mods}" ]; then
mods="$(find . -name go.mod -type f -print || true)"
fi
if [ -z "${mods}" ]; then
echo "No go.mod files found."
else
echo "Found go.mod files:"
echo "${mods}"
echo
while IFS= read -r mod; do
echo ">>> $mod"
echo "- go directive:"
awk '/^go [0-9]+\.[0-9]+/ {print $0}' "$mod" || true
echo "- toolchain directive:"
awk '/^toolchain / {print $0}' "$mod" || true
echo
done <<< "${mods}"
fi
echo "Searching for usages of t.Context()..."
rg -n --no-heading --color never $'\\bt\\.Context\\(' || true
echo "Done."
Length of output: 27011
🌐 Web query:
What Go version introduced testing.T.Context()?
💡 Result:
The testing.T.Context() method was introduced in Go 1.24. (pkg.go.dev) This method returns a context.Context that is canceled just before any functions registered with t.Cleanup are called, allowing for more precise control over test cleanup operations.
Citations:
@coderabbit-user (cc @getvictor) You’re absolutely right — thanks for the correction.
What I verified:
- Root module targets Go 1.24.5 (go.mod shows “go 1.24.5”).
- t.Context() is used widely across tests, including Line 2031 in server/datastore/mysql/labels_test.go.
- Go 1.24 introduced testing.T.Context(), so its usage here is valid.
With that, I’m withdrawing the earlier suggestion; no change is needed.
🧬 Code Graph Analysis Results
The following code snippets are possibly relevant for understanding or addressing the user comment regarding t.Context() in recent Go versions, as well as for understanding the context and dependencies of the test file server/datastore/mysql/labels_test.go:
File: server/datastore/mysql/testing_utils.go
Lines: 392-394
func CreateMySQLDS(t testing.TB) *Datastore {
return createMySQLDSWithOptions(t, nil)
}This snippet shows the utility function used in the test file to create a MySQL datastore for testing.
File: server/datastore/mysql/testing_utils.go
Lines: 406-410
func ExecAdhocSQL(tb testing.TB, ds *Datastore, fn func(q sqlx.ExtContext) error) {
tb.Helper()
err := fn(ds.primary)
require.NoError(tb, err)
}This snippet provides a helper for executing ad-hoc SQL in tests, used throughout the test file.
File: server/datastore/mysql/testing_utils.go
Lines: 422-436
func TruncateTables(t testing.TB, ds *Datastore, tables ...string) {
// those tables are seeded with the schema.sql and as such must not
// be truncated - a more precise approach must be used for those, e.g.
// delete where id > max before test, or something like that.
nonEmptyTables := map[string]bool{
"app_config_json": true,
"fleet_variables": true,
"mdm_apple_declaration_categories": true,
"mdm_delivery_status": true,
"mdm_operation_types": true,
"migration_status_tables": true,
"osquery_options": true,
}
testing_utils.TruncateTables(t, ds.writer(context.Background()), ds.logger, nonEmptyTables, tables...)
}This snippet provides the table truncation utility used in test setup and teardown.
File: server/fleet/labels.go
Lines: 184-194
LabelSpec struct {
ID uint `json:"id"`
Name string `json:"name"`
Description string `json:"description"`
Query string `json:"query"`
Platform string `json:"platform,omitempty"`
LabelType LabelType `json:"label_type,omitempty" db:"label_type"`
LabelMembershipType LabelMembershipType `json:"label_membership_type" db:"label_membership_type"`
Hosts []string `json:"hosts"`
HostVitalsCriteria *json.RawMessage `json:"criteria,omitempty" db:"criteria"`
}This struct is used extensively in the test cases for label specification.
File: server/fleet/labels.go
Lines: 142-154
Label struct {
UpdateCreateTimestamps
ID uint `json:"id"`
AuthorID *uint `json:"author_id" db:"author_id"`
Name string `json:"name"`
Description string `json:"description"`
Query string `json:"query"`
HostVitalsCriteria *json.RawMessage `json:"criteria,omitempty" db:"criteria"`
Platform string `json:"platform"`
LabelType LabelType `json:"label_type" db:"label_type"`
LabelMembershipType LabelMembershipType `json:"label_membership_type" db:"label_membership_type"`
HostCount int `json:"host_count,omitempty" db:"host_count"`
}This struct represents a label in the Fleet server and is used in label-related datastore operations.
File: server/fleet/datastore.go
Lines: 49-2239
Summary:
"""
Datastore interface
- Defines all methods for interacting with the Fleet server's data layer, including user, query, label, host, team, software, MDM, and other stores.
- Label-related methods include:
- ApplyLabelSpecs, GetLabelSpecs, GetLabelSpec
- AddLabelsToHost, RemoveLabelsFromHost
- UpdateLabelMembershipByHostIDs, UpdateLabelMembershipByHostCriteria
- NewLabel, SaveLabel, DeleteLabel, Label, ListLabels, LabelsSummary
- LabelQueriesForHost, ListLabelsForHost, ListHostsInLabel, ListUniqueHostsInLabels, SearchLabels, LabelIDsByName, LabelsByName
- RecordLabelQueryExecutions
- Host-related methods include:
- NewHost, DeleteHost, Host, ListHosts, HostByIdentifier, HostLiteByIdentifier, etc.
- Many methods accept a
context.Contextas the first parameter, which is relevant for test code that may uset.Context()in newer Go versions.
"""
File: server/fleet/hosts.go
Lines: 275-394
Host struct {
UpdateCreateTimestamps
HostSoftware
ID uint `json:"id" csv:"id"`
// OsqueryHostID is the key used in the request context that is
// used to retrieve host information. It is sent from osquery and may currently be
// a GUID or a Host Name, but in either case, it MUST be unique
OsqueryHostID *string `json:"-" db:"osquery_host_id" csv:"-"`
DetailUpdatedAt time.Time `json:"detail_updated_at" db:"detail_updated_at" csv:"detail_updated_at"` // Time that the host details were last updated
LabelUpdatedAt time.Time `json:"label_updated_at" db:"label_updated_at" csv:"label_updated_at"` // Time that the host labels were last updated
PolicyUpdatedAt time.Time `json:"policy_updated_at" db:"policy_updated_at" csv:"policy_updated_at"` // Time that the host policies were last updated
LastEnrolledAt time.Time `json:"last_enrolled_at" db:"last_enrolled_at" csv:"last_enrolled_at"` // Time that the host last enrolled
SeenTime time.Time `json:"seen_time" db:"seen_time" csv:"seen_time"` // Time that the host was last "seen"
RefetchRequested bool `json:"refetch_requested" db:"refetch_requested" csv:"refetch_requested"`
NodeKey *string `json:"-" db:"node_key" csv:"-"`
OrbitNodeKey *string `json:"-" db:"orbit_node_key" csv:"-"`
Hostname string `json:"hostname" db:"hostname" csv:"hostname"` // there is a fulltext index on this field
UUID string `json:"uuid" db:"uuid" csv:"uuid"` // there is a fulltext index on this field
// Platform is the host's platform as defined by osquery's os_version.platform.
Platform string `json:"platform" csv:"platform"`
OsqueryVersion string `json:"osquery_version" db:"osquery_version" csv:"osquery_version"`
OrbitVersion *string `json:"orbit_version" db:"orbit_version" csv:"orbit_version"`
DesktopVersion *string `json:"fleet_desktop_version" db:"fleet_desktop_version" csv:"fleet_desktop_version"`
ScriptsEnabled *bool `json:"scripts_enabled" db:"scripts_enabled" csv:"scripts_enabled"`
OSVersion string `json:"os_version" db:"os_version" csv:"os_version"`
Build string `json:"build" csv:"build"`
PlatformLike string `json:"platform_like" db:"platform_like" csv:"platform_like"`
CodeName string `json:"code_name" db:"code_name" csv:"code_name"`
Uptime time.Duration `json:"uptime" csv:"uptime"`
Memory int64 `json:"memory" sql:"type:bigint" db:"memory" csv:"memory"`
// system_info fields
CPUType string `json:"cpu_type" db:"cpu_type" csv:"cpu_type"`
CPUSubtype string `json:"cpu_subtype" db:"cpu_subtype" csv:"cpu_subtype"`
CPUBrand string `json:"cpu_brand" db:"cpu_brand" csv:"cpu_brand"`
CPUPhysicalCores int `json:"cpu_physical_cores" db:"cpu_physical_cores" csv:"cpu_physical_cores"`
CPULogicalCores int `json:"cpu_logical_cores" db:"cpu_logical_cores" csv:"cpu_logical_cores"`
HardwareVendor string `json:"hardware_vendor" db:"hardware_vendor" csv:"hardware_vendor"`
HardwareModel string `json:"hardware_model" db:"hardware_model" csv:"hardware_model"`
HardwareVersion string `json:"hardware_version" db:"hardware_version" csv:"hardware_version"`
HardwareSerial string `json:"hardware_serial" db:"hardware_serial" csv:"hardware_serial"`
ComputerName string `json:"computer_name" db:"computer_name" csv:"computer_name"`
// PrimaryNetworkInterfaceID if present indicates to primary network for the host, the details of which
// can be found in the NetworkInterfaces element with the same ip_address.
PrimaryNetworkInterfaceID *uint `json:"primary_ip_id,omitempty" db:"primary_ip_id" csv:"primary_ip_id"`
NetworkInterfaces []*NetworkInterface `json:"-" db:"-" csv:"-"`
PublicIP string `json:"public_ip" db:"public_ip" csv:"public_ip"`
PrimaryIP string `json:"primary_ip" db:"primary_ip" csv:"primary_ip"`
PrimaryMac string `json:"primary_mac" db:"primary_mac" csv:"primary_mac"`
DistributedInterval uint `json:"distributed_interval" db:"distributed_interval" csv:"distributed_interval"`
ConfigTLSRefresh uint `json:"config_tls_refresh" db:"config_tls_refresh" csv:"config_tls_refresh"`
LoggerTLSPeriod uint `json:"logger_tls_period" db:"logger_tls_period" csv:"logger_tls_period"`
TeamID *uint `json:"team_id" db:"team_id" csv:"team_id"`
// Loaded via JOIN in DB
PackStats []PackStats `json:"pack_stats" csv:"-"`
// TeamName is the name of the team, loaded by JOIN to the teams table.
TeamName *string `json:"team_name" db:"team_name" csv:"team_name"`
// Additional is the additional information from the host
// additional_queries. This should be stored in a separate DB table.
Additional *json.RawMessage `json:"additional,omitempty" db:"additional" csv:"-"`
// Users currently in the host
Users []HostUser `json:"users,omitempty" csv:"-"`
GigsDiskSpaceAvailable float64 `json:"gigs_disk_space_available" db:"gigs_disk_space_available" csv:"gigs_disk_space_available"`
PercentDiskSpaceAvailable float64 `json:"percent_disk_space_available" db:"percent_disk_space_available" csv:"percent_disk_space_available"`
GigsTotalDiskSpace float64 `json:"gigs_total_disk_space" db:"gigs_total_disk_space" csv:"gigs_total_disk_space"`
// DiskEncryptionEnabled is only returned by GET /host/{id} and so is not
// exportable as CSV (which is the result of List Hosts endpoint). It is
// a *bool because for some Linux we set it to NULL and omit it from the JSON
// response if the host does not have disk encryption enabled. It is also
// omitted if we don't have encryption information yet.
DiskEncryptionEnabled *bool `json:"disk_encryption_enabled,omitempty" db:"disk_encryption_enabled" csv:"-"`
HostIssues `json:"issues,omitempty" csv:"-"`
// DeviceMapping is in fact included in the CSV export, but it is not directly
// encoded from this column, it is processed before marshaling, hence why the
// struct tag here has csv:"-".
DeviceMapping *json.RawMessage `json:"device_mapping,omitempty" db:"device_mapping" csv:"-"`
MDM MDMHostData `json:"mdm" db:"mdm_host_data" csv:"-"`
// RefetchCriticalQueriesUntil can be set to a timestamp up to which the
// "critical" queries will be constantly reported to the host that checks in
// to be re-executed until a condition is met (or the timestamp expires). The
// notion of "critical query" is voluntarily loosely defined so that future
// requirements may use this mechanism. The difference with RefetchRequested
// is that the latter is a one-time request, while this one is a persistent
// until the timestamp expires. The initial use-case is to check for a host
// to be unenrolled from its old MDM solution, in the "migrate to Fleet MDM"
// workflow (both Apple and Windows).
//
// In the future, if we want to use it for more than one use-case, we could
// add a "reason" field with well-known labels so we know what condition(s)
// are expected to clear the timestamp. For now there's a single use-case
// so we don't need this.
RefetchCriticalQueriesUntil *time.Time `json:"refetch_critical_queries_until" db:"refetch_critical_queries_until" csv:"-"`
// DEPAssignedToFleet is set to true if the host is assigned to Fleet in Apple Business Manager.
// It is a *bool becase we want it to be returned from only a subset of endpoints related to
// Orbit and Fleet Desktop. Otherwise, it will be set to NULL so it is omitted from JSON
// responses.
//
// The boolean is based on information ingested from the Apple DEP API that is stored in the
// host_dep_assignments table.
DEPAssignedToFleet *bool `json:"dep_assigned_to_fleet,omitempty" db:"dep_assigned_to_fleet" csv:"-"`
// LastRestartedAt is a UNIX timestamp that indicates when the Host was last restarted.
LastRestartedAt time.Time `json:"last_restarted_at" db:"last_restarted_at" csv:"last_restarted_at"`
// Policies is the list of policies and whether it passes for the host
Policies *[]*HostPolicy `json:"policies,omitempty" csv:"-"`
// nil -> field isn't loaded
// true -> at least one non-revoked cert exists
// false -> we know there is no cert
HasHostIdentityCert *bool `json:"-" db:"has_host_identity_cert" csv:"-"`
}This struct is used for host creation and manipulation in the tests.
File: server/fleet/labels.go
Lines: 33-39
HostVitalCriteria struct {
Vital *string `json:"vital,omitempty"`
Value *string `json:"value,omitempty"`
Operator *HostVitalOperator `json:"operator,omitempty"`
And []HostVitalCriteria `json:"and,omitempty"`
Or []HostVitalCriteria `json:"or,omitempty"`
}This struct is used for host vitals label criteria in advanced label membership tests.
File: server/fleet/datastore.go
Lines: 2380-2383
NotFoundError interface {
error
IsNotFound() bool
}This interface is used in error handling in the test file, e.g., when deleting non-existent labels.
File: server/fleet/datastore.go
Lines: 2405-2411
func IsForeignKey(err error) bool {
var fke ForeignKeyError
if errors.As(err, &fke) {
return fke.IsForeignKey()
}
return false
}This function is used in the test file to check for foreign key constraint errors.
File: server/mdm/android/mysql/testing_utils.go
Lines: 21-23
func CreateMySQLDS(t testing.TB) *Datastore {
return createMySQLDSWithOptions(t, nil)
}This is a duplicate of the utility function for creating a MySQL datastore, as above.
Note:
The test file uses t.Context() in at least one test (testApplyLabelSpecsWithPlatformChange), which is only available in Go 1.20 and later. The above snippets provide context for the test helpers, label and host structures, and datastore interface methods that are relevant to label and host operations in the tests.
✏️ Learnings added
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.274Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.274Z
Learning: In fleetdm/fleet, tests may validly use testing.T.Context() when the module/toolchain targets Go 1.24+. Do not flag t.Context() usage in this codebase if go.mod/toolchain indicates Go >= 1.24.
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.274Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Learnt from: getvictor
PR: fleetdm/fleet#31579
File: ee/orbit/pkg/hostidentity/host_identity.go:15-15
Timestamp: 2025-08-04T18:28:51.825Z
Learning: The math/rand/v2 package is available in Go 1.22 and later releases, not just a proposal as previously thought.
Learnt from: lucasmrod
PR: fleetdm/fleet#30550
File: server/datastore/mysql/policies_test.go:6368-6378
Timestamp: 2025-07-03T15:13:36.494Z
Learning: Go 1.22 introduced a significant change to loop variable semantics where each iteration of a for loop creates a new variable, automatically fixing the classic closure capture bug that existed in earlier Go versions. Code that captures loop variables in closures no longer needs explicit variable copying in Go 1.22+.
Learnt from: lucasmrod
PR: fleetdm/fleet#30550
File: server/datastore/mysql/policies_test.go:6368-6378
Timestamp: 2025-07-03T15:13:36.494Z
Learning: Go 1.22 introduced a significant change to loop variable semantics where each iteration of a for loop creates a new variable, automatically fixing the classic closure capture bug that existed in earlier Go versions. Code that captures loop variables in closures no longer needs explicit variable copying in Go 1.22+. This fix is only enabled if the module's go.mod file specifies go 1.22 or higher.
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
LGTM, suggestion about using the last insert ID rather than a new query.
There's a subtle thing worth mentioning here, if not in docs: we don't technically support changing the platform of a label (at least, it's specifically prohibited in the UI). The most correct thing to do here IMO would be to delete the label and the memberships, and make an entirely new label. This would mirror the process the user takes in the UI, including have new create and update timestamps and author ID. On the other hand, anything linked to the old label would be affected, so on balance it's probably better to do it this way.
| } else { | ||
| // New label - fetch the ID we just created | ||
| sqlGetID := `SELECT id FROM labels WHERE name = ?` | ||
| if err := sqlx.GetContext(ctx, tx, &labelID, sqlGetID, s.Name); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "get new label ID for manual membership") | ||
| } |
There was a problem hiding this comment.
Now that we have the existing label IDs, it seems like we could get the result from the insert statement and use lastInsertId() here instead of doing another query? If we reach this branch of the code then we know that lastInsertId will be non-zero, otherwise we'd have it in existingLabels already.
There was a problem hiding this comment.
Yes, makes sense. Would need to move the existing labels fetch into the transaction to be safe. I'll plan to make this change in a follow up PR.
2 participants
Fixes #29596
Related, new issue filed: #31727
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Bug Fixes
Tests