Final slog migration PR: test infrastructure + tools + remaining standalone files - #40727
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughThe change performs a large-scale migration from the repository's custom logging wrapper to Go's standard log/slog. It replaces imports of server/platform/logging with log/slog, updates logger construction to use slog.New(...) and slog.DiscardHandler, removes legacy wrapper helper constructors, introduces a new kitlogadapter to adapt *slog.Logger to the go-kit kitlog.Logger interface, removes the old kitlog adapter, and updates many call sites and test helpers to accept and pass *slog.Logger directly. go.mod is adjusted to move github.com/go-kit/log to an indirect requirement. Several public/internal function and struct fields were updated to use *slog.Logger where applicable. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
server/worker/apple_mdm_test.go (1)
101-103: Default test logger should be discard-first.Line 103 uses a JSON handler to
os.Stdout, which can flood CI logs for this suite. Preferslog.DiscardHandlerby default and keep stdout logging as an opt-in debug toggle.Suggested tweak
- // nopLog := slog.New(slog.DiscardHandler) - // use this to debug/verify details of calls - slogLog := slog.New(slog.NewJSONHandler(os.Stdout, nil)) + slogLog := slog.New(slog.DiscardHandler) + // use this to debug/verify details of calls: + // slogLog = slog.New(slog.NewJSONHandler(os.Stdout, nil))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/worker/apple_mdm_test.go` around lines 101 - 103, Change the default test logger from writing JSON to stdout to a discard-first logger: set slogLog to use slog.New(slog.DiscardHandler()) by default, and make the JSON stdout handler (slog.NewJSONHandler(os.Stdout, nil)) an opt-in toggle controlled by an environment variable or test flag (e.g., check os.Getenv("DEBUG_TEST_LOGS") or a testing flag in TestMain and swap slogLog to the JSON handler when enabled). Update the code that currently constructs slogLog so it references slog.DiscardHandler, and add the small conditional around slog.NewJSONHandler to avoid flooding CI logs while still allowing manual debug output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/worker/apple_mdm_test.go`:
- Around line 101-103: Change the default test logger from writing JSON to
stdout to a discard-first logger: set slogLog to use
slog.New(slog.DiscardHandler()) by default, and make the JSON stdout handler
(slog.NewJSONHandler(os.Stdout, nil)) an opt-in toggle controlled by an
environment variable or test flag (e.g., check os.Getenv("DEBUG_TEST_LOGS") or a
testing flag in TestMain and swap slogLog to the JSON handler when enabled).
Update the code that currently constructs slogLog so it references
slog.DiscardHandler, and add the small conditional around slog.NewJSONHandler to
avoid flooding CI logs while still allowing manual debug output.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmd/fleetctl/fleetctl/preview.goee/server/integrationtest/condaccess/suite.goee/server/integrationtest/hostidentity/suite.goee/server/integrationtest/scim/suite.gogo.modserver/fleet/mdm_test.goserver/mdm/android/service/enterprises_test.goserver/mdm/android/tests/testing_utils.goserver/service/integration_enterprise_test.goserver/service/integration_mdm_lifecycle_test.goserver/service/osquery_test.goserver/worker/apple_mdm_test.goserver/worker/db_migrations_test.goserver/worker/macos_setup_assistant_test.goserver/worker/worker_test.gotools/mdm/apple/apnspush/main.gotools/mdm/apple/applebmapi/main.gotools/mdm/apple/setupexperience/main.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40727 +/- ##
==========================================
- Coverage 66.30% 66.28% -0.02%
==========================================
Files 2468 2469 +1
Lines 197491 197481 -10
Branches 8772 8731 -41
==========================================
- Hits 130939 130896 -43
- Misses 54708 54740 +32
- Partials 11844 11845 +1
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates remaining standalone tools and test infrastructure from the legacy server/platform/logging wrappers to log/slog, continuing the broader slog migration across the Fleet codebase.
Changes:
- Update several Apple MDM debug tools and multiple server/worker tests to construct and pass
*slog.Loggerdirectly. - Update Android MDM test server utilities to use
*slog.Loggerwith go-kit HTTP server options/error handlers. - Adjust
go.modto demotegithub.com/go-kit/logto an indirect dependency as direct usage is removed.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/mdm/apple/setupexperience/main.go | Switch tool DB logging from platform/logging to direct slog logger. |
| tools/mdm/apple/applebmapi/main.go | Replace platform/logging usage with slog logger for datastore creation. |
| tools/mdm/apple/apnspush/main.go | Migrate tool logging to slog and pass *slog.Logger into NanoMDM logger adapter. |
| server/worker/worker_test.go | Update worker tests to use slog discard logger. |
| server/worker/macos_setup_assistant_test.go | Update worker + DEP service/client test wiring to accept *slog.Logger. |
| server/worker/db_migrations_test.go | Use *slog.Logger directly in DB migration worker tests. |
| server/worker/apple_mdm_test.go | Update Apple MDM worker tests to use slog handlers/loggers. |
| server/service/osquery_test.go | Replace JSON logger construction and pass *slog.Logger into logging context. |
| server/service/integration_mdm_lifecycle_test.go | Migrate integration test cron logger parameter to *slog.Logger. |
| server/service/integration_enterprise_test.go | Update uninstall migration test to pass *slog.Logger. |
| server/mdm/android/tests/testing_utils.go | Update android test server utilities to use *slog.Logger in error handling/request logging. |
| server/mdm/android/service/enterprises_test.go | Migrate android service unit tests to pass *slog.Logger into service constructor. |
| server/fleet/mdm_test.go | Update DEP client tests to use *slog.Logger. |
| go.mod | Move github.com/go-kit/log to indirect requirement. |
| ee/server/integrationtest/scim/suite.go | Construct test logger via slog-backed adapter. |
| ee/server/integrationtest/hostidentity/suite.go | Use *slog.Logger where required and keep adapter for server test opts. |
| ee/server/integrationtest/condaccess/suite.go | Same pattern as host identity: *slog.Logger for depot + adapter for server. |
| cmd/fleetctl/fleetctl/preview.go | Replace legacy logger with slog in fleetctl preview starter library flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
getvictor
changed the title
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/mdm_scep.go (1)
86-91:⚠️ Potential issue | 🟡 MinorThe
loggerparameter is accepted but not used.
NewSCEPServiceaccepts alogger *slog.Loggerparameter but ignores it, always initializingdebugLoggerwithslog.DiscardHandler. This means callers cannot configure logging for the SCEP service.🐛 Proposed fix to use the passed logger
func NewSCEPService(ds fleet.MDMAssetRetriever, signer scepserver.CSRSignerContext, logger *slog.Logger) scepserver.Service { + if logger == nil { + logger = slog.New(slog.DiscardHandler) + } return &service{ signer: signer, - debugLogger: slog.New(slog.DiscardHandler), + debugLogger: logger, ds: ds, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/mdm_scep.go` around lines 86 - 91, NewSCEPService ignores the provided logger and always sets debugLogger to slog.New(slog.DiscardHandler); change it to use the passed-in logger (falling back to a discard logger only if logger is nil). In the NewSCEPService constructor, set debugLogger to the provided logger (or to slog.New(slog.DiscardHandler) only when logger == nil) while leaving signer and ds assignments unchanged so callers can configure SCEP logging via the logger parameter.
🧹 Nitpick comments (4)
ee/server/integrationtest/condaccess/condaccess_test.go (1)
312-337: Optional: reuse one adapter instance instead of constructing twice.
kitlogadapter.NewLogger(s.Logger)is created at both call sites. Hoisting it once improves readability and avoids repetition.♻️ Proposed cleanup
func requestSCEPCertificateWithOptions(t *testing.T, s *Suite, uris []*url.URL, challenge string) (*http.Response, *scep.PKIMessage, *x509.Certificate) { ctx := context.Background() + scepLogger := kitlogadapter.NewLogger(s.Logger) @@ - msg, err := scep.NewCSRRequest(csr, pkiMsgReq, scep.WithLogger(kitlogadapter.NewLogger(s.Logger))) + msg, err := scep.NewCSRRequest(csr, pkiMsgReq, scep.WithLogger(scepLogger)) @@ - pkiMsgResp, err := scep.ParsePKIMessage(respBytes, scep.WithLogger(kitlogadapter.NewLogger(s.Logger)), scep.WithCACerts(msg.Recipients)) + pkiMsgResp, err := scep.ParsePKIMessage(respBytes, scep.WithLogger(scepLogger), scep.WithCACerts(msg.Recipients))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/integrationtest/condaccess/condaccess_test.go` around lines 312 - 337, The code creates kitlogadapter.NewLogger(s.Logger) twice (used in scep.NewCSRRequest and scep.ParsePKIMessage); hoist it into a single variable (e.g., logger := kitlogadapter.NewLogger(s.Logger)) before creating the CSR request and reuse that logger variable in both scep.NewCSRRequest(...) and scep.ParsePKIMessage(...), removing the duplicated constructor to improve readability and avoid repetition.server/mdm/scep/kitlogadapter/kitlogadapter.go (1)
18-20: Add nil-safe logger fallback in the adapter.
NewLogger(nil)currently creates a wrapper that can panic onLog/With. A nil-safe default keeps these boundary paths robust.♻️ Proposed hardening patch
func NewLogger(logger *slog.Logger) *Logger { + if logger == nil { + logger = slog.New(slog.DiscardHandler) + } return &Logger{logger: logger} } func (a *Logger) Log(keyvals ...any) error { + if a == nil || a.logger == nil { + return nil + } if len(keyvals) == 0 { return nil } @@ a.logger.LogAttrs(context.Background(), level, msg, attrs...) return nil } func (a *Logger) With(keyvals ...any) *Logger { + if a == nil || a.logger == nil { + return NewLogger(nil) + } return &Logger{logger: a.logger.With(keyvals...)} }Also applies to: 24-27, 55-56, 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/scep/kitlogadapter/kitlogadapter.go` around lines 18 - 20, NewLogger currently returns a Logger wrapper that will panic if passed nil; make it nil-safe by checking the incoming logger in NewLogger and substituting a safe default (e.g., slog.Default() or a no-op logger) when logger == nil, and ensure Logger.Log and Logger.With also guard against a nil underlying logger by delegating to that same safe default; update NewLogger, the Logger struct initialization, and the implementations of Logger.Log and Logger.With to use the fallback so calling NewLogger(nil).Log or .With cannot panic.pkg/mdm/mdmtest/apple.go (1)
785-785: Optional cleanup: reuse one adapter instance indoSCEP.You can create the kit logger once and pass it to both
scep.NewCSRRequestandscep.ParsePKIMessagefor slightly cleaner code.♻️ Proposed refactor
- msg, err := scep.NewCSRRequest(csr, pkiMsgReq, scep.WithLogger(kitlogadapter.NewLogger(logger))) + scepKitLogger := kitlogadapter.NewLogger(logger) + msg, err := scep.NewCSRRequest(csr, pkiMsgReq, scep.WithLogger(scepKitLogger)) if err != nil { return nil, nil, fmt.Errorf("create CSR request: %w", err) } respBytes, err := client.PKIOperation(ctx, msg.Raw) if err != nil { return nil, nil, fmt.Errorf("do CSR request: %w", err) } - pkiMsgResp, err := scep.ParsePKIMessage(respBytes, scep.WithLogger(kitlogadapter.NewLogger(logger)), scep.WithCACerts(msg.Recipients)) + pkiMsgResp, err := scep.ParsePKIMessage(respBytes, scep.WithLogger(scepKitLogger), scep.WithCACerts(msg.Recipients))Also applies to: 793-793
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mdm/mdmtest/apple.go` at line 785, In doSCEP, create a single kitlogger by calling kitlogadapter.NewLogger(logger) once and reuse that instance for both scep.NewCSRRequest(...) and scep.ParsePKIMessage(...), replacing the two separate kitlogadapter.NewLogger(logger) calls so both scep.NewCSRRequest and scep.ParsePKIMessage receive the same logger adapter.server/service/integration_mdm_test.go (1)
204-207: Optional: deduplicate test logger initialization.The same env-gated logger construction logic appears three times; extracting a small helper would reduce repetition.
♻️ Optional refactor
+newTestLogger := func() *slog.Logger { + if os.Getenv("FLEET_INTEGRATION_TESTS_DISABLE_LOG") != "" { + return slog.New(slog.DiscardHandler) + } + return slog.New(slog.NewJSONHandler(os.Stdout, nil)) +} - -pushLog := slog.New(slog.NewJSONHandler(os.Stdout, nil)) -if os.Getenv("FLEET_INTEGRATION_TESTS_DISABLE_LOG") != "" { - pushLog = slog.New(slog.DiscardHandler) -} +pushLog := newTestLogger() - -wlog := slog.New(slog.NewJSONHandler(os.Stdout, nil)) -if os.Getenv("FLEET_INTEGRATION_TESTS_DISABLE_LOG") != "" { - wlog = slog.New(slog.DiscardHandler) -} +wlog := newTestLogger() - -serverLogger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) -if os.Getenv("FLEET_INTEGRATION_TESTS_DISABLE_LOG") != "" { - serverLogger = slog.New(slog.DiscardHandler) -} +serverLogger := newTestLogger()Also applies to: 219-222, 280-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_test.go` around lines 204 - 207, The logger initialization for tests is duplicated; extract the env-gated logic into a small helper (e.g., newTestLogger or initTestLogger) that returns a *slog.Logger and encapsulates the use of slog.NewJSONHandler(os.Stdout, nil) vs slog.DiscardHandler based on os.Getenv("FLEET_INTEGRATION_TESTS_DISABLE_LOG"); replace the three occurrences where pushLog is set (the slog.New(...) blocks around pushLog) with calls to that helper to remove duplication while preserving the same behavior and variable usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/service/mdm_scep.go`:
- Around line 86-91: NewSCEPService ignores the provided logger and always sets
debugLogger to slog.New(slog.DiscardHandler); change it to use the passed-in
logger (falling back to a discard logger only if logger is nil). In the
NewSCEPService constructor, set debugLogger to the provided logger (or to
slog.New(slog.DiscardHandler) only when logger == nil) while leaving signer and
ds assignments unchanged so callers can configure SCEP logging via the logger
parameter.
---
Nitpick comments:
In `@ee/server/integrationtest/condaccess/condaccess_test.go`:
- Around line 312-337: The code creates kitlogadapter.NewLogger(s.Logger) twice
(used in scep.NewCSRRequest and scep.ParsePKIMessage); hoist it into a single
variable (e.g., logger := kitlogadapter.NewLogger(s.Logger)) before creating the
CSR request and reuse that logger variable in both scep.NewCSRRequest(...) and
scep.ParsePKIMessage(...), removing the duplicated constructor to improve
readability and avoid repetition.
In `@pkg/mdm/mdmtest/apple.go`:
- Line 785: In doSCEP, create a single kitlogger by calling
kitlogadapter.NewLogger(logger) once and reuse that instance for both
scep.NewCSRRequest(...) and scep.ParsePKIMessage(...), replacing the two
separate kitlogadapter.NewLogger(logger) calls so both scep.NewCSRRequest and
scep.ParsePKIMessage receive the same logger adapter.
In `@server/mdm/scep/kitlogadapter/kitlogadapter.go`:
- Around line 18-20: NewLogger currently returns a Logger wrapper that will
panic if passed nil; make it nil-safe by checking the incoming logger in
NewLogger and substituting a safe default (e.g., slog.Default() or a no-op
logger) when logger == nil, and ensure Logger.Log and Logger.With also guard
against a nil underlying logger by delegating to that same safe default; update
NewLogger, the Logger struct initialization, and the implementations of
Logger.Log and Logger.With to use the fallback so calling NewLogger(nil).Log or
.With cannot panic.
In `@server/service/integration_mdm_test.go`:
- Around line 204-207: The logger initialization for tests is duplicated;
extract the env-gated logic into a small helper (e.g., newTestLogger or
initTestLogger) that returns a *slog.Logger and encapsulates the use of
slog.NewJSONHandler(os.Stdout, nil) vs slog.DiscardHandler based on
os.Getenv("FLEET_INTEGRATION_TESTS_DISABLE_LOG"); replace the three occurrences
where pushLog is set (the slog.New(...) blocks around pushLog) with calls to
that helper to remove duplication while preserving the same behavior and
variable usage.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
cmd/fleetctl/fleetctl/preview.gocmd/fleetctl/fleetctl/query_test.gocmd/fleetctl/fleetctl/trigger_test.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.goee/orbit/pkg/scep/scep.goee/orbit/pkg/scep/scep_test.goee/server/integrationtest/condaccess/condaccess_test.goee/server/integrationtest/condaccess/suite.goee/server/integrationtest/hostidentity/hostidentity_test.goee/server/integrationtest/hostidentity/scep_rate_limit_test.goee/server/integrationtest/hostidentity/suite.goee/server/integrationtest/scim/suite.goee/server/service/condaccess/scep.goee/server/service/hostidentity/scep.goee/server/service/mdm_external_test.goee/server/service/testing_utils.gogo.modpkg/mdm/mdmtest/apple.goserver/fleet/mdm_test.goserver/launcher/server.goserver/mdm/android/service/enterprises_test.goserver/mdm/android/tests/testing_utils.goserver/mdm/scep/cmd/scepclient/scepclient.goserver/mdm/scep/kitlogadapter/kitlogadapter.goserver/mdm/scep/kitlogadapter/kitlogadapter_test.goserver/mdm/scep/server/service.goserver/mdm/scep/server/transport.goserver/platform/logging/kitlog_adapter.goserver/platform/logging/kitlog_adapter_test.goserver/platform/logging/logging.goserver/service/integration_enterprise_test.goserver/service/integration_install_test.goserver/service/integration_live_queries_test.goserver/service/integration_logger_test.goserver/service/integration_mdm_ddm_test.goserver/service/integration_mdm_lifecycle_test.goserver/service/integration_mdm_profiles_test.goserver/service/integration_mdm_test.goserver/service/integration_smtp_test.goserver/service/integration_sso_test.goserver/service/integrationtest/android/suite.goserver/service/integrationtest/scep_server/scep.goserver/service/integrationtest/suite.goserver/service/mdm_scep.goserver/service/osquery_test.goserver/service/testing_client.goserver/service/testing_utils.goserver/worker/apple_mdm_test.goserver/worker/db_migrations_test.goserver/worker/macos_setup_assistant_test.goserver/worker/worker_test.gotools/mdm/apple/apnspush/main.gotools/mdm/apple/applebmapi/main.gotools/mdm/apple/setupexperience/main.go
💤 Files with no reviewable changes (3)
- server/platform/logging/logging.go
- server/platform/logging/kitlog_adapter.go
- server/platform/logging/kitlog_adapter_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
getvictor
changed the title
getvictor
marked this pull request as ready for review
February 27, 2026 22:47
ksykulev
approved these changes
Feb 28, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me.
kitlogadapter +1
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
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.
Related issue: Resolves #40540
go-kit/log is no longer a direct dependency; moved kitlog adapter required for some 3rd party libraries into its own package
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit