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

Fix variable enabled=false results in errors #47

Merged
merged 3 commits into from
Apr 26, 2021

Conversation

paulrob-100
Copy link
Contributor

@paulrob-100 paulrob-100 commented Apr 20, 2021

what

  • Fix Setting variable enabled=false results in errors #46
  • Incorporates, supersedes, and closes Fix nat_gateway_enabled=false InvalidNatGatewayId.Malformed error  #45
  • Incorporates, supersedes, and closes Removed deprecated template provider #41
  • Add local availability_zones which is empty if disabled
    • private_count and public_count are either 0 if disabled or the length of the local.availability_zones list
    • therefore aws resource counts will not reference empty list in element function
    • also guaranteed to have same number of elements in zipmap function
  • Added convenience local lists of tuples for outputs
  • Note az_ngw_ids is now an empty map if disabled - previously a map of constant "0"
  • dummy_az_ngw_ids is no longer referenced so remove
  • Transform local lists of tuples to output maps
    • since private_count and public_count are not both >0, no ellipsis needed in transform, producing single map value
    • output maps are all empty if disabled

why

  • Users should be able to set the enabled flag to false
  • This is useful when used in conjunction with the terraform-yaml-stack-config module where subnets may be disabled but yaml configuration ready to flip the flag

references

* Add local availability_zones which is empty if disabled
  * private_count and public_count are either 0 if disabled or the length of the local.availability_zones list
  * therefore aws resource counts will not reference empty list in element function
  * also guaranteed to have same number of elements in zipmap function
* Added convenience local lists of tuples for outputs
* Note az_ngw_ids is now an empty map if disabled - previously a map of constant "0"
 * dummy_az_ngw_ids is no longer referenced so remove
* Transform local lists of tuples to output maps
  * since private_count and public_count are not both >0, no ellipsis needed in transform, producing single map value
  * output maps are all empty if disabled
@paulrob-100 paulrob-100 requested review from a team as code owners April 20, 2021 16:45
@paulrob-100 paulrob-100 requested review from adamcrews and nitrocode and removed request for a team April 20, 2021 16:45
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this, and thank you especially for adding tests.

We can accept this as is, but if you have the energy, I would appreciate it if you would please help bring this module up to our current standards.

UPDATE: I know this is asking a lot, really too much. I will take care of it for you.

Update the label modules and usage:

  • Replace module.this.enabled with local.enabled everywhere
  • Update labels to version 0.24.1
  • Replace old and unhelpful
  attributes = compact(concat(var.attributes, ["public"]))

with

  attributes = ["public"]

(and same for "private" label)

Also please run the following commands and check in the changes:

make init
make github/init
make pr/auto-format

For extra credit, merge #41 into this PR first.

main.tf Outdated
Comment on lines 2 to 4
public_enabled = module.this.enabled && var.type == "public"
private_enabled = module.this.enabled && var.type == "private"
availability_zones = (module.this.enabled) ? var.availability_zones : []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new style guide is

Suggested change
public_enabled = module.this.enabled && var.type == "public"
private_enabled = module.this.enabled && var.type == "private"
availability_zones = (module.this.enabled) ? var.availability_zones : []
enabled = module.this.enabled
public_enabled = local.enabled && var.type == "public"
private_enabled = local.enabled && var.type == "private"
availability_zones = local.enabled ? var.availability_zones : []

@Nuru
Copy link
Contributor

Nuru commented Apr 25, 2021

/test all

@paulrob-100 paulrob-100 requested a review from a team as a code owner April 26, 2021 07:22
@Nuru
Copy link
Contributor

Nuru commented Apr 26, 2021

/test all

@Nuru Nuru self-requested a review April 26, 2021 07:29
@Nuru Nuru merged commit 4e3780d into cloudposse:master Apr 26, 2021
@paulrob-100
Copy link
Contributor Author

Thank you for your review and merge @Nuru.
I also noticed the cloudposse vpc module using module.this.enabled instead of var.enabled so was going to update that.
I can help with any remaining review comments. Let me know if I can assist please.

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.

Setting variable enabled=false results in errors
3 participants