-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: support providing a cloudtrail from a different account #136
Conversation
Make it so! (this comment triggers tests) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved this change. But before we merge can you add an example of the scenario to examples/ folder and run the docs tool to update the Readme
https://terraform-docs.io/
terraform-docs markdown .
I added examples .. the diff on the module doc is quite large .. feels like its not been run in a little while .. so I added just the one line of diff adding the new variable :) You may want to include a |
Ok fine .. see last commit .. I added a running script, the terraform-docs config, and the fully updated readme :) |
source = "lacework/cloudtrail/aws" | ||
version = "~> 2.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source = "lacework/cloudtrail/aws" | |
version = "~> 2.8" | |
source = "../../" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We refer to the module locally instead of the registry ☝🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Make it so 🎉 |
scripts/terraform-docs.sh
Outdated
@@ -0,0 +1,14 @@ | |||
|
|||
if which terraform-docs >/dev/null; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, look at this feedback and apply the diff in your PR, then we will merge and release!
Note that I had to rebase from main
since we had some CI changes, that is probably why PR passes CI. 🍏
@piotrb Your work inspired us to adopt it in all our Terraform Modules! 🎉 Thank you. We really don't want to let this PR fall behind, any chance you can take some time to |
…nt than the cloudtrail itself. SNS option. This is a tiny tweak which allows passing in the arn of the cloudtrail in a different account, and it will allow that cloudtrail to publish into the sns topic.
Alright, I rebased again :) .. applied the one tweak to the example. Was there anything else? |
@piotrb thanks for the changes. We will be working on getting this merged. |
Merged #147 . Closing this out |
This is a tiny tweak which allows passing in the arn of the cloudtrail in a different account, and it will allow that cloudtrail to publish into the sns topic.
Summary
We have a particular use case:
We've been working with Clayton Sopel on your side to come up with a solution to make this work and it proved to be a very small change to the SNS topic policy, but given how this module is created, it didn't give us a nice way to slip this tweak in.
How did you test this change?
Switched the module reference in our environment to this fork and it worked.