Skip to content

Saner and consistent YAML fields in config #2273

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

Merged
merged 2 commits into from
Mar 26, 2020

Conversation

gouthamve
Copy link
Contributor

Signed-off-by: Goutham Veeramachaneni [email protected]

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

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

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

If we're doing this, let's be consistent with our naming. URLs should have url in the name, duration fields should be _period, _interval, _time (max idle chunk time), booleans names like enable_ (not disable), etc. WDYT?

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

TY so much for taking this on. Now I can cross this off of my single-binary todo list haha. I have a few minor comments.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Very good job @gouthamve! I left few comments, but the overall changes make much sense to me. Please remember to mention the changes in the CHANGELOG.

@gouthamve gouthamve changed the title Saner YAML fields in config Saner and consistent YAML fields in config Mar 25, 2020
@bboreham
Copy link
Contributor

booleans names like enable_ (not disable)

Historically these were chosen so the zero value (false) is the default.
At some point we went to mandatory initialization of config structs, so maybe that reason has gone, but I don't think there is a strong argument just on consistency.

For many flags the default is "what Cortex did earlier" but should be changed to "what most people would use". I think this type of change is much more important.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM! I left a minor nit. Please don't forget the CHANGELOG entry.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
@gouthamve gouthamve merged commit 12b15b9 into cortexproject:master Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants