-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix(deployment): Get the helm chart working with Perses 0.47.0 #16
Conversation
40725ba
to
6ae97dd
Compare
Signed-off-by: Ronny Trommer <[email protected]>
Update the schemas and default datasource to get it back working with the latest release of Perses. Signed-off-by: Ronny Trommer <[email protected]>
6ae97dd
to
cc460de
Compare
charts/perses/Chart.yaml
Outdated
@@ -3,8 +3,8 @@ name: perses | |||
description: Perses helm chart | |||
icon: https://avatars.githubusercontent.com/u/77209215?s=200&v=4 | |||
type: application | |||
version: 0.3.0 | |||
appVersion: "0.42.1" | |||
version: 1.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning here for 1.0.0, the changes I made are technically not backward compatible with older versions from a semver.org perspective. If you want a different version bump here to indicate some fragility, I say yes to any suggestions you want to make here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the semver point of view, v0 is showing that the project is still under development. Under a v0, it doesn't need to be backward compatible, so it's ok to just bump the version to 0.4.0
charts/perses/values.yaml
Outdated
directUrl: http://localhost:9090 | ||
scrapeInterval: "15s" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This here is also a good one for feedback, to make it work out without having the user worry too much, I created a data source by default. If you prefer to keep that as a comment here, no objections, and just let me know what you prefer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I would prefer to keep it commented. In production environment, it would be annoying to have this datasource.
Probably to make it work easily for user, an idea could be to see prometheus-operator deploying Perses, like it deploys Grafana.
f3f69f4
to
cc460de
Compare
Regnerate the README using make update-helm-readme to reflect the versions and paths for the schemas accordingly. Signed-off-by: Ronny Trommer <[email protected]>
4bdb810
to
b7a87c6
Compare
charts/perses/values.yaml
Outdated
queries_path: "/etc/perses/cue/schemas/queries" | ||
datasources_path: "/etc/perses/cue/schemas/datasources" | ||
variables_path: "/etc/perses/cue/schemas/variables" | ||
interval: "6h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nexucis I have seen here different default values, the values.schema.json has it referenced as 5m, the docs 1h. What is a reasonable consistent default here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial idea was 5m. That was in case the watching file is failing to update the schemas loaded.
Also after many issues we ran into with cuelang, keeping 5m
is a good value because it helps to trash the cuelang context that is leaking. So it mitigates this memory leak
awesome work @indigo423 thank you for doing it. I think I have answered all your hints and comments. Excepting few points raised, it looks good to me |
@Nexucis thanks for the quick feedback and changed all the bits an pieces. |
a38bb5c
to
539abc2
Compare
Apply feedback regarding new version for the Helm chart, default settings for the dashboard. A prometheus data source is commented in. Regenerated the README with the new version number and defaults. Signed-off-by: Ronny Trommer <[email protected]>
539abc2
to
2eba034
Compare
# plugin: | ||
# kind: PrometheusDatasource | ||
# spec: | ||
# directUrl: https://prometheus.demo.do.prometheus.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nexucis I've changed this back to the original content to introduce fewer changes with the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect ! Thank you @indigo423 for the changes !
Investigate what minimal changes are required to get the Helm chart back working with the latest Perses 0.47.0 release.
I've added a few reviewer hints as a comment here in the PR.
Resolves: #15