Extract datastore initialization out of runServeCmd - #46742
Merged
MagnusHJensen merged 3 commits intoJun 4, 2026
Conversation
Move the MySQL datastore bring-up (DB connections, datastore, carve store) and the migration-status decision out of runServeCmd into a new cmd/fleet/datastore.go, following the established serve.go extraction pattern. evalMigrationStatus returns whether to exit so the boot/refuse decision is unit-testable while os.Exit stays in runServeCmd. Adds TestEvalMigrationStatus covering every migration status code across dev-mode and allow-missing-migrations combinations. Refs fleetdm#33370
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds helpers to build MySQL datastore options, initialize the datastore and carve store (MySQL or S3), and evaluate migration status. Replaces inline startup logic in serve.go with initDatastore and evalMigrationStatus calls, adding nil guards. Includes table-driven tests that capture stdout and verify evalMigrationStatus behavior across migration states, dev mode, and allowMissing combinations. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/fleet/datastore_test.go (1)
32-119: ⚡ Quick winAssert stdout for every policy branch.
evalMigrationStatusnow owns both the exit decision and the operator guidance, but this table only checks banners on two statuses. A regression that starts printing onAllMigrationsCompleted, or stops warning on the v4.73.2 / uninitialized paths, would still pass. Populate the expected stdout behavior for every row so the message contract moves with the exit contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/fleet/datastore_test.go` around lines 32 - 119, The table-driven TestEvalMigrationStatus must assert stdout for every branch so the message contract follows the exit decision: update each test case in TestEvalMigrationStatus to set wantOut (not just some cases) and assert it via captureStdout; specifically, for status codes from evalMigrationStatus use the current user-facing substrings the function prints (e.g., for fleet.AllMigrationsCompleted expect the success banner or empty string if it prints nothing, for fleet.UnknownMigrations expect "unrecognized migrations", for fleet.NeedsFleetv4732Fix expect the v4.73.2 fix guidance, for fleet.UnknownFleetv4732State expect the uninitialized/v4.73.2 state message, for fleet.SomeMigrationsCompleted expect the missing table list like "tables=[7]", and for fleet.NoMigrationsCompleted expect the definitive no-migrations message) — add those wantOut values to each test case and keep the existing assertion that checks assert.Contains(t, out, tc.wantOut) so every branch validates its stdout contract against evalMigrationStatus.cmd/fleet/datastore.go (1)
79-100: ⚡ Quick winFail closed on unrecognized migration status codes.
The fallthrough
return falseboots the server for anyStatusCodenot covered by the switch. Since this helper is now the single boot gate, the default path should log guidance and refuse startup so a new or malformed status cannot silently run against an unsupported schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/fleet/datastore.go` around lines 79 - 100, The switch in evalMigrationStatus currently falls through to returning false for any unrecognized fleet.MigrationStatus.StatusCode which allows boot; change it to fail closed by adding a default/unhandled case that logs guidance and refuses startup: inside evalMigrationStatus handle unknown status codes by printing a clear message (use or add a helper like printUnrecognizedMigrationStatus or reuse printUnknownMigrationsMessage but include the raw status.StatusCode and any relevant fields from status) and return true (shouldExit) so the process does not start on an unrecognized or malformed status. Ensure the new behavior is used in evalMigrationStatus and references the status variable so developers can see the unexpected code in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/fleet/serve.go`:
- Around line 254-257: After calling initDatastore, check whether the returned
mds (and any other critical returns like dbConns/carveStore if needed) is nil
and return/terminate immediately instead of wrapping a nil *mysql.Datastore into
the fleet.Datastore interface; modify the code around initDatastore, mds,
dbConns, carveStore and ds so that you only assign var ds fleet.Datastore = mds
and call ds.MigrationStatus(...) when mds is non-nil, otherwise invoke the
existing initFatal/return path to avoid the subsequent panic.
---
Nitpick comments:
In `@cmd/fleet/datastore_test.go`:
- Around line 32-119: The table-driven TestEvalMigrationStatus must assert
stdout for every branch so the message contract follows the exit decision:
update each test case in TestEvalMigrationStatus to set wantOut (not just some
cases) and assert it via captureStdout; specifically, for status codes from
evalMigrationStatus use the current user-facing substrings the function prints
(e.g., for fleet.AllMigrationsCompleted expect the success banner or empty
string if it prints nothing, for fleet.UnknownMigrations expect "unrecognized
migrations", for fleet.NeedsFleetv4732Fix expect the v4.73.2 fix guidance, for
fleet.UnknownFleetv4732State expect the uninitialized/v4.73.2 state message, for
fleet.SomeMigrationsCompleted expect the missing table list like "tables=[7]",
and for fleet.NoMigrationsCompleted expect the definitive no-migrations message)
— add those wantOut values to each test case and keep the existing assertion
that checks assert.Contains(t, out, tc.wantOut) so every branch validates its
stdout contract against evalMigrationStatus.
In `@cmd/fleet/datastore.go`:
- Around line 79-100: The switch in evalMigrationStatus currently falls through
to returning false for any unrecognized fleet.MigrationStatus.StatusCode which
allows boot; change it to fail closed by adding a default/unhandled case that
logs guidance and refuses startup: inside evalMigrationStatus handle unknown
status codes by printing a clear message (use or add a helper like
printUnrecognizedMigrationStatus or reuse printUnknownMigrationsMessage but
include the raw status.StatusCode and any relevant fields from status) and
return true (shouldExit) so the process does not start on an unrecognized or
malformed status. Ensure the new behavior is used in evalMigrationStatus and
references the status variable so developers can see the unexpected code in
logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15e6e885-b939-4dac-9e18-d883e556908b
📒 Files selected for processing (3)
cmd/fleet/datastore.gocmd/fleet/datastore_test.gocmd/fleet/serve.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46742 +/- ##
==========================================
+ Coverage 66.91% 66.93% +0.02%
==========================================
Files 2832 2835 +3
Lines 224970 225031 +61
Branches 11663 11663
==========================================
+ Hits 150531 150620 +89
+ Misses 60783 60746 -37
- Partials 13656 13665 +9
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:
|
initFatal is swapped out in tests and does not terminate there, so initDatastore and MigrationStatus can return nil; guard before dereferencing to avoid a panic and satisfy nilaway. Assert the migration banner output for every status branch. Refs fleetdm#33370
Contributor
Author
|
Hey @MagnusHJensen - Would you mind taking a look at this PR, please? This is another installment for refactoring serve.go /cc - @getvictor |
A silent return swallows the failure signal. Route through initFatal so the prod path prints and exits loudly, while tests that swap initFatal still record the event and return cleanly. Refs fleetdm#33370
1 task
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 5, 2026
Extracts the Redis pool and the cached_mysql / mysqlredis datastore wrappers out of `runServeCmd` and into a new `cmd/fleet/redis.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742). Continues the path toward `serve.go` >60% coverage per the discussion on #33370. Three functions come out of the inline block: - `initRedis` — builds the Redis pool, wraps the datastore with `cached_mysql.New`, and applies `mysqlredis.New` with the license-enforced host limit and host-cache options. Returns the pool, the fully wrapped `fleet.Datastore`, and the outermost `*mysqlredis.Datastore` (a few callers need the concrete type). - `buildRedisPoolConfig` — translates `config.RedisConfig` into the `redis.PoolConfig`, including the `redis://` scheme strip. - `validateRedisConfig` — encodes the host-cache invariant: `HostCacheEnabled` requires `HostCacheTTL > 0`. Returns an error so the caller (or in this case `initRedis` via `initFatal`) can refuse boot without that decision being buried inside a pure builder. Behavior is preserved — `runServeCmd` calls these in the same order with the same arguments, the host-cache validation still aborts startup when violated, and the full `cmd/fleet` suite passes against MySQL + Redis. `initRedis` returns early after `initFatal` so it's safe when the caller's `initFatal` doesn't terminate (the case in tests). Following the precedent established on #46742, the caller also has a loud `initFatal` + `return` guard against a nil pool (covers the same nilaway flow we hit on the datastore slice). On test scope: `TestValidateRedisConfig` covers all four combinations of `HostCacheEnabled` and `HostCacheTTL` — that's the real boot/refuse-to-boot decision. `TestBuildRedisPoolConfigStripsScheme` pins the `redis://` scheme-strip contract for Render-style URIs. I didn't add a `buildRedisPoolConfig` field-mapping matrix or an `initRedis` happy-path unit test: the former would just re-state the struct literal, and the latter needs a real Redis pool (the smoke boot exercises it end-to-end instead). This completes the four named init-block extractions on this issue. If further coverage gains are needed beyond what these have already moved, the next conversation is whether to test `runServeCmd` directly via the injected `initFatal`. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Consolidated Redis initialization and datastore wrapping into a dedicated helper; startup now validates the Redis pool and handles initialization failures explicitly. * **Tests** * Added unit tests for Redis address handling and host-cache TTL validation to ensure config behavior is enforced. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 task
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 8, 2026
Extracts the osquery status, result, and audit JSON logger setup out of `runServeCmd` and into a new `cmd/fleet/logging.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830). Continues trimming `runServeCmd` toward the `serve.go` coverage goal on #33370 — this is the largest single slice so far (~100 lines out). Three functions come out of the inline block: - `initOsqueryLogging` — builds the status and result loggers, plus the audit logger when enabled. Mutates the shared `logging.Config` per logger in the same sequence as before, so the constructed loggers are identical. - `buildLoggingConfig` — maps `config.FleetConfig` into the common `logging.Config` shared by all three loggers. - `shouldEnableAuditLog` — the premium-and-enabled gate for the audit logger, pulled out so the decision is its own testable unit. Behavior is preserved — `runServeCmd` calls this in the same place with the same arguments, the per-logger config mutation order is unchanged, and the full `cmd/fleet` suite passes against MySQL + Redis. `initOsqueryLogging` returns early after `initFatal` so it's safe when the caller's `initFatal` doesn't terminate (the case in tests), and it guards a nil license up front since the audit gate dereferences it (matching the nil-guard precedent from #46742/#46830). On test scope: `TestShouldEnableAuditLog` covers all four combinations of license tier and the config flag — audit logging is a premium feature, so the gate is the meaningful decision here. `TestBuildLoggingConfigMapsConfig` is a light check that the config mapping is wired through. I didn't add a full `initOsqueryLogging` happy-path unit test: `logging.NewJSONLogger` constructs real log sinks, so that path is exercised by booting the server rather than by standing up logger backends in a unit test. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Audit logging support is now available for premium license holders. * **Refactor** * Improved logging initialization and configuration management. * **Tests** * Added test coverage for audit logging enablement and configuration mapping. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 task
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 9, 2026
) Extracts the geoIP provider and mail service setup out of `runServeCmd` and into new `cmd/fleet/geoip.go` and `cmd/fleet/mail.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893). Both are best-effort startup providers — they log and fall back rather than aborting boot — so they group naturally. Functions: - `initGeoIP` — returns the GeoIP provider. When no database path is configured, or the MaxMind database fails to load, it returns a no-op provider and logs rather than aborting startup. - `initMailService` — configures the mail service; a construction failure is logged and the (possibly nil) service is returned, matching the prior best-effort behavior. - `shouldForceSMTPBackend` — the SMTP-vs-custom-backend rule, pulled out so the decision is its own testable unit: SMTP and a custom email backend are mutually exclusive, and an already-enabled SMTP configuration wins. Behavior is preserved — `runServeCmd` calls these in the same place with the same arguments, and the full `cmd/fleet` suite passes against MySQL + Redis. The mail block's `config.Email.EmailBackend` reset is local to mail construction (nothing downstream reads it), so moving it into `initMailService` is behavior-identical. On test scope: `TestInitGeoIP` pins the not-fatal fallback for both the missing-path and invalid-path cases — GeoIP being best-effort is a real guarantee worth locking. `TestShouldForceSMTPBackend` covers the backend mutual-exclusion decision, including the nil app config / nil SMTP settings edges. I didn't add a full `initMailService` happy-path unit test: `mail.NewService` builds real SMTP/SES backends, so that path is exercised by booting the server. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Refactor** * Improved GeoIP initialization with automatic fallback when database configuration is unavailable * Enhanced mail service initialization with better error handling during startup * Refined SMTP backend precedence logic * **Tests** * Added comprehensive unit tests for GeoIP and mail service initialization scenarios <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 tasks
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 17, 2026
Extracts the cron schedule registration out of `runServeCmd` and into a new `cmd/fleet/cron_registration.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151). This is the largest slice so far — `runServeCmd` drops from ~1300 to ~1000 lines, and `serve.go` from 1776 to 1472. The 33 `StartCronSchedule` registrations move into one `startCronSchedules` entry point backed by a `cronSchedulesDeps` struct (the dependencies the closures previously captured from `runServeCmd`). Registration is grouped by domain: - `registerCleanupAndMaintenanceCrons` — chart data collection, the `cron_stats` cleanup goroutine, software migrations, frequent cleanups, cleanups-then-aggregation, query results cleanup, upcoming activities, usage statistics, batch activities. - `registerVulnerabilityCrons` — the vulnerabilities schedule, or the remote-trigger proxy when processing is disabled on this instance. - `registerWorkerCrons` — automations and worker integrations. - `registerMDMCrons` — Apple MDM worker, DEP profile assigner, service discovery, the Apple/Windows/Android profile managers, the Android device reconciler, the Android policy migrations, and the APNs pusher. - `registerPremiumCrons` — iPhone/iPad refetcher and reviver, maintained apps, VPP app version refresh (and the one-shot VPP country backfill), recovery lock passwords, managed local account rotation, activities streaming, and the calendar schedule. - `registerMiscCrons` — host vitals label membership and the batch activity completion checker. Behavior is preserved — the schedules register in the same order with the same arguments, the same conditionals gate them (premium, audit log, env vars, software store presence), and the `config` is threaded as a pointer so the `&config` and `config.Calendar` mutations inside the calendar closure keep their original semantics. `cmd/fleet/cron.go` (the schedule definitions) is intentionally untouched; only the wiring moved. One unit test added: `TestVulnerabilityProcessingDisabled` covers the vuln enable/disable predicate extracted into `vulnerabilityProcessingDisabled`, including the legacy `current_instance_checks` `"0"` value. The rest of the file is dependency-wiring relocation with no further decision logic to unit-test — those paths construct real schedules, so they stay covered by the existing suite and integration tests. The full `cmd/fleet` suite passes against MySQL + Redis, and a local server boot confirms the same 30 cron schedules start as before (verified against the "started cron schedules" log line). **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (verified via local server boot — same 30 cron schedules start) - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Centralized background cron schedule startup and standardized job initialization sequencing for maintenance, vulnerability handling, integrations, MDM workflows, and premium tasks. * **New Features / Behavior** * Added config- and license-controlled enablement for vulnerability processing (local vs remote triggering), MDM automation (including APNs delivery and device reconciliation), and premium-only refresh/recovery behaviors. * Made chart data collection and optional activity streaming configurable, with safe fallbacks for scheduling periodicity. * **Tests** * Added coverage for vulnerability-schedule enable/disable decision logic. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 tasks
JordanMontgomery
pushed a commit
that referenced
this pull request
Jun 24, 2026
Extracts the `/api/` request timeout/body-size override middleware out of `runServeCmd` and into `apiTimeoutOverrideHandler` in a new `cmd/fleet/http_middleware.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151, #47562). `runServeCmd` drops from ~1000 to ~900 lines, and `serve.go` from 1475 to 1373. The middleware is the `~100`-line `rootMux.HandleFunc("/api/", ...)` closure that applies per-route read/write deadline overrides for endpoints that legitimately run long — synchronous script runs, large software-installer and bootstrap-package uploads, the Android enterprise signup SSE stream, and large MDM profile batch operations — and, for package-upload routes, caps the request body and threads the configured max installer size through the request context. Behavior is preserved — the handler is moved verbatim and wired into `rootMux` via a single `apiTimeoutOverrideHandler(apiHandler, config, logger)` call, so the same routes get the same overrides and every request still falls through to `apiHandler.ServeHTTP`. The now-unused `scripts` and `installersize` imports drop out of `serve.go`. On test scope: `TestAPITimeoutOverrideHandler` verifies the real decision in this middleware — that package-upload paths thread the configured max installer size into the request context (and non-upload requests keep the default) — and that the wrapped API handler is always invoked. The deadline overrides themselves go through `http.ResponseController`, which a unit-test `ResponseRecorder` doesn't support (the handler logs and proceeds, as in production), so those are exercised by booting the server rather than asserted in a unit test. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (verified via local server boot) - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved timeout handling for long-running operations across the API. Script execution, file uploads, Server-Sent Event streams, and batch operations now have optimized request timeouts and body size limits. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 tasks
juan-fdz-hawa
pushed a commit
that referenced
this pull request
Jun 30, 2026
) Adds an end-to-end boot test for `runServeCmd`, the main server entry point. This is the coverage milestone for #33370: `serve.go` goes from ~7% to ~64%, and `runServeCmd` itself from 0% to ~62%. The earlier PRs on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151, #47562, #47891) extracted testable pieces out of `runServeCmd`, but the function itself stayed at 0% — it blocks on an OS signal and wires the entire server together, so the only way to cover it is to actually boot it. This PR does that. `TestRunServeCmd` (gated behind `MYSQL_TEST` + `REDIS_TEST`) boots the full server against a real migrated test MySQL and Redis, waits for `/healthz`, then cancels the command context to trigger a graceful shutdown. It covers two paths: - **Full boot with Apple MDM enabled** — a 32-byte server private key brings up the Apple MDM protocol services and the host-identity / conditional-access SCEP setup, so the boot exercises the MDM startup path as well as the core wiring, cron schedules, and HTTP server. - **Fail-fast on bad config** — an invalid Redis host-cache configuration (enabled with a non-positive TTL) aborts startup through `initFatal` and returns rather than serving, covering the Redis-init error path and the nil-pool guard. Beyond coverage, this doubles as a regression net for the ongoing `runServeCmd` slicing: a future change that breaks startup now fails this test instead of reaching a release. **One production change**, in `runServeCmd`'s shutdown `select`: it now also watches `cmd.Context().Done()`. This is inert in production — the root command runs via `Execute()` (not `ExecuteContext()`), so `cmd.Context()` is `context.Background()` and never cancels. Only the test runs the command with a cancelable context, which is how it shuts the server down without sending a real signal (a `SIGTERM` would kill the test binary). A couple of notes for reviewers: - The test uses `os.Setenv` (not `t.Setenv`) because the MySQL test helper marks the test parallel; the boot scenarios run as serial subtests so the process-global config env doesn't race. - `runServeCmd` registers metrics with the process-global Prometheus registry, which can only happen once per process, so there is a single full boot here; the error-path scenario fails before that registration. - The test DB is loaded from a schema dump that doesn't mark every data migration as applied, so the boot runs with `FLEET_UPGRADES_ALLOW_MISSING_MIGRATIONS=1`. It adds ~2s to the `cmd/fleet` (`main`) test bundle, which is well off the CI critical path. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (verified locally: boots to /healthz, graceful shutdown, ~64% serve.go coverage) - Changes file: not applicable — internal test coverage with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved server shutdown handling to stop cleanly when the running command’s context is canceled, not only on OS signals. * Added stronger startup validation to fail fast for invalid Redis host-cache configuration (e.g., non-positive TTL). * **Tests** * Added an end-to-end test that boots the server against real MySQL/Redis, verifies graceful startup/shutdown, and confirms fast-fail behavior for misconfiguration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracts the MySQL datastore initialization out of
runServeCmdand into a newcmd/fleet/datastore.go. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517). Continues the path towardserve.go>60% coverage per the discussion on #33370.Three functions come out of the inline block:
initDatastore— builds the shared DB connections, the datastore, and the carve store (S3-backed when configured, otherwise the datastore itself).buildMySQLOpts— assembles the DB options: base logger and config, plus the optional read replica, dev SQL interceptor, and tracing.evalMigrationStatus— prints any operator guidance for the migration status and returns whetherrunServeCmdshould exit. Theos.Exitstays inrunServeCmd, so the boot/refuse-to-boot decision becomes unit-testable without the function terminating the test binary.Behavior is preserved —
runServeCmdcalls these in the same order with the same arguments, the migration-exit conditions are unchanged, and the fullcmd/fleetsuite passes against MySQL + Redis.initDatastorereturns early afterinitFatalso it's safe when the caller'sinitFataldoesn't terminate (the case in tests).On test scope:
TestEvalMigrationStatuscovers every migration status code across the dev-mode and allow-missing-migrations combinations — that's the real decision logic. I deliberately didn't add unit tests forinitDatastore/buildMySQLOpts: their only failure paths are paranoidinitFatalwrapping around constructors that don't dial at construction time, and the option builder returns opaque option closures. Those success paths are already exercised by booting the server, so a full datastore mock wasn't worth it for coverage's sake.Remaining slice per the broader plan: Redis init.
Related issue: Refs #33370
Checklist for submitter
Summary by CodeRabbit
Refactor
Tests