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

Refactor outputs for 0.12, switch from maps to lists #9

Merged
merged 8 commits into from
Aug 14, 2019

Conversation

ludoo
Copy link
Contributor

@ludoo ludoo commented Aug 13, 2019

This is one of the PRs we need for Fabric, that switch outputs in modules supporting multiple resources from maps to list. The rationale is to keep the same order passed in input variables (eg names here) for outputs, which lists do but maps don't as they reorder keys lexically. Having a different order in output vars tripped up ourselves and a few customers in the past.

This also:

  • adds outputs for the base google_service_account resources, since it's now possible with Terraform 0.12;
  • fixes an issue with formatlist in 0.12 where the resulting list always has a nominal length of 1 and cannot e combined with other list of the same (effective) length, switching to using the new for construct is cleaner and works better.

It's a backwards-incompatible change, so it needs a major version bump. Tests and example have been updated accordingly.

@ludoo ludoo requested a review from morgante August 13, 2019 06:36
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

As discussed, let's switch to adding a new output to avoid breaking changes.

@ludoo ludoo requested a review from morgante August 14, 2019 11:56
@ludoo
Copy link
Contributor Author

ludoo commented Aug 14, 2019

Refactored following yesterday's chat discussion: the map-style outputs are preserved, and new list-style outputs have been added. All tests pass (and they too were refactored to test both styles).

outputs.tf Show resolved Hide resolved
outputs.tf Show resolved Hide resolved
@ludoo
Copy link
Contributor Author

ludoo commented Aug 14, 2019

Re ingwarr's suggestions to switch back to the legacy splat operator, this is the recommended best practice on the Terraform documentation:

We recommend using the full splat syntax for all new configurations since its behavior is generally more intuitive. The older syntax is retained for backwards compatibility but is not recommended.

@ingwarr
Copy link
Contributor

ingwarr commented Aug 14, 2019

@ludoo Ok, you are right, thank you

@ludoo
Copy link
Contributor Author

ludoo commented Aug 14, 2019

@ludoo Ok, you are right, thank you

I think you need to approve, since you requested changes ;)

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Thanks! Just two minor issues - want to confirm you're willing to make this a breaking change.

outputs.tf Show resolved Hide resolved
@morgante morgante merged commit d13604d into master Aug 14, 2019
ludoo added a commit that referenced this pull request Aug 14, 2019
morgante added a commit that referenced this pull request Aug 14, 2019
…samples

Update auto-generated docs for changes in #9
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.

3 participants