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

Ordered output for Terraform resources with 'count' #77

Merged
merged 1 commit into from
Oct 20, 2017
Merged

Ordered output for Terraform resources with 'count' #77

merged 1 commit into from
Oct 20, 2017

Conversation

SamiHiltunen
Copy link
Contributor

Hey,

First of, thanks for the tool! It makes using Ansible with Terraform very convenient.

This PR sorts the output groups that correlate to Terraform resources created with count.

As an example, check this old generated inventory. The kafka list is not ordered a in the same manner as it is in Terraform.

...
"kafka":  ["kafka1", "kafka3", "kafka2"],
"kafka.0": "xxx.xxx.xxx.xxx",
"kafka.1": "yyy.yyy.yyy.yyy",
"kafka.2": "zzz.zzz.zzz.zzz",
...

After this PR, the output of kafka list will be in proper order:

...
"kafka":  ["kafka1", "kafka2", "kafka3"],
"kafka.0": "xxx.xxx.xxx.xxx",
"kafka.1": "yyy.yyy.yyy.yyy",
"kafka.2": "zzz.zzz.zzz.zzz",
...

As an aside, it will print to standard error when a key is overwritten during resource gathering. The tool has an issue, where resources of different types will overwrite others with the same name. Also, having tags with same value as the name of a resource will overwrite the old value. As it is not solvable in a backwards compatible manner, I decided to just print this out so the users will at least see the errors.

@adammck
Copy link
Owner

adammck commented Sep 19, 2017

Thanks, this looks like a very sensible change. I'll try to get it reviewed in the next couple of days. Could you rebase against master, to ensure that it works with #72?

* Outputs Terraform resources created with Count in the correct
  order.
@SamiHiltunen
Copy link
Contributor Author

Hey, I've rebased on master and the tests are passing.

@Dwaynekj
Copy link

Dwaynekj commented Oct 1, 2017

This would also be useful for me

@SamiHiltunen
Copy link
Contributor Author

SamiHiltunen commented Oct 20, 2017

@adammck Polite ping! Would it be possible to have this merged?

@adammck adammck merged commit 2222e64 into adammck:master Oct 20, 2017
@adammck
Copy link
Owner

adammck commented Oct 20, 2017

Thanks very much for this! Sorry about the delay on reviewing it.

@Dwaynekj
Copy link

Awesome! @adammck When do you think another official release will be packaged?

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