New resource r/aws_codestarnotification_rule #10991

spirius · 2019-11-23T15:12:12Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSCodeStarNotification -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCodeStarNotificationRule_basic
=== PAUSE TestAccAWSCodeStarNotificationRule_basic
=== CONT  TestAccAWSCodeStarNotificationRule_basic
--- PASS: TestAccAWSCodeStarNotificationRule_basic (114.43s)

Notes

There are two types of resources involved in codestar notifications - rules and targets. Unfortunately targets do not have dedicated creation API, they are implicitly created while creating rules or when new subscription is added. So there is no explicit control over targets and therefore this implementation only introduces one new resource (aws_codestarnotification_rule), which maintains targets state as well. It tries to remove targets when rule is modified or removed, but targets that are attached to multiple rules will not be removed.

References

https://docs.aws.amazon.com/codestar-notifications/latest/APIReference/API_Operations.html

spirius · 2019-12-07T21:01:35Z

hi @bflad, I am not sure what is the current community contribution review process, but any chance this can be reviewed soon? or am i missing something for proper PR?

bflad

Hi @spirius 👋 Thank you for submitting this. It was off to a good start! Please see the below initial feedback and let us know if you have any questions or do not have time to implement the items.

bflad · 2020-01-03T13:40:53Z

aws/provider.go

@@ -420,6 +420,7 @@ func Provider() terraform.ResourceProvider {
 			"aws_codebuild_webhook":                                   resourceAwsCodeBuildWebhook(),
 			"aws_codepipeline":                                        resourceAwsCodePipeline(),
 			"aws_codepipeline_webhook":                                resourceAwsCodePipelineWebhook(),
+			"aws_codestarnotification_rule":                           resourceAwsCodeStarNotificationRule(),


Since the API Reference, Go SDK Reference, AWS CLI, and the new custom endpoint refer to the AWS service with a plural name (CodeStar Notifications), we should probably follow suit for consistency. The API also refers to these rules as "notification rules", so to prevent any future problems with other rules being introduced in this particular API, we should probably also include the longer resource naming. The recommendation here will be aws_codestarnotifications_notification_rule even if it introduces a small stutter.

Can you please update the following:

Function naming (e.g. find-replace CodeStarNotificationRule with CodeStarNotificationsNotificationRule)

File naming, e.g.

git mv aws/resource_aws_codestarnotification_rule.go aws/resource_aws_codestarnotifications_notification_rule.go git mv aws/resource_aws_codestarnotification_rule_test.go aws/resource_aws_codestarnotifications_notification_rule_test.go git mv website/docs/r/codestarnotification_rule.html.markdown website/docs/r/codestarnotifications_notification_rule.html.markdown

Resource naming and usage (e.g. find-replace aws_codestarnotification_rule with aws_codestarnotifications_notification_rule)

Suggested change

"aws_codestarnotification_rule": resourceAwsCodeStarNotificationRule(),

"aws_codestarnotifications_notification_rule": resourceAwsCodeStarNotificationsNotificationRule(),

Thanks so much.

bflad · 2020-01-03T13:43:58Z

aws/resource_aws_codestarnotification_rule.go

+				Set: func(t interface{}) int {
+					target := t.(map[string]interface{})
+					return schema.HashString(fmt.Sprintf("%s|%s", target["address"], target["type"]))
+				},


Our general recommendation is to omit the Set function unless it is explicitly needed. In this case, the Terraform Plugin SDK should automatically skip the Computed: true only attribute. 👍

bflad · 2020-01-03T13:47:51Z

aws/resource_aws_codestarnotification_rule.go

+	}
+}
+
+func expandCodeStarNotificationRuleTargets(d *schema.ResourceData) []*codestarnotifications.Target {


Nit: Generally it is our practice to limit the "surface area" of input parameters for expand/flatten functions since it can be confusing when reading the calling code to know what the function actually needs from schema.ResourceData.

Suggested change

func expandCodeStarNotificationRuleTargets(d *schema.ResourceData) []*codestarnotifications.Target {

func expandCodeStarNotificationRuleTargets(targetsData []interface{}) []*codestarnotifications.Target {

And the calling code:

expandCodeStarNotificationRuleTargets(d.Get("target").(*schema.Set).List())

bflad · 2020-01-03T13:56:04Z

aws/resource_aws_codestarnotification_rule.go

+			TargetAddress: getStringPtr(target, "address"),
+			TargetType:    getStringPtr(target, "type"),


There is not much usage of getStringPtr() in this provider and we'd like to not introduce new usage (see also #5983). Since these attributes are Required and Optional+Default, its safe to access them via standard map access and type assertion:

TargetAddress: aws.String(target["address"].(string)), TargetType: aws.String(target["type"].(string)),

In the future, the Terraform Plugin SDK will likely introduce stronger typed attribute fetching (potentially with cty types), which is when we will likely want to re-introduce these types of abstractions.

bflad · 2020-01-03T14:01:07Z

aws/resource_aws_codestarnotification_rule.go

+	}
+
+	if v, ok := d.GetOk("tags"); ok {
+		params.Tags = keyvaluetags.New(v)


We should ignore AWS system tags and export as the service tagging type here (this is for future enhancements such as #7926) -- please see also the Contributing Guide section on adding resource tagging.

Suggested change

params.Tags = keyvaluetags.New(v)

params.Tags = keyvaluetags.New(v.(map[string]interface{})).IgnoreAws().CodestarnotificationsTags()

bflad · 2020-01-03T14:46:19Z

website/aws.erb

+                            <a href="#">Resources</a>
+                            <ul class="nav nav-auto-expand">
+                                <li>
+                                    <a href="/docs/providers/aws/r/codestarnotification_rule.html">aws_codestarnotification_rule</a>


Suggested change

<a href="/docs/providers/aws/r/codestarnotification_rule.html">aws_codestarnotification_rule</a>

<a href="/docs/providers/aws/r/codestarnotifications_notification_rule.html">aws_codestarnotifications_notification_rule</a>

bflad · 2020-01-03T14:47:16Z

website/docs/r/codestarnotification_rule.markdown

@@ -0,0 +1,87 @@
+---
+subcategory: "CodeStar Notifications"


Since this PR was introduced, we have since added subcategory checking to the testing. This new subcategory can be added to the website/allowed-subcategories.txt file to prevent the check from failing. 👍

bflad · 2020-01-03T15:10:14Z

website/docs/r/codestarnotification_rule.markdown

+* `resource` - (Required) The ARN of the resource to associate with the notification rule.
+* `status` - (Optional) The status of the notification rule. Possible balues are `ENABLED` and `DISABLED`, default is `ENABLED`.
+* `tags` - (Optional) A mapping of tags to assign to the resource.
+* `target` - (Optional) A `target` block. Can be specified multiple times.


It might be good to include a note here about the target requirement on creation:

Suggested change

* `target` - (Optional) A `target` block. Can be specified multiple times.

* `target` - (Optional) Configuration blocks containing notification target information. Can be specified multiple times. At least one target must be specified on creation.

bflad · 2020-01-03T15:10:26Z

website/docs/r/codestarnotification_rule.markdown

+An `target` block supports the following arguments:
+
+* `address` - (Required) The ARN of noitifaction rule target, typically SNS topic.
+* `type` - (Required) The type of the notification target. Default value is `SNS`.


Suggested change

* `type` - (Required) The type of the notification target. Default value is `SNS`.

* `type` - (Optional) The type of the notification target. Default value is `SNS`.

bflad · 2020-01-03T15:11:24Z

website/docs/r/codestarnotification_rule.markdown

+
+An `target` block supports the following arguments:
+
+* `address` - (Required) The ARN of noitifaction rule target, typically SNS topic.


Typo 😎

Suggested change

* `address` - (Required) The ARN of noitifaction rule target, typically SNS topic.

* `address` - (Required) The ARN of notification rule target. For example, a SNS Topic ARN.

LennyCastaneda · 2020-01-14T16:30:18Z

@bflad will this new resource “code star notification rule” also allow to be applied to CodeBuild to trigger an SNS topic? Just wanted to confirm, as I’m working on an AMI pipeline for AWS as we speak and need this notification rule in order to enable EBS encryption in case CodeBuild fails or stops.

And when is the expected ETA we can see this in the latest Terraform minor version update?

onoranzefunebricloud · 2020-01-23T22:09:32Z

+1

spirius · 2020-02-02T20:01:59Z

hi @bflad, finally found some free time to finalize the PR ☺️. ready for the review 🤞

Acceptance test results:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSCodeStarNotificationsNotificationRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSCodeStarNotificationsNotificationRule_ -timeout 120m
=== RUN   TestAccAWSCodeStarNotificationsNotificationRule_Basic
=== PAUSE TestAccAWSCodeStarNotificationsNotificationRule_Basic
=== RUN   TestAccAWSCodeStarNotificationsNotificationRule_Status
=== PAUSE TestAccAWSCodeStarNotificationsNotificationRule_Status
=== RUN   TestAccAWSCodeStarNotificationsNotificationRule_Targets
=== PAUSE TestAccAWSCodeStarNotificationsNotificationRule_Targets
=== RUN   TestAccAWSCodeStarNotificationsNotificationRule_Tags
=== PAUSE TestAccAWSCodeStarNotificationsNotificationRule_Tags
=== RUN   TestAccAWSCodeStarNotificationsNotificationRule_EventTypeIds
=== PAUSE TestAccAWSCodeStarNotificationsNotificationRule_EventTypeIds
=== CONT  TestAccAWSCodeStarNotificationsNotificationRule_Basic
=== CONT  TestAccAWSCodeStarNotificationsNotificationRule_Tags
=== CONT  TestAccAWSCodeStarNotificationsNotificationRule_Targets
=== CONT  TestAccAWSCodeStarNotificationsNotificationRule_Status
=== CONT  TestAccAWSCodeStarNotificationsNotificationRule_EventTypeIds
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_Basic (23.47s)
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_Targets (50.69s)
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_Status (50.80s)
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_Tags (51.72s)
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_EventTypeIds (58.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	61.444s

bflad

Great work, @spirius, let's get it out! 🚀

Output from acceptance testing:

--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_Tags (23.32s)
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_Targets (24.19s)
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_Basic (28.90s)
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_EventTypeIds (40.63s)
--- PASS: TestAccAWSCodeStarNotificationsNotificationRule_Status (41.29s)

bflad · 2020-02-11T20:08:28Z

aws/resource_aws_codestarnotifications_notification_rule.go

+
+	targets := removedTargets.List()
+
+	err := resource.Retry(time.Duration(5)*time.Second, func() *resource.RetryError {


This logic may be problematic in practice (#7873 / hashicorp/terraform-plugin-sdk#199), however it is more valuable to get this new resource out so folks can start trying it.

ghost · 2020-02-14T15:48:53Z

This has been released in version 2.49.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-03-27T17:14:46Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

spirius requested a review from a team November 23, 2019 15:12

bflad suggested changes Jan 3, 2020

View reviewed changes

bflad added waiting-response Maintainers are waiting on response from community or contributor. and removed needs-triage Waiting for first response or review from a maintainer. labels Jan 3, 2020

bflad self-assigned this Jan 3, 2020

bflad added the new-resource Introduces a new resource. label Jan 3, 2020

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 14, 2020

spirius force-pushed the feature/codestar-notification-rule branch from 1e65749 to b85dbec Compare February 2, 2020 19:34

spirius added 3 commits February 2, 2020 20:45

Add codestarnotifications service to vendor

f3c98f5

Add codestarnotification tags support in keyvaluetags

dcf7e42

Implement aws_codestarnotification_rule resource

65f85e4

spirius force-pushed the feature/codestar-notification-rule branch from b85dbec to 65f85e4 Compare February 2, 2020 19:50

spirius requested a review from bflad February 2, 2020 19:51

bflad added this to the v2.49.0 milestone Feb 11, 2020

bflad approved these changes Feb 11, 2020

View reviewed changes

bflad merged commit 63afda9 into hashicorp:master Feb 11, 2020

bflad added a commit that referenced this pull request Feb 11, 2020

Update CHANGELOG for #10991

6ea48bd

ghost locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource r/aws_codestarnotification_rule #10991

New resource r/aws_codestarnotification_rule #10991

spirius commented Nov 23, 2019 •

edited

Loading

spirius commented Dec 7, 2019

bflad left a comment

bflad Jan 3, 2020

bflad Jan 3, 2020

bflad Jan 3, 2020

bflad Jan 3, 2020

bflad Jan 3, 2020

bflad Jan 3, 2020

bflad Jan 3, 2020

bflad Jan 3, 2020

bflad Jan 3, 2020

bflad Jan 3, 2020

LennyCastaneda commented Jan 14, 2020 •

edited

Loading

onoranzefunebricloud commented Jan 23, 2020

spirius commented Feb 2, 2020

bflad left a comment

bflad Feb 11, 2020

ghost commented Feb 14, 2020

ghost commented Mar 27, 2020

	"aws_codestarnotification_rule": resourceAwsCodeStarNotificationRule(),
	"aws_codestarnotifications_notification_rule": resourceAwsCodeStarNotificationsNotificationRule(),

	func expandCodeStarNotificationRuleTargets(d schema.ResourceData) []codestarnotifications.Target {
	func expandCodeStarNotificationRuleTargets(targetsData []interface{}) []*codestarnotifications.Target {

		TargetAddress: getStringPtr(target, "address"),
		TargetType: getStringPtr(target, "type"),

	params.Tags = keyvaluetags.New(v)
	params.Tags = keyvaluetags.New(v.(map[string]interface{})).IgnoreAws().CodestarnotificationsTags()

	<a href="/docs/providers/aws/r/codestarnotification_rule.html">aws_codestarnotification_rule</a>
	<a href="/docs/providers/aws/r/codestarnotifications_notification_rule.html">aws_codestarnotifications_notification_rule</a>

	* `target` - (Optional) A `target` block. Can be specified multiple times.
	* `target` - (Optional) Configuration blocks containing notification target information. Can be specified multiple times. At least one target must be specified on creation.

	* `type` - (Required) The type of the notification target. Default value is `SNS`.
	* `type` - (Optional) The type of the notification target. Default value is `SNS`.


		An `target` block supports the following arguments:

		* `address` - (Required) The ARN of noitifaction rule target, typically SNS topic.

	* `address` - (Required) The ARN of noitifaction rule target, typically SNS topic.
	* `address` - (Required) The ARN of notification rule target. For example, a SNS Topic ARN.


		targets := removedTargets.List()

		err := resource.Retry(time.Duration(5)time.Second, func() resource.RetryError {

New resource r/aws_codestarnotification_rule #10991

New resource r/aws_codestarnotification_rule #10991

Conversation

spirius commented Nov 23, 2019 • edited Loading

Community Note

Notes

References

spirius commented Dec 7, 2019

bflad 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LennyCastaneda commented Jan 14, 2020 • edited Loading

onoranzefunebricloud commented Jan 23, 2020

spirius commented Feb 2, 2020

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 14, 2020

ghost commented Mar 27, 2020

spirius commented Nov 23, 2019 •

edited

Loading

LennyCastaneda commented Jan 14, 2020 •

edited

Loading