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

feat: update to context #38

Closed
wants to merge 20 commits into from
Closed

Conversation

syphernl
Copy link

@syphernl syphernl commented Sep 7, 2020

what

  • Update to context.tf
  • renamed original enabled to distribution_enabled (breaking change)

why

  • Most/all other CloudPosse modules have an enabled flag to prevent resources from being created. This module used enabled to toggle the status of the distribution itself.

references

@nitrocode
Copy link
Member

Hm that's a good point but it would be better to cherry pick the enabled commits into a separate pr in case the other pending pr takes longer to complete.

@syphernl
Copy link
Author

syphernl commented Sep 8, 2020

Hm that's a good point but it would be better to cherry pick the enabled commits into a separate pr in case the other pending pr takes longer to complete.

@nitrocode I agree that a separate PR would be a better idea but the current master is based on Terraform prior to 0.12, which is lacking functions like try on which these changes depend.

@aknysh
Copy link
Member

aknysh commented Sep 21, 2020

@syphernl thanks for the PR, this is good.
We are actually updating all modules to a new convention: using context.tf which has all the commonly used variables like namespace, environment, stage, name, enabled`, etc.
Please see this PR as an example cloudposse/terraform-aws-efs#55
I'd update this PR for to the new convention since we are going to do it to all modules.
Thanks

@syphernl syphernl requested review from a team as code owners September 22, 2020 06:12
@syphernl syphernl changed the title feat: introduce enabled flag feat: update to context Sep 22, 2020
@syphernl
Copy link
Author

@aknysh I've updated the PR (and title/description since the scoped changed somewhat)

@osterman
Copy link
Member

/test all

@jamengual jamengual added the terraform/0.13 Module requires Terraform 0.13 or later label Oct 9, 2020
@jamengual
Copy link

/test all

@jamengual
Copy link

/rebuild-readme

@jamengual
Copy link

/test all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
terraform/0.13 Module requires Terraform 0.13 or later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabled input variable doesn't actually prevent resource creation
6 participants