grafana / oncall

Developer-friendly incident response with brilliant Slack integration
GNU Affero General Public License v3.0
3.53k stars 294 forks source link

Can't configure oncall with zvonok with helm #2520

Open eugenefeel opened 1 year ago

eugenefeel commented 1 year ago

What went wrong?

What happened:

What did you expect to happen:

How do we reproduce it?

  1. Open Grafana OnCall and enable integration zvonok
  2. Restart celery and engine
  3. Wait for the celery to return traceback. Error message says: "│File "/etc/app/apps/phone_notifications/phone_provider.py", line 184, in get_phone_provider │ │ return _providers[live_settings.PHONE_PROVIDER] │ │ ~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ │ │ KeyError: 'test'

Grafana OnCall Version

v1.3.9

Product Area

Alert Flow & Configuration, Helm

Grafana OnCall Platform?

Kubernetes

eugenefeel commented 1 year ago

My colleagues found similar issue #2323

sreway commented 1 year ago

Hi, the issue arises when the environment variable PHONE_PROVIDER has an invalid or null value (default value is twilio), resulting in a KeyError exception here

eugenefeel commented 1 year ago

Yes. I reviewed code and uderstand it. I think that this wrong behavior. I could be wrong in PHONE_PROVIDER and what i'll do? I must have other way change this variable (with os.getenv) or maybe send me error about wrong phone_provider without save.

eugenefeel commented 1 year ago

General problem this issue is we can't change PHONE_PROVIDER after misstake. We get 500 status code and it's wrong. My point of view that we can't choose wrong provider. We can choose provider from avaliable. Also we can configure this is parameters with os.getenv

sreway commented 1 year ago

I made a PR that fixes this issue

eugenefeel commented 1 year ago

@sreway Thx for contributed. I'll try it and give you feed back. I have any question. Why If i add env in helm PHONE_PROVIDER=zvonok, neither changed. I hope if i create env, I can resolve this porblem, but isn't. Have you any idea?

sreway commented 1 year ago

Sorry, I don't have any ideas. When you add an environment variable in a Helm chart, it should be picked up by the deployed application if it is properly configured to read environment variables.

eugenefeel commented 1 year ago

hmm, okay. I'll try on weekend again. thx. Also we are waiting to approve the pull request :)

eugenefeel commented 1 year ago

UPD: If you change PHONE_PROVIDER in UI, it value will be add to tablespace base_livesetting and if you want change it with env you will can't. I think it's not normal, cos env need have high priority and if we change env this value coul'd be delete from this table. I resolve this problem with delete row in database, after my env was apply. @sreway what do u think?

sreway commented 1 year ago

It seems to me that it is functioning correctly. If you want to reset the value to the default value set by the environment variable, you can click on the "Reset to default" button.

eugenefeel commented 1 year ago

Yes, you right but if we stay without value check we can't reset to default.

eugenefeel commented 1 year ago

Won't close issue cos i want configure helm for configure zvonok