Add support for notification configuration resource #86

lafentres · 2019-08-08T20:01:16Z

Description

This adds support and documentation for creating, updating, destroying, and importing notification configurations. It resolves #68.

Notification configuration resources require:

name
destination_type - one of slack or generic
url - url the notifications will go to
workspace_external_id - external id of the workspace this notification configuration belongs to

Optionally, you can provide:

enabled - true or false, defaults to false
token - key used for providing an HMAC signature on notification requests, only allowed with destination_type of generic
triggers - set of triggers, defaults to empty set (no notification trigger on any run events)

Updating destination_type or workspace_external_id will force the creation of a new notification configuration resource.

Testing

The examples from the documentation I added or from the test cases work and you can use them to generate notification configurations if you pull this down and use it locally.

Documentation

lafentres · 2019-08-09T17:18:40Z

I might have been a little overzealous with the tests, so please let me know if there are tests that are unnecessary. Or if there are more tests that need to be added :)

davidcelis · 2019-08-09T17:29:52Z

tfe/resource_tfe_notification_configuration.go

+	d.Set("enabled", notificationConfiguration.Enabled)
+	d.Set("name", notificationConfiguration.Name)
+	// Don't set token here, as it is write only
+	// and setting it here would make it blank


Thank you for adding this comment! I noticed that resources like OrganizationToken and TeamToken also don't set the actual token value, but with no comment as to why.

I pieced this together after a lot of trial and error. It would be nice to get confirmation from someone who has done a lot of work with providers though.

@lafentres I think you're good here. 👍 If the value would be blank, then setting it here would for sure break the state and cause a diff.

I'm wondering why we even have a field for this value though? That's not your fault, just curious why it would even be in the SDK 🤔 Unless the notification configuration is shared with other parts of the CRUD, but it doesn't seem to be.

I'm wondering why we even have a field for this value though?

Good point. In this resource the token field should probably be omitted in the response type.

In other resources (e.g. team token, organization token) its used to return a newly created token in the create response, so it adds value there. But here its a value that is provided by the caller, so its never used in any of the responses.

tfe/resource_tfe_notification_configuration_test.go

davidcelis

This looks AWESOME ✨ great work with this PR!

I just have a couple small questions before I turn this into a ✅!

vancluever

@lafentres great work! I have some minor nits and a few things that should probably be changed (namely the de-coupling of the import test from the rest of the tests, and the update of the intention on the check functions). Otherwise this is a 👍 from me!

vancluever · 2019-08-12T21:54:50Z

tfe/resource_tfe_notification_configuration.go

+				Optional: true,
+				Elem: &schema.Schema{
+					Type: schema.TypeString,
+					ValidateFunc: validation.StringInSlice(


@lafentres did this work in your testing? Depending on how hashicorp/terraform#22298 goes, this might need to be removed. Just FYI. Hopefully the primitive-element type case gets allowed though.

Being able to do this (in a supported way) would be nice though.

@vancluever, yep, this worked :) In addition to passing the tests I wrote, it also stopped me from putting in various invalid strings when I was creating notifications with Terraform locally.

Weird. I'm asking @radeksimko to weigh in here as I'm not too sure how this is working given the restrictions in place. Maybe the acceptance testing suite is not using InternalValidate after all.

This works just fine because you are validating each element, not the whole set. In other words the ValidateFunc is associated with TypeString, not TypeSet.

~~@radeksimko what I'm wondering about is how it's passing InternalValidate with the current code in place: https://github.com/hashicorp/terraform/blob/v0.12.6/helper/schema/schema.go#L831-L836~~

Scratch that, I see that now. Good call and great application @lafentres!

vancluever · 2019-08-12T22:01:08Z

tfe/resource_tfe_notification_configuration.go

+	d.Set("enabled", notificationConfiguration.Enabled)
+	d.Set("name", notificationConfiguration.Name)
+	// Don't set token here, as it is write only
+	// and setting it here would make it blank


@lafentres I think you're good here. 👍 If the value would be blank, then setting it here would for sure break the state and cause a diff.

I'm wondering why we even have a field for this value though? That's not your fault, just curious why it would even be in the SDK 🤔 Unless the notification configuration is shared with other parts of the CRUD, but it doesn't seem to be.

tfe/resource_tfe_notification_configuration.go

tfe/resource_tfe_notification_configuration_test.go

website/docs/r/notification_configuration.html.markdown

svanharmelen

Added my 2cts 😉

svanharmelen · 2019-08-13T07:41:58Z

tfe/resource_tfe_notification_configuration.go

+				Optional: true,
+				Elem: &schema.Schema{
+					Type: schema.TypeString,
+					ValidateFunc: validation.StringInSlice(


Being able to do this (in a supported way) would be nice though.

svanharmelen · 2019-08-13T07:49:11Z

tfe/resource_tfe_notification_configuration.go

+	d.Set("enabled", notificationConfiguration.Enabled)
+	d.Set("name", notificationConfiguration.Name)
+	// Don't set token here, as it is write only
+	// and setting it here would make it blank


I'm wondering why we even have a field for this value though?

Good point. In this resource the token field should probably be omitted in the response type.

In other resources (e.g. team token, organization token) its used to return a newly created token in the create response, so it adds value there. But here its a value that is provided by the caller, so its never used in any of the responses.

tfe/resource_tfe_notification_configuration.go

tfe/resource_tfe_notification_configuration_test.go

website/docs/r/notification_configuration.html.markdown

tfe/resource_tfe_notification_configuration_test.go

vancluever

Just making sure that this is changed to approved based on all of the conversations. Great work again @lafentres!

davidcelis

Looks great to me too 🎉

radeksimko · 2019-08-16T17:35:28Z

tfe/resource_tfe_notification_configuration.go

+				Optional: true,
+				Elem: &schema.Schema{
+					Type: schema.TypeString,
+					ValidateFunc: validation.StringInSlice(


This works just fine because you are validating each element, not the whole set. In other words the ValidateFunc is associated with TypeString, not TypeSet.

lafentres added enhancement documentation new-resource labels Aug 8, 2019

lafentres self-assigned this Aug 8, 2019

ghost added the size/XL label Aug 8, 2019

lafentres force-pushed the lafentres/add-notification-configuration-support branch from 5d6f7b3 to a76a313 Compare August 9, 2019 16:55

lafentres marked this pull request as ready for review August 9, 2019 17:16

davidcelis reviewed Aug 9, 2019

View reviewed changes

tfe/resource_tfe_notification_configuration_test.go Outdated Show resolved Hide resolved

davidcelis reviewed Aug 9, 2019

View reviewed changes

tfe/resource_tfe_notification_configuration_test.go Outdated Show resolved Hide resolved

davidcelis reviewed Aug 9, 2019

View reviewed changes

lafentres changed the title ~~[DRAFT] Add support for notification configuration resource~~ Add support for notification configuration resource Aug 9, 2019

vancluever suggested changes Aug 13, 2019

View reviewed changes

svanharmelen reviewed Aug 13, 2019

View reviewed changes

lafentres force-pushed the lafentres/add-notification-configuration-support branch from a76a313 to 3cc2b0f Compare August 13, 2019 14:45

vancluever mentioned this pull request Aug 13, 2019

helper/schema: remove outdated ValidateFunc restrictions hashicorp/terraform#22298

Closed

Add support for notification configuration resource

1418928

lafentres force-pushed the lafentres/add-notification-configuration-support branch from 3cc2b0f to 1418928 Compare August 13, 2019 17:21

lafentres requested a review from vancluever August 13, 2019 17:32

vancluever approved these changes Aug 13, 2019

View reviewed changes

davidcelis approved these changes Aug 14, 2019

View reviewed changes

radeksimko approved these changes Aug 16, 2019

View reviewed changes

lafentres merged commit 7fc1d5f into master Aug 19, 2019

lafentres deleted the lafentres/add-notification-configuration-support branch August 19, 2019 19:32

hagurganus mentioned this pull request Aug 19, 2019

Please add ‘notification_configuration’(?) resource #68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for notification configuration resource #86

Add support for notification configuration resource #86

lafentres commented Aug 8, 2019 •

edited

Loading

lafentres commented Aug 9, 2019

davidcelis Aug 9, 2019

lafentres Aug 9, 2019

vancluever Aug 12, 2019

svanharmelen Aug 13, 2019

davidcelis left a comment

vancluever left a comment

vancluever Aug 12, 2019

svanharmelen Aug 13, 2019

lafentres Aug 13, 2019

vancluever Aug 13, 2019

radeksimko Aug 16, 2019

vancluever Aug 17, 2019 •

edited

Loading

vancluever Aug 12, 2019

svanharmelen left a comment

svanharmelen Aug 13, 2019

svanharmelen Aug 13, 2019

vancluever left a comment

davidcelis left a comment

radeksimko Aug 16, 2019

Add support for notification configuration resource #86

Add support for notification configuration resource #86

Conversation

lafentres commented Aug 8, 2019 • edited Loading

Description

Notification configuration resources require:

Optionally, you can provide:

Testing

Documentation

lafentres commented Aug 9, 2019

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

davidcelis left a comment

Choose a reason for hiding this comment

vancluever 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

vancluever Aug 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanharmelen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancluever left a comment

Choose a reason for hiding this comment

davidcelis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafentres commented Aug 8, 2019 •

edited

Loading

vancluever Aug 17, 2019 •

edited

Loading