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

Can't configure oncall with zvonok with helm #2520

Open
eugenefeel opened this issue Jul 13, 2023 · 12 comments
Open

Can't configure oncall with zvonok with helm #2520

eugenefeel opened this issue Jul 13, 2023 · 12 comments

Comments

@eugenefeel
Copy link

eugenefeel commented Jul 13, 2023

What went wrong?

What happened:

  • Last week ago was added integration with service zvonok add zvonok integration #2339
  • If enable integration you will get different problems, because you didn't add service variables and you will can't use oncall. (blank interface)
    ScreenShot-2023-07-13
    ScreenShot-2023-07-13

What did you expect to happen:

    1. @sreway you can rewrite your code without this error. If we haven't env, you can print about it in interface(Add exception)
    1. I will can create pull requests for update helm manifest

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
Copy link
Author

My colleagues found similar issue #2323

@sreway
Copy link
Contributor

sreway commented Jul 13, 2023

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
Copy link
Author

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
Copy link
Author

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 sreway mentioned this issue Jul 13, 2023
3 tasks
@sreway
Copy link
Contributor

sreway commented Jul 13, 2023

I made a PR that fixes this issue

@eugenefeel
Copy link
Author

@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
Copy link
Contributor

sreway commented Jul 13, 2023

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
Copy link
Author

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

@eugenefeel
Copy link
Author

eugenefeel commented Jul 16, 2023

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
Copy link
Contributor

sreway commented Jul 17, 2023

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
Copy link
Author

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

github-merge-queue bot pushed a commit that referenced this issue Jul 17, 2023
# What this PR does

Sets a default value for the PHONE_PROVIDER setting and replaces the
value of PHONE_PROVIDER with this default value if it is not valid.

## Which issue(s) this PR fixes

- [#2520](#2520)
- [#2323](#2323)

## Checklist

- [ ] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required)
- [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)

---------

Co-authored-by: Innokentii Konstantinov <innokenty.konstantinov@grafana.com>
Co-authored-by: Joey Orlando <joseph.t.orlando@gmail.com>
Co-authored-by: Joey Orlando <joey.orlando@grafana.com>
@eugenefeel
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants