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

mimirtool: fail on duplicate rules with --strict #5035

Merged
merged 6 commits into from
May 18, 2023

Conversation

colega
Copy link
Contributor

@colega colega commented May 17, 2023

What this PR does

Changes the behaviour of mimirtool rules check to fail on duplicated rules when --strict is provided.

Also changed the output to use the logger instead of fmt, and added a test.

The output looks like this (on main before #5023 was merged):

$ go run ./cmd/mimirtool rules check --strict --rule-dirs ./operations/mimir-mixin-compiled
ERRO[0000] duplicate rules found                         duplicate_rules=2 error="rules should emit unique series, to avoid producing inconsistencies while recording expressions" namespace=alerts
ERRO[0000] duplicate rule                                label[severity]=warning metric=MimirRolloutStuck namespace=alerts
ERRO[0000] duplicate rule                                label[severity]=critical metric=MimirCompactorHasNotUploadedBlocks namespace=alerts
mimirtool: error: 2 duplicate rules found in namespace "alerts", try --help
exit status 1

Which issue(s) this PR fixes or relates to

Relates to #5023

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega colega marked this pull request as ready for review May 17, 2023 10:51
@colega colega requested a review from a team as a code owner May 17, 2023 10:51
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

The implementation looks solid, but there are some minor issues with the tests.

pkg/mimirtool/commands/rules_test.go Outdated Show resolved Hide resolved
pkg/mimirtool/commands/rules_test.go Outdated Show resolved Hide resolved
pkg/mimirtool/commands/rules_test.go Show resolved Hide resolved
pkg/mimirtool/commands/rules_test.go Show resolved Hide resolved
pkg/mimirtool/commands/rules_test.go Show resolved Hide resolved
colega and others added 3 commits May 18, 2023 10:59
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Copy link
Contributor Author

colega commented May 18, 2023

Thank you for your review, @aknuds1, the comments should be addressed now.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing my feedback!

@colega colega enabled auto-merge (squash) May 18, 2023 09:20
@colega colega merged commit aec9b8a into main May 18, 2023
@colega colega deleted the fail-on-duplicate-rules-when-strict branch May 18, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants