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

Adding custom_headers to delivery_method in webhook_subscription resource #455

Merged
merged 8 commits into from
Jun 9, 2022
Merged

Adding custom_headers to delivery_method in webhook_subscription resource #455

merged 8 commits into from
Jun 9, 2022

Conversation

devops-rob
Copy link
Contributor

This PR adds the ability to configure custom headers in the webhook_subscription resource.

@stmcallister
Copy link
Contributor

Thanks for the PR @devops-rob!! I think this will be a great feature for the provider. However, when I run the test for the Webhook Subscription resource I get the following error. Could you address that? Thanks!

TF_ACC=1 go test -run "TestAccPagerDutyWebhookSubscription" ./pagerduty -v -timeout 120m  
=== RUN   TestAccPagerDutyWebhookSubscription_import
panic: interface conversion: interface {} is map[string]interface {}, not map[string]string

goroutine 392 [running]:
github.com/terraform-providers/terraform-provider-pagerduty/pagerduty.expandDeliveryMethod(...)
	.../terraform-provider-pagerduty/pagerduty/resource_pagerduty_webhook_subscription.go:203
github.com/terraform-providers/terraform-provider-pagerduty/pagerduty.buildWebhookSubscriptionStruct(0xc0000ee070)
	 .../terraform-provider-pagerduty/pagerduty/resource_pagerduty_webhook_subscription.go:105 +0x616
github.com/terraform-providers/terraform-provider-pagerduty/pagerduty.resourcePagerDutyWebhookSubscriptionCreate(0xc0002ede00, {0x19b86a0, 0xc0000ee070})
	 .../terraform-provider-pagerduty/pagerduty/resource_pagerduty_webhook_subscription.go:115 +0x58
....

@smaeda-ks
Copy link
Contributor

@stmcallister 🤝

FYI, @devops-rob My PR to the underlying PD client library just got merged: heimweh/go-pagerduty#81
so I may propose you to just run go mod vendor, and try my earlier attempt here:
smaeda-ks@127f634

That should work, but there's one caveat that I mentioned in that PR above:
heimweh/go-pagerduty#81 (comment)

@devops-rob
Copy link
Contributor Author

Thanks @stmcallister and @smaeda-ks. The tests are now passing my end. Let me know what you see on your side if anything isn't working

@smaeda-ks
Copy link
Contributor

@devops-rob Can you also update the dependency to advance the commit hash?
https://github.com/PagerDuty/terraform-provider-pagerduty/blob/master/go.mod#L8

$ go get github.com/heimweh/go-pagerduty@41d374b4d21bb32712d9a2a5db2bf0f232d4cc53
$ go mod vendor

That will update go.mod (as well as go.sum) in this repo and sync /vendor directory, which is necessary when using the new client library version. This may include other unrelated client library changes but as long as it can be compiled and tests are passing it should be fine. Files under /vendor directory are usually managed by the go mod command and should be untouched manually.

devops-rob and others added 3 commits February 9, 2022 11:02
Co-authored-by: Shohei Maeda <11495867+smaeda-ks@users.noreply.github.com>
Co-authored-by: Shohei Maeda <11495867+smaeda-ks@users.noreply.github.com>
@smaeda-ks
Copy link
Contributor

@devops-rob Thanks for updating this PR! I've left one last comment (suggestion) but otherwise this looks good to me :)

Co-authored-by: Shohei Maeda <11495867+smaeda-ks@users.noreply.github.com>
@stmcallister
Copy link
Contributor

Thanks for the combined effort on this everyone! And, sorry for the delay on me getting back to this. Any chance one of you could resolve the conflicts? Thanks!

@stmcallister
Copy link
Contributor

@devops-rob would you be able to fix the conflicts in this PR?

@devops-rob
Copy link
Contributor Author

Hey @stmcallister Sorry for the delay. The notification got lost in my jungle of an inbox. I've resolved all the conflicts now so should be good to go. Thanks for all you help on this @smaeda-ks too

@stmcallister
Copy link
Contributor

Hey @devops-rob! Thanks for fixing the conflicts! While giving this one more look I noticed you're naming the field custom_headers in the schema. I also know that you (HashiCorp) recommend that we use plural names as items in a list as a best practice. However, in an effort to keep naming conventions consistent in the PagerDuty provider we ask that these fields be listed as singular. A good example of this is in the Schedule resource where we have the layer field to represent each layer in a schedule. Will you change the field to be the singular customer_header in the schema?

@devops-rob
Copy link
Contributor Author

Thanks @stmcallister I've updated the schema as per your comments. Acceptance tests for this resource are passing. Some tests are failing but not related to any resources I have worked on.

Copy link
Contributor

@stmcallister stmcallister left a comment

Choose a reason for hiding this comment

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

I added the custom_header field to the acceptance tests and found a few issues 😅 . I've addressed those, add my commit, and will merge this to master. Thanks for adding this! 🎉 👍

@stmcallister stmcallister merged commit bb3008f into PagerDuty:master Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants