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

refactor: remove disablemetrics for enablemetrics #554

Conversation

nerdeveloper
Copy link
Member

Signed-off-by: nerdeveloper [email protected]

@scbizu
Copy link
Contributor

scbizu commented Feb 17, 2022

BTW , what issue the PR points to ?

@nerdeveloper
Copy link
Member Author

@scbizu the purpose of the PR is to disable metrics by default; this is the issue: #447

@cbuto
Copy link
Contributor

cbuto commented Feb 17, 2022

Just a note, we’ll also want to reflect this change in the helm chart and the docs after it’s released. @nerdeveloper let us know if you are up for that!

Also we’ll want to wait for v0.15.0 since this is a breaking change.

@nerdeveloper
Copy link
Member Author

Sure, I am definitely up for that! I can see on the docs: https://chartmuseum.com/docs/#prometheus-metrics

Also, I will refactor it and wait till v0.15.0 is out cc @jdolitsky

Signed-off-by: nerdeveloper <[email protected]>
@scbizu
Copy link
Contributor

scbizu commented Feb 18, 2022

Agree with @cbuto's idea , and this issue should be related to the charts and doc PR .

@scbizu scbizu added this to the v0.15.0 milestone Feb 18, 2022
Copy link
Contributor

@jdolitsky jdolitsky 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 tackling this.

Can you also modify the README section to describe the new behavior?
https://github.com/helm/chartmuseum#prometheus-metrics

Maybe even mention the now deprecated old flag in versions v0.14.0 and prior

@@ -94,9 +94,19 @@ var configVars = map[string]configVar{
Default: false,
CLIFlag: cli.BoolFlag{
Name: "disable-metrics",
Usage: "disable Prometheus metrics",
Usage: "(deprecated)disable Prometheus metrics",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a space between (deprecated) and disable Prometheus metrics ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will into this today.

@nerdeveloper
Copy link
Member Author

Thanks for tackling this.

Can you also modify the README section to describe the new behavior? https://github.com/helm/chartmuseum#prometheus-metrics

Maybe even mention the now deprecated old flag in versions v0.14.0 and prior

Definitely

@nerdeveloper
Copy link
Member Author

I have update chart museum Helm Chart for the Prometheus config here: chartmuseum/charts#38

@jdolitsky jdolitsky merged commit 49b460d into helm:main Feb 23, 2022
@nerdeveloper
Copy link
Member Author

OMG! Yes, it has been merged! I am so happy. thank you, team.

@nerdeveloper nerdeveloper deleted the fix/deprecate-disable-metrics-for-enable-metrics branch February 23, 2022 21:53
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