New Resource: aws_iot_topic_rule

[abingham]

This PR is fundamentally just the "IoT rules" subset of the large PR created by @jhedev a while back. My hope is to help push this work forward by providing it in digestible chunks.

This seems to work insofar as I'm able to manage "aws_iot_topic_rule" resources: I can create them and destroy them, and they seem to be what I expect on the AWS side. The test suite provided in the original PR also seems to pass (though I don't know how substantial it is).

NB: I'm a complete noob at go and the terraform code base. Let me know if there are problems that need to be fixed, but don't be surprised if I have questions :)

TODO:

• Flatten actions
• Add the import
• Update acceptance tests to check destination attributes
• Add import test

[abingham]

This is apropos to #143

[rob-smallshire]

I notice that this PR #1858 seems equivalent to #1723. If nothing else this demonstrates a certain level of demand for this feature.

[cwardcode]

@rob-smallshire The lack of expediency to support features by Terraform is one of the major reasons we ended up going with CloudFormation in the end.

[rob-smallshire]

Is there anything the community can do to expedite either this PR #1858 or PR #1723 ?

[Ninir]

Hi @abingham

Thanks for the work here. Can you expose in why this seems easier? it's just me trying to make sure I understand correctly what's going on here! :)

The number of additions is the same, so before looking at a given work, it would probably be better for us to get the full explanation.

Thanks, will review & merge IoT Rules this week! 🚀

[abingham]

Can you expose in why this seems easier?

I'm not sure I understand the question, but I think you're asking why a smaller pull request (i.e. this one) is better than a large one (i.e. the original one I copied from). The answer is that the original one seemed to be stuck because it was too big for this project to digest. It really was a monster, and nobody seemed to have the time/energy/patience to deal with it, including the original author. The perception - both mine and that of some others following this line of development - is that a smaller set of focused PRs will get the changes in faster.

[Ninir]

Hi @abingham

What I meant is that #1723 has 865 additions, this work has 858 ones and looking briefly, it seems to be the same kind of work: schema structure, documentation, etc.

It's just me trying to understand why this work is easier to review, as I may be overlooking something! 😄

[abingham]

@Ninir They two PRs are probably almost identical. They both attempt to extract the "iot topic" elements from a much larger PR that has foundered. If you're considering how to decide between the two, at first approximation you should probably just chose the first.

It might be useful to get both PRs in to local branches and do a git diff between them. Maybe that'll shed some light on the differences (or lack thereof) between them.

[abingham]

@Ninir Actually, I take that back. The diff between the two is pretty monumental, largely I think because my PR is based on a more recent master version. That may be a good reason to review this PR rather than the other, but of course you could take the route of rebasing each PR on the same master to find the real differences.

[abingham]

@Ninir Ok, I've done the "rebase and compare" step, and it looks like the changes are almost identical. The main difference is that #1723 seems to have leading "+"s on the lines in website/docs/r/iot_topic_rule.html.markdown; my guess is that these are diff markers that somehow snuck in. The remaining differences seem to be inconsequential whitespace issues.

So, if my analysis is right, you should review this PR because it's a) a bit fresher and b) doesn't include the leading plus signs. In the end, though, either should be fine.

[Ninir]

@abingham By convention, we will first have a look at #1723 and see how it goes. If however the author does not have time to update, explicitely says someone can take over it or there is a lack of answer, we will check & review this one.

This is just us trying to be fair with authors, respecting ordering & so. Hope it makes sense!

[Ninir]

Hi @abingham

Since we didn't have any reply in the other PR, would you mind checking my comments on it and apply them here, so that I can then make a final review and merge it?

Also, could you update the documentation so that it has a working example? (the smallest configuration the better).

Would be really appreciated, thanks for the work!

[abingham]

Certainly, I'll try to do that tomorrow.

…

On Mon, Dec 4, 2017 at 4:30 PM Gauthier Wallet ***@***.***> wrote: Hi @abingham <https://github.com/abingham> Since we didn't have any reply in the other PR, would you mind checking my comments on it and apply them here, so that I can then make a final review and merge it? Also, could you update the documentation so that it has a working example? (the smallest configuration the better). Would be really appreciated, thanks for the work! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1858 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE1eJKj14HH14zOBHuDoWZSvl12q_c6ks5s9BANgaJpZM4P1T67> .

[abingham]

@Ninir I got some time to start working on this today, but it'll take me some time to work through everything you listed in the review. I'll push changes up as I have them, and it might be useful if you can look them over as I do, both to make sure I'm doing what you've asked and also to make sure I'm doing things idiomatically (since I don't really know go).

Since your comments are on a separate PR, this might get a bit awkward. I could do something like add notes to the other PR when I've pushed up relevant changes. How does that sound to you?

[Ninir]

Hey @abingham

I'm currently fixing last issues before merging, hope you don't mind.
This should be ready this day.

Still need to flatten actions when reading & complete the import.

Cheers!

[Ninir]

I've just made the last changes. Thanks for the work @abingham !

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSIoTTopicRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIoTTopicRule_ -timeout 120m
=== RUN   TestAccAWSIoTTopicRule_importbasic
--- PASS: TestAccAWSIoTTopicRule_importbasic (22.87s)
=== RUN   TestAccAWSIoTTopicRule_basic
--- PASS: TestAccAWSIoTTopicRule_basic (18.64s)
=== RUN   TestAccAWSIoTTopicRule_cloudwatchalarm
--- PASS: TestAccAWSIoTTopicRule_cloudwatchalarm (18.80s)
=== RUN   TestAccAWSIoTTopicRule_cloudwatchmetric
--- PASS: TestAccAWSIoTTopicRule_cloudwatchmetric (17.98s)
=== RUN   TestAccAWSIoTTopicRule_elasticsearch
--- PASS: TestAccAWSIoTTopicRule_elasticsearch (22.77s)
=== RUN   TestAccAWSIoTTopicRule_firehose
--- PASS: TestAccAWSIoTTopicRule_firehose (19.17s)
=== RUN   TestAccAWSIoTTopicRule_kinesis
--- PASS: TestAccAWSIoTTopicRule_kinesis (19.95s)
=== RUN   TestAccAWSIoTTopicRule_lambda
--- PASS: TestAccAWSIoTTopicRule_lambda (18.43s)
=== RUN   TestAccAWSIoTTopicRule_republish
--- PASS: TestAccAWSIoTTopicRule_republish (18.44s)
=== RUN   TestAccAWSIoTTopicRule_s3
--- PASS: TestAccAWSIoTTopicRule_s3 (21.00s)
=== RUN   TestAccAWSIoTTopicRule_sns
--- PASS: TestAccAWSIoTTopicRule_sns (18.90s)
=== RUN   TestAccAWSIoTTopicRule_sqs
--- PASS: TestAccAWSIoTTopicRule_sqs (19.40s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	236.391s

[bflad]

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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!