Add iot TopicRuleDestination to v1beta1 #1015
Conversation
|
@mbbush, since you are now a contributor, you should be able to trigger uptest on open PRs. |
|
/test-examples="examples/iot/topicruledestination.yaml" |
|
Nifty! I thought it was only allowed for upbound org members. This is going to be useful, thanks for letting me know! |
mbbush
force-pushed
the
iot-topic-rule-destination
branch
from
304db0d to
f95d15a
Compare
|
/test-examples="examples/iot/topicruledestination.yaml" |
mbbush
requested review from
ulucinar,
sergenyalcin and
turkenf
as code owners
| @@ -2081,6 +2078,9 @@ var NoForkExternalNameConfigs = map[string]config.ExternalName{ | |||
| "aws_iot_thing_type": config.IdentifierFromProvider, | |||
| // IoT Topic Rules can be imported using the name | |||
| "aws_iot_topic_rule": config.NameAsIdentifier, | |||
| // IoT topic rule destinations can be imported using the arn | |||
| // arn:aws:iot:us-west-2:123456789012:ruledestination/vpc/2ce781c8-68a6-4c52-9c62-63fe489ecc60 | |||
| "aws_iot_topic_rule_destination": TemplatedStringAsProviderDefinedIdentifier("arn:aws:iot:{{ .setup.configuration.region }}:{{ .setup.client_metadata.account_id }}:ruledestination/vpc/{{ .external_name }}"), | |||
There was a problem hiding this comment.
What do you think about the relative merits of using just the provider-generated UUID suffix as the external name (as I've done here) vs the full arn (which is also the terraform id)? The arn is certainly longer, and more cumbersome, but also perhaps easier for new users to discover, since we don't currently have any way of documenting the expected format for an external name. The UUID is shorter, and is the minimum amount of information necessary to track, so there's no redundancy or room for conflict between e.g. the arn and the spec.
There was a problem hiding this comment.
I am okay with the above configuration and if we think this is better in terms of API normalization we can go with this.
|
/test-examples="examples/iot/topicruledestination.yaml" |
|
@turkenf Thanks for the testhook recommendation. It worked great. I think the only thing left before this can merge is a decision on whether to use the short identifier or the longer but more "discoverable" full ARN as the external name. I see pros and cons with both options, and am willing to go either way. |
Description of your changes
Fixes #361
I have:
make check-diff lint testto ensure this PR is ready for review.How has this code been tested
make e2e UPTEST_EXAMPLE_LIST=examples/iot/topicruledestination.yamlThe test takes about 15 minutes to run, due to AWS being slow to create and delete these resources. The equivalent acceptance test in the terraform provider takes basically the same amount of time.
Note that in the example, I used a management policy that excludes
Deletefor the IAM role. This is necessary to allow the IoT service to assume the role during deletion of the topic rule destination, to delete the ENIs it created. Otherwise deletion of the VPC fails because there are leftover resources in it. The result is that the IAM role won't be cleaned up. But that shouldn't be a problem. It's got a name that's unlikely to conflict with anything else, and rerunning the test a second time when the role already exist works just fine. That's one reason I used an inline policy, so I could only have a single leftover resource, rather than also leaving behind an IAM policy.