Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make alerting package stateful and directly error-logging. Fix the rest of the unchecked errors. #505

Merged
merged 7 commits into from
Apr 23, 2024

Conversation

jshufro
Copy link
Contributor

@jshufro jshufro commented Apr 21, 2024

The alerting package previously was stateless and simply recreated state every time any alert was called.

Now it is stateful, and only checks config on init(). Logging is done through a logger instead of through stdout.

Fixes errcheck issues and uses newest rocketpool-go/v2 and node-manager-core commits which resolve upstream errcheck issues.

Enables all linters 🎉

Copy link
Member

@jclapis jclapis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, have a few error messages to embellish but they won't block the PR; they can come later with the master merge.

Copy link
Member

@jclapis jclapis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added said embellishments in as comments.

rocketpool-cli/commands/service/config.go Show resolved Hide resolved
rocketpool-daemon/node/metrics-exporter.go Show resolved Hide resolved
rocketpool-daemon/node/node.go Outdated Show resolved Hide resolved
rocketpool-daemon/node/node.go Outdated Show resolved Hide resolved
@jshufro jshufro requested a review from jclapis April 23, 2024 13:32
Copy link
Member

@jclapis jclapis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers!

@jclapis jclapis merged commit 75f7994 into rocket-pool:v2 Apr 23, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants