Manage extension configuration as plain JSON #84

pdecat · 2018-05-09T15:16:06Z

This PR adds support to manage the configuration of extensions.

It is done as plain JSON as the configuration part differs from vendor to vendor: https://v2.developer.pagerduty.com/v2/page/api-reference#!/Extension_Schemas/get_extension_schemas

TODO:

add documentation
update vendored go-pagerduty dependency once Manage extension configuration as plain JSON heimweh/go-pagerduty#22 is merged

Note: upgrading terraform dependencies as done in https://github.com/pdecat/terraform-provider-pagerduty/tree/upgrade-terraform-deps would allow for more readable terraform configuration snippets using structured locals and advanced jsonencode():

locals {
  extension_config_slack = {
    restrict = "any"

    notify_types {
      resolve     = false
      acknowledge = false
      assignments = false
    }
  }
}

resource "pagerduty_extension" "foo"{
...
  config = "${jsonencode(local.extension_config_slack)}"
}

…nsion-config branch: govendor fetch github.com/heimweh/go-pagerduty/pagerduty::github.com/pdecat/go-pagerduty/pagerduty@=f-extension-config

pdecat · 2018-07-10T07:02:42Z

Hi @heimweh, what do you think about this change?

If it sounds good to you, I'll proceed with the remaining work.

pdecat · 2018-07-10T07:03:05Z

Depends on heimweh/go-pagerduty#22

haines · 2018-07-10T08:06:16Z

Was just looking for a way to do this! If this change is accepted I'd be keen to make a similar one for pagerduty_service_integration.

samm-git · 2018-08-01T17:53:06Z

@pdecat just tested this patch with slack extension - works really good! I been able to create already authorized extension using access_token field.
@heimweh - is there any chance to get this accepted to master?

pdecat · 2018-08-09T08:41:44Z

@samm-git thanks for this info! I did not expect this would work for access_token.

heimweh

Hi @pdecat, thank you so much for the PR and sorry for the delay. I added some comments, nothing super critical and overall it looks good but would you be able to update the documentation as well?

Many thanks!

Best regards,

Alexander

heimweh · 2018-08-14T14:07:02Z

pagerduty/resource_pagerduty_extension.go

+				Type:             schema.TypeString,
+				Optional:         true,
+				ValidateFunc:     validation.ValidateJsonString,
+				DiffSuppressFunc: jsonExtensionConfigDiffSuppress,


Would it be possible to use https://github.com/hashicorp/terraform/blob/master/helper/structure/suppress_json_diff.go#L9 here?

Good catch, done.

heimweh · 2018-08-14T14:38:50Z

pagerduty/resource_pagerduty_extension.go

@@ -58,6 +67,7 @@ func buildExtensionStruct(d *schema.ResourceData) *pagerduty.Extension {
 			ID:   d.Get("extension_schema").(string),
 		},
 		ExtensionObjects: expandServiceObjects(d.Get("extension_objects")),
+		Config:           expandExtensionConfig(d.Get("config")),


Since this is an optional field I believe you must check if the field is in the configuration using GetOk.

Example:

if v, ok := d.GetOk("config"); ok { Extension.Config = v }

Indeed, this will avoid misleading unmarshalling errors.
Done.

heimweh · 2018-08-14T14:40:09Z

pagerduty/resource_pagerduty_extension.go

@@ -174,3 +188,35 @@ func flattenExtensionObjects(serviceList []*pagerduty.ServiceReference) interfac
 	}
 	return services
 }
+
+func expandExtensionConfig(v interface{}) interface{} {


Would it be possible to just use the generic https://github.com/hashicorp/terraform/blob/master/helper/structure/expand_json.go#L5 instead?

The generic ExpandJsonFromString() function provided by terraform unmarshals JSON-encoded data into map[string]interface{} instead of interface{}

Same for flattenExtensionConfig() that marshals a map[string]interface{} value into JSON-encoded data.

I believe using those would require to change the type of the terraform configuration field config from TypeString to TypeMap and specify a schema for this map.
That last part is what I'm trying to avoid here as this would require to enumerate all possible configuration options.

Ah makes sense 👍 let's keep it as-is then.

is merged

pdecat · 2018-08-14T15:55:15Z

Hi @heimweh , thanks for your feedback.
I'll be happy to update the documentation once the expand/flatten part is settled as I believe changing it would require to change the configuration format too.

heimweh · 2018-08-14T16:06:05Z

Hi @pdecat, nice work! I agree, let's leave the flatten/expand funcs as they are. I'll do a final review when the documentation is in place. Thank you so much for this!

Best regards,

Alexander

pdecat · 2018-08-14T16:35:25Z

Great, documentation added.

heimweh · 2018-08-14T17:10:05Z

pagerduty/resource_pagerduty_extension.go

@@ -98,6 +111,10 @@ func resourcePagerDutyExtensionRead(d *schema.ResourceData, meta interface{}) er
 	}
 	d.Set("extension_schema", extension.ExtensionSchema)

+	if err := d.Set("config", flattenExtensionConfig(extension.Config)); err != nil {


The schema expects a string here but is actually a []byte so this won't work.
Think you must wrap a string() here or have the flatten func return a string.

Well, it does work as is as the acceptance tests pass.

Also, I'm using it with success on our own stack.

I'm sure it works correctly but the problem here is that we're failing to set config as returned by the API (it's never updated).

Note: I'm seeing error setting extension config when I'm running the acctests.

That makes sense to me now, thanks!

heimweh · 2018-08-14T17:11:32Z

website/docs/r/extension.html.markdown

+			"acknowledge": false,
+			"assignments": false
+	},
+  "access_token": "XXX"


No biggie but the indentation here looks a bit off :)

heimweh · 2018-08-14T17:28:42Z

pagerduty/resource_pagerduty_extension_test.go

@@ -68,10 +68,12 @@ func TestAccPagerDutyExtension_Basic(t *testing.T) {
 						"pagerduty_extension.foo", "extension_schema", "PJFWPEP"),
 					resource.TestCheckResourceAttr(
 						"pagerduty_extension.foo", "endpoint_url", url),
+					resource.TestCheckResourceAttr(
+						"pagerduty_extension.foo", "config", "{\n\t\"restrict\": \"any\",\n\t\"notify_types\": {\n\t\t\t\"resolve\": false,\n\t\t\t\"acknowledge\": false,\n\t\t\t\"assignments\": false\n\t}\n}\n"),


I believe this should be:

"pagerduty_extension.foo", "config", "{\"notify_types\":{\"acknowledge\":false,\"assignments\":false,\"resolve\":false},\"restrict\":\"any\"}"),

Changing this makes the acceptance to fail:

# make testacc TEST=./pagerduty/ TESTARGS='-run=TestAccPagerDutyExtension_Basic' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./pagerduty/ -v -run=TestAccPagerDutyExtension_Basic -timeout 120m === RUN TestAccPagerDutyExtension_Basic --- FAIL: TestAccPagerDutyExtension_Basic (10.67s) testing.go:459: Step 0 error: Check failed: Check 5/5 error: pagerduty_extension.foo: Attribute 'config' expected "{\"restrict\": \"any\",\"notify_types\": {\"resolve\": false,\"acknowledge\": false,\"assignments\": false}}", got "{\n\t\"restrict\": \"any\",\n\t\"notify_types\": {\n\t\t\t\"resolve\": false,\n\t\t\t\"acknowledge\": false,\n\t\t\t\"assignments\": false\n\t}\n}\n" FAIL FAIL github.com/terraform-providers/terraform-provider-pagerduty/pagerduty 10.674s GNUmakefile:17: recipe for target 'testacc' failed make: *** [testacc] Error 1

I think that's due to the d.Set issue above,
because we're not actually reading back the config from the API.

heimweh · 2018-08-14T17:29:18Z

pagerduty/resource_pagerduty_extension_test.go

@@ -80,6 +82,8 @@ func TestAccPagerDutyExtension_Basic(t *testing.T) {
 						"pagerduty_extension.foo", "extension_schema", "PJFWPEP"),
 					resource.TestCheckResourceAttr(
 						"pagerduty_extension.foo", "endpoint_url", url_updated),
+					resource.TestCheckResourceAttr(
+						"pagerduty_extension.foo", "config", "{\n\t\"restrict\": \"pd-users\",\n\t\"notify_types\": {\n\t\t\t\"resolve\": true,\n\t\t\t\"acknowledge\": true,\n\t\t\t\"assignments\": true\n\t}\n}\n"),


I believe this should be:

"pagerduty_extension.foo", "config", "{\"notify_types\":{\"acknowledge\":true,\"assignments\":true,\"resolve\":true},\"restrict\":\"pd-users\"}"),

Same as above.

pdecat · 2018-08-16T09:09:09Z

Acceptance tests results

# make testacc TEST=./pagerduty/ TESTARGS='-run=TestAccPagerDutyExtension_Basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./pagerduty/ -v -run=TestAccPagerDutyExtension_Basic -timeout 120m
=== RUN   TestAccPagerDutyExtension_Basic
--- PASS: TestAccPagerDutyExtension_Basic (18.65s)
PASS
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   18.655s

heimweh · 2018-08-16T09:38:14Z

I got it to work using the following patch:

diff --git a/pagerduty/resource_pagerduty_extension.go b/pagerduty/resource_pagerduty_extension.go
index 7a7da32..13c9894 100644
--- a/pagerduty/resource_pagerduty_extension.go
+++ b/pagerduty/resource_pagerduty_extension.go
@@ -207,5 +207,5 @@ func flattenExtensionConfig(config interface{}) interface{} {
                log.Printf("[ERROR] Could not marshal extension config %s: %v", config.(string), err)
                return nil
        }
-       return json
+       return string(json)
 }
diff --git a/pagerduty/resource_pagerduty_extension_test.go b/pagerduty/resource_pagerduty_extension_test.go
index 7650404..876582b 100644
--- a/pagerduty/resource_pagerduty_extension_test.go
+++ b/pagerduty/resource_pagerduty_extension_test.go
@@ -69,7 +69,7 @@ func TestAccPagerDutyExtension_Basic(t *testing.T) {
                                        resource.TestCheckResourceAttr(
                                                "pagerduty_extension.foo", "endpoint_url", url),
                                        resource.TestCheckResourceAttr(
-                                               "pagerduty_extension.foo", "config", "{\n\t\"restrict\": \"any\",\n\t\"notify_types\": {\n\t\t\t\"resolve\": false,\n\t\t\t\"acknowledge\": false,\n\t\t\t\"assignments\": false\n\t}\n}\n"),
+                                               "pagerduty_extension.foo", "config", "{\"notify_types\":{\"acknowledge\":false,\"assignments\":false,\"resolve\":false},\"restrict\":\"any\"}"),
                                ),
                        },
                        {
@@ -83,7 +83,7 @@ func TestAccPagerDutyExtension_Basic(t *testing.T) {
                                        resource.TestCheckResourceAttr(
                                                "pagerduty_extension.foo", "endpoint_url", url_updated),
                                        resource.TestCheckResourceAttr(
-                                               "pagerduty_extension.foo", "config", "{\n\t\"restrict\": \"pd-users\",\n\t\"notify_types\": {\n\t\t\t\"resolve\": true,\n\t\t\t\"acknowledge\": true,\n\t\t\t\"assignments\": true\n\t}\n}\n"),
+                                               "pagerduty_extension.foo", "config", "{\"notify_types\":{\"acknowledge\":true,\"assignments\":true,\"resolve\":true},\"restrict\":\"pd-users\"}"),
                                ),
                        },
                },

TF_ACC=1 go test ./pagerduty -v -run=TestAccPagerDutyExtension -timeout 120m
=== RUN   TestAccPagerDutyExtension_import
--- PASS: TestAccPagerDutyExtension_import (16.27s)
=== RUN   TestAccPagerDutyExtension_Basic
--- PASS: TestAccPagerDutyExtension_Basic (19.89s)
PASS
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   36.175s

pdecat · 2018-08-16T09:44:18Z

Ok, got it, thanks!

heimweh

Thank you so much for this contribution @pdecat! this will be a great addition to the provider 🙏

This LGTM 👍

anirban1c · 2020-08-25T10:39:51Z

Is this only using slack ? looking to automation adding extentions for MS Teams, is there an example ?

pdecat · 2020-08-26T07:16:09Z

Hi @anirban1c, the config field should work with all extension schemas that support it.

You can get the extension schema for Microsoft Teams (id P6VL8WI) by invoking the Get an extension vendor API: https://developer.pagerduty.com/api-reference/reference/REST/openapiv3.json/paths/~1extension_schemas~1%7Bid%7D/get

Currently, the API documentation states it supports only one setting:

    "config": {
      "secret": {
        "encrypted": true
      }
    },

You may get additional fields from actual API responses once you create pagerduty_extension resources with terraform.
I'd then suggest to add them to your terraform configuration.

anirban1c · 2020-08-26T09:33:23Z

thanks @pdecat, just to clarify this will work with the new integration ? and not the legacy MS Teams V1 integration ?

pdecat · 2020-08-26T09:42:58Z

I only see one MS Teams extension in the API and the UI.

anirban1c · 2020-08-26T09:57:47Z

this is whats confusing me :
on Teams:

The PagerDuty (Legacy) connector sends notifications about triggered incidents. To use this connector, you'll need to create certain settings on the PagerDuty website by following a few easy steps. If you don't already have an account, create one on the PagerDuty (Legacy) website.

anirban1c · 2020-08-26T09:59:11Z

MS teams integration works just fine from the UI and notifications flowing btw

anirban1c · 2020-08-26T10:09:05Z

this is what I get, let me try this in TF and wondering if anyone has got this working ?

In [10]: json.loads(r.content)
Out[10]:
{'extension_schema': {'id': 'P6VL8WI',
'type': 'extension_schema',
'summary': 'Microsoft Teams',
'self': 'https://api.pagerduty.com/extension_schemas/P6VL8WI',
'html_url': None,
'key': 'msteams',
'label': 'Microsoft Teams',
'description': '',
'url': 'https://app.pagerduty.com/ms-teams/pd_webhooks',
'guide_url': '',
'icon_url': 'https://pdpartner.s3.amazonaws.com/extensions_portal/ms-teams-100.png',
'logo_url': 'https://pdpartner.s3.amazonaws.com/extensions_portal/ms-teams-100.png',
'features': [],
'authorizable': False,
'config': {'secret': {'encrypted': True}},
'send_types': ['trigger',
'acknowledge',
'resolve',
'reopen',
'delegate',
'escalate',
'unacknowledge',
'assign',
'annotate'],
'addon': {'id': 'PQWF3WV',
'type': 'addon_reference',
'summary': 'Microsoft Teams Integration',
'self': 'https://api.pagerduty.com/base_addons/PQWF3WV',
'html_url': None},
'buttons': []}}

In [11]:

pdecat added 3 commits June 1, 2018 00:19

Update go-pagerduty from github.com/pdecat/go-pagerduty fork's f-exte…

2ff3e7f

…nsion-config branch: govendor fetch github.com/heimweh/go-pagerduty/pagerduty::github.com/pdecat/go-pagerduty/pagerduty@=f-extension-config

Manage extension configuration as plain JSON

dff40b8

Add github.com/hashicorp/terraform/helper/validation dependencies

6b6afa4

samm-git mentioned this pull request Aug 1, 2018

unable to use terraform-provider-pagerduty to create a Slack extension #73

Closed

heimweh reviewed Aug 14, 2018

View reviewed changes

pdecat added 3 commits August 14, 2018 16:52

Update vendored go-pagerduty dependency now that heimweh/go-pagerduty#22

c286885

is merged

Replace custom json diff with terraform's structure.SuppressJsonDiff()

09b75ab

Check extension config is set before trying to unmarshall it

35f0a1d

Document the service extension config field

43422b5

heimweh requested changes Aug 14, 2018

View reviewed changes

heimweh added the enhancement label Aug 15, 2018

Fix indentation

fddc0dd

Fix storage of the JSON data returned by the API

cd13b6a

heimweh approved these changes Aug 16, 2018

View reviewed changes

heimweh merged commit 4e2304a into PagerDuty:master Aug 16, 2018

pdecat deleted the f-extension-config branch August 17, 2018 05:58

Manage extension configuration as plain JSON #84

Manage extension configuration as plain JSON #84

Conversation

pdecat commented May 9, 2018 • edited Loading

pdecat commented Jul 10, 2018

pdecat commented Jul 10, 2018

haines commented Jul 10, 2018

samm-git commented Aug 1, 2018 • edited Loading

pdecat commented Aug 9, 2018

heimweh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdecat commented Aug 14, 2018

heimweh commented Aug 14, 2018

pdecat commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdecat commented Aug 16, 2018

Acceptance tests results

heimweh commented Aug 16, 2018

pdecat commented Aug 16, 2018

heimweh left a comment

Choose a reason for hiding this comment

anirban1c commented Aug 25, 2020

pdecat commented Aug 26, 2020

anirban1c commented Aug 26, 2020

pdecat commented Aug 26, 2020

anirban1c commented Aug 26, 2020

anirban1c commented Aug 26, 2020

anirban1c commented Aug 26, 2020

pdecat commented May 9, 2018 •

edited

Loading

samm-git commented Aug 1, 2018 •

edited

Loading