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 non-string vars in --inventory output #72

Merged
merged 6 commits into from Sep 19, 2017

Conversation

ctrlok
Copy link
Contributor

@ctrlok ctrlok commented Sep 1, 2017

I receive a error when I try to use list in output variables:

[all:vars]
panic: interface conversion: interface {} is []interface {}, not string

goroutine 1 [running]:
main.cmdInventory(0x11d35c0, 0xc42000c018, 0x11d35c0, 0xc42000c020, 0xc42000a0e0, 0x0)
	/Users/me/go/src/github.com/adammck/terraform-inventory/cli.go:77 +0x65a
main.main()
	/Users/me/go/src/github.com/adammck/terraform-inventory/main.go:100 +0x4be

@adammck
Copy link
Owner

adammck commented Sep 4, 2017

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.

@ctrlok
Copy link
Contributor Author

ctrlok commented Sep 6, 2017

@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.

@adammck
Copy link
Owner

adammck commented Sep 8, 2017

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?

@ctrlok
Copy link
Contributor Author

ctrlok commented Sep 8, 2017

@adammck I do that, because 'expectedInventoryOutput' is unsorted, so I receive different output every time.

@adammck
Copy link
Owner

adammck commented Sep 12, 2017

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.

@ctrlok
Copy link
Contributor Author

ctrlok commented Sep 12, 2017

@adammck Sure. I didn't want to change anything before discuss :)
Also, I find a bug in my code, so I will fix it.

@ctrlok
Copy link
Contributor Author

ctrlok commented Sep 19, 2017

@adammck looks like it works :)

@adammck
Copy link
Owner

adammck commented Sep 19, 2017

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"])
Copy link
Owner

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
Copy link
Owner

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.

Copy link
Contributor Author

@ctrlok ctrlok Sep 19, 2017

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.

Copy link
Owner

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.

@ctrlok
Copy link
Contributor Author

ctrlok commented Sep 19, 2017

@adammck done. Also, I tested it in ansible for sure :) Thanks again!

@adammck adammck changed the title Fix --inventory outputs Fix non-string vars in --inventory output Sep 19, 2017
@adammck adammck merged commit c23c86a into adammck:master Sep 19, 2017
@ctrlok ctrlok deleted the add-json-outputs branch September 20, 2017 06:08
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.

None yet

2 participants