Add service_region support #384

jbfavre · 2021-09-09T11:15:03Z

In order to be able to use Pagerduty provider to manage EU platform deployment, we need to be able to define a custom API URL 0

As gar as I can tell, heimweh/go-pagerduty client use baseURL which is already configurable.
I tried to pass this configuration option when setting up heimweh/go-pagerduty.

@stmcallister please be indulgent, this is one of my first contribution in Go language 😉
I surely missed many things.

stmcallister · 2021-09-09T19:27:43Z

Well done! This project was my first using the go language as well! Code-wise, I think it's great. However, I think I want to go a slightly different direction. Rather than having folks set the URLs directly I think we should just provide a service_region field to the provider tag. Then the provider will figure out which URLs to use. At least that's my thoughts right now.

From a user perspective, do you think that would be a cleaner approach?

jbfavre · 2021-09-10T12:15:11Z

I understand and agree that having only one configuration option would help to avoid misbehaviours.

think we should just provide a service_region field to the provider tag

Do you mean, like:

provider "pagerduty" {
  token          = xXxXxX
  service_region = "EU"
}

The only caveat I can think of is that having "hard coded" URLs would require a provider release for each new service region opened. It's not that I expect many service region opening, but still I think we should look for the easiest solution.

Maybe dynamically building URLs would help. Kind of:

if service_region == "" or service_region == "us"
then
  ApiUrl = "https://api.pagerduty.com"
  AppUrl = "https://app.pagerduty.com"
else
  ApiUrl = "https://api." + service_region + ".pagerduty.com"
  AppUrl = "https://app." + service_region + ".pagerduty.com"

stmcallister

Great work! Thank you for this! Just a couple changes before approving.

pagerduty/provider.go

stmcallister · 2021-10-21T00:33:36Z

Hey @jbfavre ! When I run the acceptance tests for the SlackConnection it's trying to POST to https://api.pagerduty.com rather than https://app.pagerduty.com.

Will you update your branch with the latest master and let me know if your SlackConnecting acceptance tests pass? Thanks!

jbfavre · 2021-10-21T08:25:51Z

Hey @jbfavre ! When I run the acceptance tests for the SlackConnection it's trying to POST to https://api.pagerduty.com rather than https://app.pagerduty.com.

Will you update your branch with the latest master and let me know if your SlackConnecting acceptance tests pass? Thanks!

Hello @stmcallister
I'm currently trying to run the acceptance tests. Will keep you updated

jbfavre · 2021-10-21T09:21:17Z

Hey @jbfavre ! When I run the acceptance tests for the SlackConnection it's trying to POST to https://api.pagerduty.com rather than https://app.pagerduty.com.

Will you update your branch with the latest master and let me know if your SlackConnecting acceptance tests pass? Thanks!

Well, I'm pretty sure I'm missing something, but I can't even run acceptance tests on master 🤔
I'm using:

a dev PagerDuty account
a free slack account
PagerDuty has been installed in Slack
Pagerduty and Slack accounts are linked
I've a User PagerDuty Token defined with export PAGERDUTY_USER_TOKEN=xxx
I've a Global PagerDuty Token defined with export PAGERDUTY_TOKEN=xxx
Slack Workspace ID is declared with export SLACK_CONNECTION_WORKSPACE_ID=
Slack Channel ID is statically declared in pagerduty/resource_pagerduty_slack_connection_test.go

Any advice welcome 😬

jbfavre · 2021-10-21T12:41:27Z

OK, made some progress.

you must enable Incident Priority in Account Settings > Incident Priorities
make testacc runs, slowly, for v2.0.0 tag, with "only" 2 errors related to Slack tests:

=== RUN   TestAccPagerDutySlackConnection_import
    import_pagerduty_slack_connection_test.go:18: Step 1/3 error: Error running apply: exit status 1
        
        Error: POST API call to https://api.pagerduty.com/users failed 400 Bad Request. Code: 2001, Errors: [You have hit your regular user limit], Message: Invalid Input Provided
        
          with pagerduty_user.foo,
          on terraform_plugin_test.tf line 2, in resource "pagerduty_user" "foo":
           2: 	resource "pagerduty_user" "foo" {
        
--- FAIL: TestAccPagerDutySlackConnection_import (2.27s)
=== RUN   TestAccPagerDutySlackConnectionTeam_import
--- PASS: TestAccPagerDutySlackConnectionTeam_import (8.41s)

and

=== RUN   TestAccPagerDutySlackConnection_Basic
    resource_pagerduty_slack_connection_test.go:29: Step 1/2 error: Error running apply: exit status 1
        
        Error: POST API call to https://api.pagerduty.com/users failed 400 Bad Request. Code: 2001, Errors: [You have hit your regular user limit], Message: Invalid Input Provided
        
          with pagerduty_user.foo,
          on terraform_plugin_test.tf line 2, in resource "pagerduty_user" "foo":
           2: 	resource "pagerduty_user" "foo" {
        
--- FAIL: TestAccPagerDutySlackConnection_Basic (2.44s)
=== RUN   TestAccPagerDutySlackConnection_Team
--- PASS: TestAccPagerDutySlackConnection_Team (11.32s)
=== RUN   TestAccPagerDutySlackConnection_Envar
--- PASS: TestAccPagerDutySlackConnection_Envar (6.83s)

However, I got plenty of other errors, like:

Error: POST API call to https://api.pagerduty.com/users failed 400 Bad Request. Code: 2001, Errors: [You have hit your regular user limit], Message: Invalid Input Provided

Don't know whether it's because I'm using a dev account a not a "real" one. But I'm relunctant to use our production setup to do my tests.

Now trying to run tests for my branch

jbfavre · 2021-10-21T12:43:49Z

Indeed, I broke some tests in my branch 🤔

=== RUN   TestAccPagerDutySlackConnectionTeam_import
    import_pagerduty_slack_connection_test.go:42: Step 1/2 error: Error running apply: exit status 1
        
        Error: POST API call to https://api.pagerduty.com/integration-slack/workspaces/T02JBV9PANA/connections failed 404 Not Found. Code: 2100, Errors: <nil>, Message: Not Found
        
          with pagerduty_slack_connection.foo,
          on terraform_plugin_test.tf line 5, in resource "pagerduty_slack_connection" "foo":
           5: 		resource "pagerduty_slack_connection" "foo" {
        
--- FAIL: TestAccPagerDutySlackConnectionTeam_import (125.60s)

Ill have an look at it 👀

stmcallister · 2021-10-21T16:08:52Z

@jbfavre Yeah, the errors on users are because you're using a Developer Account. I'm only worried about the TestAccPagerDutySlackConnection tests passing on your end. I can run the other tests on my account. You can run your tests with this command. Thanks for taking a look!

terraform-provider-pagerduty $ TF_ACC=1 go test -run "TestAccPagerDutySlackConnection" ./pagerduty -v -timeout 120m

jbfavre · 2021-10-22T07:04:18Z

@jbfavre Yeah, the errors on users are because you're using a Developer Account. I'm only worried about the TestAccPagerDutySlackConnection tests passing on your end. I can run the other tests on my account. You can run your tests with this command. Thanks for taking a look!

terraform-provider-pagerduty $ TF_ACC=1 go test -run "TestAccPagerDutySlackConnection" ./pagerduty -v -timeout 120m

OK, found something. Breaking change has been introduced in b1221fd
I misunderstood how it worked. Will fix this ASAP

Make PagerDuty api.pagerduty.com and app.pagerduty.com configurable

…ly defined constant

…previously defined constant" This reverts commit b1221fd.

…on for slack resources

jbfavre · 2021-10-22T18:44:37Z

Finaly found a way to do it.

Problem was: provider creates a pagerduty.Client at startup. Multi-region is easy to support.
Slack resources create a pagerduty.Client when needed. At this point, we can't access anymore the initial Config structure in config.go. Therefore, it's impossible to reuse the multi-region implementation.
I choosed to:

Change the way provider get initialized. It now return the Config structure which in turn can be reused later.
Update slack resources to call Config.SlackClient to get a relevant pagerduty.Client (cons: it duplicates most of the config.Client code)
Update all other resources which have now to create a pagerduty client when needed (as slack resources already did).
Update tests

Only remaining question I have, is: creating a new pagerduty client for each resource might not be that efficient in terms of resource consumption. Maybe we should try to initiate only 2 (default API & Slack one).
What do you think @stmcallister ?

I've checked that all acceptance tests run fine (except user's calls)

stmcallister · 2021-10-24T14:40:41Z

@jbfavre 🤔 I think this is the right way to go, but let me put some thought into it through the weekend. One nitpick that I know I'd want changed at this point is the naming for the token_user field. Will you change those to user_token through out the code? Thanks for your work on this feature!

jbfavre · 2021-10-25T06:31:33Z

@jbfavre thinking I think this is the right way to go, but let me put some thought into it through the weekend. One nitpick that I know I'd want changed at this point is the naming for the token_user field. Will you change those to user_token through out the code? Thanks for your work on this feature!

token_user has been renamed to user_token (and TokenUser to UserToken).

jbfavre force-pushed the make_provider_ready_for_eu_platform branch from f4c46f1 to 8aab478 Compare September 9, 2021 13:31

jbfavre changed the title ~~Add support for provider baseurl new configuration option~~ Make PagerDuty api.pagerduty.com and app.pagerduty.com configurable Sep 9, 2021

jbfavre changed the title ~~Make PagerDuty api.pagerduty.com and app.pagerduty.com configurable~~ Make PagerDuty URLs api.pagerduty.com and app.pagerduty.com configurable Sep 9, 2021

jbfavre force-pushed the make_provider_ready_for_eu_platform branch from b712657 to 9e926e0 Compare September 13, 2021 07:40

jbfavre changed the title ~~Make PagerDuty URLs api.pagerduty.com and app.pagerduty.com configurable~~ Add service_region support Sep 13, 2021

stmcallister suggested changes Sep 28, 2021

View reviewed changes

pagerduty/provider.go Outdated Show resolved Hide resolved

jbfavre force-pushed the make_provider_ready_for_eu_platform branch from 40dc901 to f6fd5f3 Compare September 29, 2021 07:10

jbfavre force-pushed the make_provider_ready_for_eu_platform branch from f6fd5f3 to 09dcb2b Compare October 21, 2021 08:04

jbfavre added 9 commits October 22, 2021 20:32

Make PagerDuty api.pagerduty.com and app.pagerduty.com configurable

5f787ba

Make PagerDuty api.pagerduty.com and app.pagerduty.com configurable

rename variables to respect coding rules

6f0fd8d

Add basic tests and documentation update

aa8bba3

Switch to service_region configuration option

a988d71

Update provider options schema

4c2db89

Make new slack extension uses the computed AppUrl instead of previous…

cc2c6f1

…ly defined constant

Force ServiceRegion value to lowercase

7f648fb

Revert "Make new slack extension uses the computed AppUrl instead of …

a202dc1

…previously defined constant" This reverts commit b1221fd.

Allow to provide user level API Token via provider configuration

600b66d

jbfavre force-pushed the make_provider_ready_for_eu_platform branch from 09dcb2b to 1583820 Compare October 22, 2021 18:32

jbfavre added 2 commits October 22, 2021 20:40

Make slack connection resource support multi-region

7cc2514

Update all resource after provider logic change to support multi-regi…

0a89e0c

…on for slack resources

jbfavre force-pushed the make_provider_ready_for_eu_platform branch from 1583820 to 0a89e0c Compare October 22, 2021 18:41

jbfavre added 2 commits October 25, 2021 08:14

Update documentation with user_token details

3f68ec0

Rename token_user with user_token

8e9c219

stmcallister approved these changes Oct 25, 2021

View reviewed changes

stmcallister merged commit 8e9c219 into PagerDuty:master Oct 25, 2021

jbfavre deleted the make_provider_ready_for_eu_platform branch October 26, 2021 06:13

busatol mentioned this pull request Oct 27, 2021

Add optional base_url configuration #366

Merged

renovate bot mentioned this pull request Oct 24, 2023

Update Terraform github.com/PagerDuty/terraform-provider-pagerduty to v3 riccardo-noviello/renovate-test#8

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add service_region support #384

Add service_region support #384

jbfavre commented Sep 9, 2021 •

edited

Loading

stmcallister commented Sep 9, 2021

jbfavre commented Sep 10, 2021 •

edited

Loading

stmcallister left a comment

stmcallister commented Oct 21, 2021 •

edited

Loading

jbfavre commented Oct 21, 2021

jbfavre commented Oct 21, 2021

jbfavre commented Oct 21, 2021

jbfavre commented Oct 21, 2021

stmcallister commented Oct 21, 2021 •

edited

Loading

jbfavre commented Oct 22, 2021

jbfavre commented Oct 22, 2021

stmcallister commented Oct 24, 2021

jbfavre commented Oct 25, 2021 •

edited

Loading

Add service_region support #384

Add service_region support #384

Conversation

jbfavre commented Sep 9, 2021 • edited Loading

stmcallister commented Sep 9, 2021

jbfavre commented Sep 10, 2021 • edited Loading

stmcallister left a comment

Choose a reason for hiding this comment

stmcallister commented Oct 21, 2021 • edited Loading

jbfavre commented Oct 21, 2021

jbfavre commented Oct 21, 2021

jbfavre commented Oct 21, 2021

jbfavre commented Oct 21, 2021

stmcallister commented Oct 21, 2021 • edited Loading

jbfavre commented Oct 22, 2021

jbfavre commented Oct 22, 2021

stmcallister commented Oct 24, 2021

jbfavre commented Oct 25, 2021 • edited Loading

jbfavre commented Sep 9, 2021 •

edited

Loading

jbfavre commented Sep 10, 2021 •

edited

Loading

stmcallister commented Oct 21, 2021 •

edited

Loading

stmcallister commented Oct 21, 2021 •

edited

Loading

jbfavre commented Oct 25, 2021 •

edited

Loading