-
Notifications
You must be signed in to change notification settings - Fork 288
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
Update helm chart for newer grafana enable externalServiceAccounts #4876
Conversation
|
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.
LGTM
README.md
Outdated
5. Go to [OnCall Plugin Configuration](http://localhost:3000/plugins/grafana-oncall-app), using log in credentials | ||
as defined above: `admin`/`admin` (or find OnCall plugin in configuration->plugins) and connect OnCall _plugin_ | ||
with OnCall _backend_: | ||
5. Provision the plugin (If you run Grafana outside the included docker files, install the plugin before these steps): |
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.
Maybe some mention about using the right Grafana admin credentials? (just in case not using default, similar as stated in the Troubleshooting section below)
base_url: example.com | ||
base_url_protocol: https | ||
base_url_protocol: http |
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.
base_url_protocol
isn't the change from https
to http
a breaking change and maybe a security downgrade?
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.
Shouldn't this helm-testing-grafana-plugin-provisioning
have for instance apps.0.jsonData.onCallApiUrl
, apps.0.jsonData.orgId
, apps.0.disabled
, ... all templated?
*-testing-*
shouldn't be the name of something used in prod, right?
Shouldn't this configmap be toggleable?
extraVolumes: | ||
- name: provisioning | ||
configMap: | ||
name: helm-testing-grafana-plugin-provisioning | ||
extraVolumeMounts: | ||
- name: provisioning | ||
mountPath: /etc/grafana/provisioning/plugins/grafana-oncall-app-provisioning.yaml | ||
subPath: grafana-oncall-app-provisioning.yaml |
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 plugin provisioning logic should be done in the templates. If someone overrides extraVolumes
or extraVolumeMounts
this won't work.
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.
about plugin provisioning
What this PR does
Updates the helm chart and docker compose files with the required changes to support the plugin initialization changes. Updated instructions on the README.md show how to setup & intialize OnCall without needing to go to the configuration page, this is currently the preferred method.
Which issue(s) this PR closes
Related to [issue link here]
Checklist
pr:no public docs
PR label added if not required)release:
). These labels dictate how your PR willshow up in the autogenerated release notes.