chore(heartbeat): Refactor heartbeat. #344
Conversation
internal/heartbeat/heartbeat.go
Outdated
| "github.com/contentsquare/chproxy/config" | ||
| ) | ||
|
|
||
| var ErrUnexpectedResponse = fmt.Errorf("unexpected response") |
There was a problem hiding this comment.
since we only use this variable once and only in this package, it might be better to rename it errUnexpectedResponse to avoid polluting the IDE with "useless" variables when using autocomplete (and because it seems cleaner conceptually)
internal/heartbeat/heartbeat.go
Outdated
| } | ||
|
|
||
| // User credentials are not needed | ||
| const DefaultEndpoint string = "/ping" |
There was a problem hiding this comment.
some comment as the one for ErrUnexpectedResponse
internal/heartbeat/heartbeat.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if hb.request != DefaultEndpoint && hb.user != "" { |
There was a problem hiding this comment.
although it was in the previous code, the hb.user != "" seems useless because this condition is not possible at this point of the code since the NewHeartbeat use a default user in the worst case scenario and this default user must be not empty cf config.go:L461 & proxy.go:L669 .
If we don't want to rely on business code outside of this package (although the test coverage should spot this issue), it might be cleaner to do a check in the func NewHeartbeat(c config.HeartBeat, options ...Option) HeartBeat that there is at least a user not empty (and return an error otherwise). Because, with the current logic, if there is a regression outside of the package, it might result of have the authentication not working.
There was a problem hiding this comment.
That is a good point, I included a check in NewHeartbeat to make sure that authentication can't be broken by forgetting to pass a default user.
This is mostly an enablement PR for future follow-ups (e.g. TCP healthcheck). Signed-off-by: Lennard Eijsackers <lennardeijsackers92@gmail.com>
Blokje5
force-pushed
the
chore/refactor-heartbeat
branch
from
25a963e to
4899a1b
Compare
Description
This is mostly an enablement PR for future follow-ups (e.g. TCP healthcheck). This PR decouples the Heartbeat from e.g. ClusterUser and moves it to a seperate package. I also changed the tests to focus on the actual HeartBeat functionality as opposed of asserting on the struct itself.
Pull request type
Please check the type of change your PR introduces:
Checklist
Does this introduce a breaking change?
Further comments