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.

Sign up for GitHub

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

Jump to bottom

health: make Opsgenie API URL configurable #10561

Merged
merged 7 commits into from Feb 11, 2021
Merged

health: make Opsgenie API URL configurable #10561

merged 7 commits into from Feb 11, 2021

Conversation

tinyhammers
Copy link
Contributor

@tinyhammers tinyhammers commented Jan 27, 2021 •
edited by ilyam8

Add OPSGENIE_API_URL="" to so user can specify API endpoint

Summary

Fixes: #10556

Component Name
Test Plan

This has been tested on our production infrastructure and is working

Additional Information

@tinyhammers
Update health_alarm_notify.conf
ab45014 
Add OPSGENIE_API_URL="" to specify API endpoint
@tinyhammers tinyhammers mentioned this pull request Jan 27, 2021
Closed
thiagoftsm
thiagoftsm previously approved these changes Jan 27, 2021
View reviewed changes
@ilyam8 ilyam8 mentioned this pull request Jan 27, 2021
Closed
Ferroin
Ferroin previously approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@Ferroin Ferroin left a comment

Choose a reason for hiding this comment

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

Approving in context, but I largely agree with @ilyam8 that this should probably be merged with #10562.

thiagoftsm
thiagoftsm previously approved these changes Jan 27, 2021
View reviewed changes
@tinyhammers
Update alarm-notify.sh.in
c1be623 
Add default value of https://api.opsgenie.com for OPSGENIE_API_URL when unset
Substitute OPSGENIE_API_URL for URL on line 2182
@tinyhammers tinyhammers dismissed stale reviews from thiagoftsm and Ferroin via c1be623 February 1, 2021 13:49
@tinyhammers
Update alarm-notify.sh.in
7707e6b 
Re-added parenthesis deleted in error on ln 2180
@tinyhammers
Update README.md
b43eb8e 
Amend to reflect OPSGENIE_API_URL variable
Ferroin
Ferroin previously approved these changes Feb 1, 2021
View reviewed changes
@tinyhammers
Update health_alarm_notify.conf
db489de 
Remove hardcoded URL as now has default if unset
tinyhammers
tinyhammers commented Feb 1, 2021
View reviewed changes
Copy link
Contributor Author

@tinyhammers tinyhammers left a comment

Choose a reason for hiding this comment

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

OK should be good to go, even replaced the ) I deleted by mistake 😂

health/notifications/health_alarm_notify.conf Outdated Show resolved Hide resolved
Ferroin
Ferroin previously approved these changes Feb 1, 2021
View reviewed changes
@Ferroin
Copy link
Member

Ferroin commented Feb 1, 2021

OK should be good to go, even replaced the ) I deleted by mistake

Thanks for the work on this. It looks good to go from my perspective, but as of right now is probably not going to be merged until after the v1.29 release (currently scheduled for tomorrow) as we’re in a code freeze right now in preparation for the release and the hardcoded URL is not exactly a critical bug.

@tinyhammers
Copy link
Contributor Author

Yeah, no worries @Ferroin.
I should've pulled my finger out last week to get it in v1.29.

joelhans
joelhans previously approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@joelhans joelhans left a comment

Choose a reason for hiding this comment

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

Docs part of this LGTM!

@thiagoftsm
Copy link
Contributor

Hello @tinyhammers ,

I tested your PR right now with the old configuration I had from OPSGENIE, with this old configuration I had only OPSGENIE_API_KEY set and I got this error.

curl: (3) URL using bad/illegal format or missing URL
2021-02-01 15:57:22: alarm-notify.sh: ERROR: failed to send opsgenie notification for: hades test.chart.test_alarm is WARNING, with HTTP error code 000.

Considering this, the changes you brought is not keeping compatibily with old configuration, please, can you take a look on this before we merge your PR?

ilyam8
ilyam8 requested changes Feb 1, 2021
View reviewed changes
health/notifications/alarm-notify.sh.in Outdated Show resolved Hide resolved
@tinyhammers
Update alarm-notify.sh.in
e087be1 
Moved OPSGENIE_API_URL=${OPSGENIE_API_URL:-"https://api.opsgenie.com"} from line 394 to line 415 as per comment netdata#10561 (comment)
@tinyhammers tinyhammers dismissed stale reviews from joelhans and Ferroin via e087be1 February 4, 2021 13:04
@ilyam8 ilyam8 changed the title Update health_alarm_notify.conf health: make Ospgenie API URL configurable Feb 4, 2021
@ilyam8 ilyam8 changed the title health: make Ospgenie API URL configurable health: make Opsgenie API URL configurable Feb 4, 2021
thiagoftsm
thiagoftsm approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@thiagoftsm thiagoftsm left a comment

Choose a reason for hiding this comment

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

After the last commit, I can verify that everything is working with old configuration.

Thank you very much @tinyhammers !

@Ferroin
Copy link
Member

Ferroin commented Feb 4, 2021

The merge of this will either have to wait until we’re out of the post-release soft code freeze, or would require changing the target branch from master to develop as it’s technically a new feature and not a bugfix.

@tinyhammers It’s up to you whether you want to wait the week or two for the soft freeze to end or just rebase on top of develop.

@tinyhammers
Copy link
Contributor Author

Happy to wait :)

@odyslam
Copy link
Contributor

odyslam commented Feb 10, 2021

Now that we got the release out, we are good to go @Ferroin ?

@Ferroin
Copy link
Member

Ferroin commented Feb 10, 2021

Now that we got the release out, we are good to go @Ferroin ?

We’re waiting until tomorrow (2020-02-11) to verify that no other major bugs are discovered after the recent patch release, but this PR is right at the top of my list to merge once the code freeze is over.

@Ferroin Ferroin merged commit 877a17b into netdata:master Feb 11, 2021
@tinyhammers tinyhammers deleted the patch-2 branch February 11, 2021 19:48
Saruspete pushed a commit to Saruspete/netdata that referenced this pull request Mar 17, 2021
@tinyhammers @Saruspete
health: make Opsgenie API URL configurable (netdata#10561)
b7d372f 
* Update health_alarm_notify.conf

Add OPSGENIE_API_URL="" to specify API endpoint

* Update health_alarm_notify.conf

Add URL for sanity

* Update alarm-notify.sh.in

Add default value of https://api.opsgenie.com for OPSGENIE_API_URL when unset
Substitute OPSGENIE_API_URL for URL on line 2182

* Update alarm-notify.sh.in

Re-added parenthesis deleted in error on ln 2180

* Update README.md

Amend to reflect OPSGENIE_API_URL variable

* Update health_alarm_notify.conf

Remove hardcoded URL as now has default if unset

* Update alarm-notify.sh.in

Moved OPSGENIE_API_URL=${OPSGENIE_API_URL:-"https://api.opsgenie.com"} from line 394 to line 415 as per comment netdata#10561 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilyam8 ilyam8 ilyam8 approved these changes

@thiagoftsm thiagoftsm thiagoftsm approved these changes

@Ferroin Ferroin Ferroin left review comments

@joelhans joelhans joelhans left review comments

Assignees
No one assigned
Labels
area/docs area/health
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpsGenie API URL is hardcoded into alarm-notify.sh, more an oversight than a bug
7 participants
@tinyhammers @Ferroin @thiagoftsm @odyslam @joelhans @ilyam8 @vlvkobal