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

Mixin: Follow naming convention for bucketReplicate #4859

Merged

Conversation

jessicalins
Copy link
Contributor

Signed-off-by: Jéssica Lins [email protected]

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

As discussed here and to follow up on this TODO:

// TODO(kakkoyun): Fix naming convention: bucketReplicate
bucket_replicate+:: {

I just renamed bucket_replicate -> bucketReplicate to keep the naming convention consistent.

Verification

After the changes I've ran make examples to regenerate the files.

absent(up{job=~".*thanos-bucket-replicate.*"} == 1)
for: 5m
labels:
severity: critical
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this alert was removed at #3856. Is this an intended change? @kakkoyun

Copy link
Member

Choose a reason for hiding this comment

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

Yes, there is another generator that handles the generation of these alerts. This was redundant.

saswatamcode
saswatamcode previously approved these changes Nov 13, 2021
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks for this! 💫
Docs check seems to fail due to examples/alerts/alerts.yaml not being updated. I think running make examples-clean and then make docs will fix this.

Maybe this is a Makefile issue due to examples/alerts/alerts.yaml not being phony. 🤔

Jéssica Lins added 3 commits November 15, 2021 14:08
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for taking care of this.

I have already commented inline, it would be amazing to keep snake_case for filenames but use camelCase for variable names in code.

absent(up{job=~".*thanos-bucket-replicate.*"} == 1)
for: 5m
labels:
severity: critical
Copy link
Member

Choose a reason for hiding this comment

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

Yes, there is another generator that handles the generation of these alerts. This was redundant.

examples/alerts/alerts.yaml Show resolved Hide resolved
examples/dashboards/dashboards.md Outdated Show resolved Hide resolved
mixin/README.md Show resolved Hide resolved
kakkoyun
kakkoyun previously approved these changes Nov 18, 2021
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Signed-off-by: Jéssica Lins <[email protected]>
auto-merge was automatically disabled November 18, 2021 16:47

Head branch was pushed to by a user without write access

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Not sure why CodeQL job is queued. Merge it manually...

@yeya24 yeya24 merged commit d2d74da into thanos-io:main Nov 19, 2021
@jessicalins jessicalins deleted the fix-naming-convention-bucket-replicate branch November 19, 2021 14:48
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.

4 participants