Skip to content
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

Changed event selector to a list, so that it can be supplied as a list with a map #13

Merged
merged 1 commit into from
Aug 28, 2018

Conversation

Jamie-BitFlight
Copy link
Contributor

@Jamie-BitFlight Jamie-BitFlight commented Aug 28, 2018

what

  • Change the event_selector var from a map to a list type.

why

It is currently a type map, that then gets put inside a list.
Even though it is a null map by default, because it is embedded into a list, it is evaluated by the cloudtrail resource.

And because it gets evaluated by the resource it triggers this bug:
hashicorp/terraform-provider-aws#5448

By switching it to a list, it means that by default instead of being a null map in a null list, it is just a null list. Which is skipped over, and doesn't trigger the bug.

TF Apply with the Cloudtrail module always comes up with :

~ module.cloudtrail.aws_cloudtrail.default
     event_selector.#:                           "0" => "1"
     event_selector.0.include_management_events: "" => "true"
     event_selector.0.read_write_type:           "" => "All"

@maartenvanderhoef
Copy link

Thanks @Jamie-BitFlight!

@aknysh
Copy link
Member

aknysh commented Aug 28, 2018

@Jamie-BitFlight thanks!
Did you test it?
(I remember having some issues with those data types, but don't remember exactly what it was).

@Jamie-BitFlight
Copy link
Contributor Author

Yeah, I tested it using the example. And @maartenvanderhoef tested it against his infra as well. He caught the bug, and I fixed it, he tested the branch.

@aknysh aknysh merged commit de9ca32 into master Aug 28, 2018
@aknysh aknysh deleted the event_selector branch August 28, 2018 17:08
@osterman
Copy link
Member

Need to rebuild readme since the variable type and description changed

@osterman osterman mentioned this pull request Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants