-
Notifications
You must be signed in to change notification settings - Fork 322
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 non-string vars in --inventory output #72
Conversation
Thanks for the patch! Would you mind adding an example to parser_test.go, so we can see how this is supposed to work? I think this tool may originally have been written before Terraform had output types other than string. |
@adammck I tried to add some tests, but I'm not sure, because I can't check inventory as string, so I add tests from ansible and check equality by counting chars. |
I'm sorry, I don't quite understand the need for counting characters. It seems like we should be able to compare expectedInventoryOutput with stdout? What's different about the output? |
@adammck I do that, because 'expectedInventoryOutput' is unsorted, so I receive different output every time. |
Hmm, good point, the output isn't deterministic because we're just iterating the map and dumping it out. How about we sort the keys and iterate that, instead? That'd be more sanely testable, and much better for people who use this tool to dump the inventory and check it into a repo somewhere, too. |
@adammck Sure. I didn't want to change anything before discuss :) |
@adammck looks like it works :) |
Excellent, thanks very much for doing the extra work there. This is a huge improvement. |
cli.go
Outdated
} | ||
sort.Strings(vars) | ||
for _, key := range vars { | ||
jsonItem, _ := json.Marshal(grp.Vars["key"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this supposed to be grp.Vars[key]
?
parser_test.go
Outdated
datacenter=null | ||
ids=null | ||
map=null | ||
olddatacenter=null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the above error manifests here. These should probably match the vars at :284, in expectedListOutput.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. I'm sorry. Will fix it. Sorry again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no problem, I almost missed it myself.
@adammck done. Also, I tested it in ansible for sure :) Thanks again! |
I receive a error when I try to use list in output variables: