-
-
Notifications
You must be signed in to change notification settings - Fork 95
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
Remove enabled ternary on providers #82
Remove enabled ternary on providers #82
Conversation
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.
thanks @tommij
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.
thanks @tommij
/terratest |
@tommij can you please run the following commands and commit the changes:
|
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 rebuild README again
Just note that this reverts #56 pr. Honestly we need to remove the providers from this module so the root terraform dir that consumes this module can fully control the provider. See |
Not necessarily in disagreement - but I also don't think providers should be be ternary enabled/disabled at any point, as that would effectively mean that you cannot remove resources created with the module. Resources is another thing entirely. Fortunately, disabled was only the case for assume role parameters, and nothing else - on account of the implementation. It seems we may have been the only ones using assume_role with role chaining that have tried to set enabled = false with this module, but we did eventually get there :) |
@tommij thank you, please address this:
We'll approve the PR since I agree with you that providers should not be disabled in any case (providers are not resources) |
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 see comments
/terratest |
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.
thanks @tommij
what
removal of enabled part of the for_each ternary, as it disables assume_role on enabled = false, which blocks teardown.
why
references