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

Alerting: Support custom API URL for PagerDuty integration #88007

Merged
merged 3 commits into from
May 22, 2024
Merged

Alerting: Support custom API URL for PagerDuty integration #88007

merged 3 commits into from
May 22, 2024

Conversation

gaurav1999
Copy link
Contributor

@gaurav1999 gaurav1999 requested a review from a team as a code owner May 16, 2024 20:35
@gaurav1999 gaurav1999 requested review from jtheory, rwwiv, JacobsonMT, yuri-tceretian and grobinson-grafana and removed request for a team May 16, 2024 20:36
@CLAassistant
Copy link

CLAassistant commented May 16, 2024 •
edited

CLA assistant check
All committers have signed the CLA.

@gaurav1999
Copy link
Contributor Author

cc @yuri-tceretian

@grafana-pr-automation grafana-pr-automation bot added area/backend pr/external This PR is from external contributor labels May 16, 2024
@gaurav1999 gaurav1999 force-pushed the gaurav1999/fix-pagerduty-api-url branch from dd668cf to 61bf103 Compare May 16, 2024 20:39
yuri-tceretian
yuri-tceretian reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

A few comments.

Once the PR in alerting is merged, we need to update the reference to the new version of module.

pkg/services/ngalert/notifier/channels_config/available_channels.go Outdated Show resolved Hide resolved
pkg/services/ngalert/notifier/channels_config/available_channels.go Outdated Show resolved Hide resolved
pkg/services/ngalert/notifier/channels_config/available_channels.go Outdated Show resolved Hide resolved
@gaurav1999 gaurav1999 force-pushed the gaurav1999/fix-pagerduty-api-url branch from 61bf103 to a5988d9 Compare May 16, 2024 21:48
@yuri-tceretian yuri-tceretian changed the title Remove hardcoded pagerduty APIURL Alerting: Support custom API URL for PagerDuty integration May 17, 2024
@yuri-tceretian yuri-tceretian added this to the 11.1.x milestone May 17, 2024
@yuri-tceretian
Copy link
Contributor

Please run

go get github.com/grafana/alerting@282da04de5edea3138b2d5180ec439251705eb57

in the root of this repository. It will import a version of alerting module that contains your changes.
Please

  • commit go.mod and go.sum
  • in file available_channels.go add import alertingPagerduty "github.com/grafana/alerting/receivers/pagerduty" and update placeholder.

@gaurav1999 gaurav1999 force-pushed the gaurav1999/fix-pagerduty-api-url branch from a5988d9 to f53d282 Compare May 17, 2024 20:44
@gaurav1999 gaurav1999 requested a review from a team as a code owner May 17, 2024 20:44
@gaurav1999 gaurav1999 force-pushed the gaurav1999/fix-pagerduty-api-url branch from f53d282 to a289e7c Compare May 17, 2024 20:45
@gaurav1999
Copy link
Contributor Author

Please run

go get github.com/grafana/alerting@282da04de5edea3138b2d5180ec439251705eb57

in the root of this repository. It will import a version of alerting module that contains your changes. Please

  • commit go.mod and go.sum
  • in file available_channels.go add import alertingPagerduty "github.com/grafana/alerting/receivers/pagerduty" and update placeholder.

@yuri-tceretian Done from my side, thanks a ton!!

yuri-tceretian
yuri-tceretian approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

LGTM

@yuri-tceretian
Copy link
Contributor

Oh, CI fails because we forgot to change models for export:

gaurav1999 pushed a commit to gaurav1999/terraform-provider-grafana that referenced this pull request May 20, 2024
Remove hardocded pagerduty APIURL
6178f55 
- Fixes: grafana/grafana#82741
- Linked PR: grafana/grafana#88007
@gaurav1999 gaurav1999 mentioned this pull request May 20, 2024
Merged
@gaurav1999
Copy link
Contributor Author

Oh, CI fails because we forgot to change models for export:

@yuri-tceretian Sure thing, done -> grafana/terraform-provider-grafana#1576

gaurav1999 added a commit to gaurav1999/terraform-provider-grafana that referenced this pull request May 21, 2024
@gaurav1999
Remove hardocded pagerduty APIURL
5af2b77 
- Fixes: grafana/grafana#82741
- Linked PR: grafana/grafana#88007
@yuri-tceretian
Copy link
Contributor

@gaurav1999 it looks like you have committed from two accounts. Can you please make sure that you signed CLA with both accounts?

@gaurav1999
Copy link
Contributor Author

@gaurav1999 it looks like you have committed from two accounts. Can you please make sure that you signed CLA with both accounts?

Have reworked this, should be okay now, I have rebased it to my original author.

@yuri-tceretian
Copy link
Contributor

yuri-tceretian commented May 21, 2024 •
edited

Have reworked this, should be okay now, I have rebased it to my original author.

It is still blocked. It looks like you have to sign it with both accounts anyway.

@yuri-tceretian yuri-tceretian force-pushed the gaurav1999/fix-pagerduty-api-url branch from 8e4fdcf to f29ee09 Compare May 21, 2024 19:55
@yuri-tceretian
Copy link
Contributor

Nevermind, I squashed your commits into one. Should work now

julienduchesne added a commit to grafana/terraform-provider-grafana that referenced this pull request May 22, 2024
@gaurav1999 @julienduchesne
Remove hardocded pagerduty APIURL (#1576)
3c654b2 
* Remove hardocded pagerduty APIURL

- Fixes: grafana/grafana#82741
- Linked PR: grafana/grafana#88007

* CI Fixes for docs generation added

* go generate

* Add pack unpack for pagerduty url

* Add test

---------

Co-authored-by: Julien Duchesne <julien.duchesne@grafana.com>
@yuri-tceretian yuri-tceretian merged commit fdaa091 into grafana:main May 22, 2024
9 of 11 checks passed
@gaurav1999
Copy link
Contributor Author

@yuri-tceretian Thanks a ton! , loved the experience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuri-tceretian yuri-tceretian yuri-tceretian approved these changes

@jtheory jtheory Awaiting requested review from jtheory jtheory was automatically assigned from grafana/alerting-backend-product

@rwwiv rwwiv Awaiting requested review from rwwiv rwwiv was automatically assigned from grafana/alerting-backend-product

@JacobsonMT JacobsonMT Awaiting requested review from JacobsonMT JacobsonMT was automatically assigned from grafana/alerting-backend-product

@grobinson-grafana grobinson-grafana Awaiting requested review from grobinson-grafana grobinson-grafana was automatically assigned from grafana/alerting-backend-product

Assignees
No one assigned
Labels
add to changelog area/backend no-backport Skip backport of PR pr/external This PR is from external contributor
Projects
Alerting
Status: Done
Milestone
11.1.x
Development

Successfully merging this pull request may close these issues.

Allow user to choose between Pagerduty global and Pagerduty EU endpoint
3 participants
@gaurav1999 @CLAassistant @yuri-tceretian