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 unused hostvars array when grouping hosts #7

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

Termina1
Copy link
Contributor

@Termina1 Termina1 commented Sep 7, 2022

Currently if there is no group defined explicitly and they are defined when checking hosts, there always will be only one host in each group

@jtopjian
Copy link
Owner

jtopjian commented Sep 9, 2022

@Termina1 Thanks for submitting this.

If I recall correctly, appending to hostInventory was supposed to have the same effect as appending to groupInventory["hosts"], since changes to hostInventory should have updated groupInventory. But maybe that was never the case...

Would you be able to update the test files to confirm this behaviour and to help prevent regression in the future?

@Termina1
Copy link
Contributor Author

Termina1 commented Sep 9, 2022

Sometimes it can be counter-intuitive, but it's not the same effect. After appending to slice, you must assign result to a variable, otherwise it won't take effect.
You can read here: https://go.dev/blog/slices-intro

@jtopjian
Copy link
Owner

jtopjian commented Sep 9, 2022

@Termina1 Right, thank you! Though would you still be able to add one or more test cases to this PR to ensure it's accounted for?

@Termina1
Copy link
Contributor Author

Termina1 commented Sep 9, 2022

I would have to fix the tests first, because they don’t work on OSX I can give you tfstate which produces invalid unexpected inventory

@jtopjian
Copy link
Owner

I've been able to get the tests running on OS X and have written a test case for this. I'm going to merge this and follow-up with an additional PR for the tests.

Copy link
Owner

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

@jtopjian jtopjian merged commit 3be6dfe into jtopjian:master Sep 12, 2022
jtopjian added a commit that referenced this pull request Sep 12, 2022
@jtopjian jtopjian mentioned this pull request Sep 12, 2022
@jtopjian
Copy link
Owner

@Termina1 v0.3.1 has been released with this fix: https://github.com/jtopjian/ansible-terraform-inventory/releases/tag/v0.3.1

Please let me know if you have any issues with it.

@Termina1
Copy link
Contributor Author

Cool, thanks!

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.

2 participants