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

Update helm chart for newer grafana enable externalServiceAccounts #4876

Merged
merged 38 commits into from
Sep 5, 2024

Conversation

mderynck
Copy link
Contributor

@mderynck mderynck commented Aug 20, 2024

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

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • Added the relevant release notes label (see labels prefixed w/ release:). These labels dictate how your PR will
    show up in the autogenerated release notes.

@CLAassistant
Copy link

CLAassistant commented Aug 21, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ mderynck
❌ actions-user
You have signed the CLA already but the status is still pending? Let us recheck it.

@mderynck mderynck mentioned this pull request Aug 21, 2024
@mderynck mderynck added release:ignore PR will not be added to release notes pr:no public docs Added to a PR that does not require public documentation updates labels Sep 4, 2024
Copy link
Contributor

@matiasb matiasb left a 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):
Copy link
Contributor

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)

@mderynck mderynck merged commit 0efe51d into dev Sep 5, 2024
24 of 25 checks passed
@mderynck mderynck deleted the mderynck/plugin-init-update-helm branch September 5, 2024 18:18
Comment on lines 5 to 6
base_url: example.com
base_url_protocol: https
base_url_protocol: http

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?

Copy link

@lucasfcnunes lucasfcnunes Oct 14, 2024

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?

Comment on lines 649 to 656
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

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.

Copy link

@lucasfcnunes lucasfcnunes left a comment

Choose a reason for hiding this comment

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

about plugin provisioning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants