En/1672 disable health check log #1686
Conversation
| port := testutil.GetFreePort(t) | ||
|
|
||
| t.Setenv("HTTP_PORT", strconv.Itoa(port)) | ||
| t.Setenv("LOG_DISABLE_PROBES", "true") |
There was a problem hiding this comment.
You should use the constant you define in the other file
| @@ -9,6 +9,8 @@ import ( | |||
| "gofr.dev/pkg/gofr/config" | |||
| ) | |||
|
|
|||
| const LogDisableProbeKey = "LOG_DISABLE_PROBES" | |||
There was a problem hiding this comment.
The constant should have a clear comment about its purpose
Or simpl prefix it with EnvVar
| const LogDisableProbeKey = "LOG_DISABLE_PROBES" | |
| const EnvVarLogDisableProbeKey = "LOG_DISABLE_PROBES" |
| if ok, _ := strconv.ParseBool(middlewareConfigs[middleware.LogDisableProbeKey]); !ok { | ||
| r.Use(middleware.Logging(c.Logger)) | ||
| } else { | ||
| r.Use(middleware.LoggingSkipHealthCheck(c.Logger)) | ||
| } |
There was a problem hiding this comment.
This code looks strange to me
I mean, why having two middlewares?
There was a problem hiding this comment.
True. Same middleware behaviour should be changed.
There was a problem hiding this comment.
Okay, so changing how that logging middleware works now would mean we'd have to toss in a flag as an argument, and since that Logging function is out there for everyone, that'd be a big ol' breaking change. So, wouldn't it make more sense to just spin up a separate logging middleware and then just kinda decide whether to use it or not when we're setting up the server, based on that flag?
| @@ -9,6 +9,8 @@ import ( | |||
| "gofr.dev/pkg/gofr/config" | |||
| ) | |||
|
|
|||
| const LogDisableProbeKey = "LOG_DISABLE_PROBES" | |||
There was a problem hiding this comment.
Does this one need to be exported ?
There was a problem hiding this comment.
If followed this suggestion, then yes
| @@ -9,6 +9,8 @@ import ( | |||
| "gofr.dev/pkg/gofr/config" | |||
| ) | |||
|
|
|||
| const LogDisableProbeKey = "LOG_DISABLE_PROBES" | |||
There was a problem hiding this comment.
I would have used
| const LogDisableProbeKey = "LOG_DISABLE_PROBES" | |
| const LogDisableProbeKey = "LOG_PROBES_DISABLED" |
Or something like that. Maybe there are other env variables about disabling something
|
@kevin-lockhart-1001 Someone else has been implementing this as well: #1681 It might be a good idea to collaborate on this. |
Description:
Checklist:
goimportandgolangci-lint.