fix(coderd/debug): fix caching issue with dismissed sections #11051
Conversation
| @@ -269,7 +298,7 @@ func loadDismissedHealthchecks(ctx context.Context, db database.Store, logger sl | |||
| } | |||
| } | |||
| if err != nil && !xerrors.Is(err, sql.ErrNoRows) { | |||
| logger.Error(ctx, "unable to fetch health settings: %w", err) | |||
| logger.Error(ctx, "unable to fetch health settings", slog.Error(err)) | |||
There was a problem hiding this comment.
review: drive-by: this was causing slog to panic
| @@ -269,7 +298,7 @@ func loadDismissedHealthchecks(ctx context.Context, db database.Store, logger sl | |||
| } | |||
| } | |||
| if err != nil && !xerrors.Is(err, sql.ErrNoRows) { | |||
| logger.Error(ctx, "unable to fetch health settings: %w", err) | |||
| logger.Error(ctx, "unable to fetch health settings", slog.Error(err)) | |||
| hc.WorkspaceProxy.Dismissed = true | ||
| } | ||
| } | ||
|
|
||
| format := r.URL.Query().Get("format") | ||
| switch format { | ||
| case "text": |
There was a problem hiding this comment.
not part of this PR: we could update it to use severity + dismissed as well.
There was a problem hiding this comment.
Will address in a follow-up.
| hc.WorkspaceProxy.Dismissed = true | ||
| } | ||
| } | ||
|
|
||
| format := r.URL.Query().Get("format") | ||
| switch format { | ||
| case "text": |
There was a problem hiding this comment.
not part of this PR: "simple" is not an available option?
There was a problem hiding this comment.
I thought the available formats were just json and text.
| return | ||
| } | ||
| } | ||
|
|
||
| func formatHealthcheck(ctx context.Context, rw http.ResponseWriter, r *http.Request, hc *healthcheck.Report) { | ||
| func formatHealthcheck(ctx context.Context, rw http.ResponseWriter, r *http.Request, hc healthcheck.Report, dismissed ...codersdk.HealthSection) { |
There was a problem hiding this comment.
nit: if you have switch to go over a finite number of sections, this does not have to be varags.
There was a problem hiding this comment.
Will address in a follow-up.
| func formatHealthcheck(ctx context.Context, rw http.ResponseWriter, r *http.Request, hc *healthcheck.Report) { | ||
| func formatHealthcheck(ctx context.Context, rw http.ResponseWriter, r *http.Request, hc healthcheck.Report, dismissed ...codersdk.HealthSection) { | ||
| // Mark any sections previously marked as dismissed. | ||
| for _, d := range dismissed { |
There was a problem hiding this comment.
I'm afraid that somebody may forget to extend this switch in the future. Maybe we should move the logic to codersdk, closer to Healthcheck report?
There was a problem hiding this comment.
Will address in a follow-up.
I don't remember the exact complaint, but it was something about a context key value. |
Previously we were making healthcheck sections as dismissed in the healthcheck function, the reuslt of which gets cached. This commit moves the logic outside of the function. It results in an extra database lookup on every call to healthcheck.