Add cli flag for accepting custom rendering template

[moatra]

The markdown output is convenient but opinionated. It would be nice to take a custom template to control the final output.

This PR adds the template cli flag to use a custom render template to control the output.

[hashicorp-cla]

All committers have signed the CLA.

[mildwonkey]

Hi @moatra, thanks for this PR!

I very much like the change, but I have a few concerns about the implementation. I'd like to see some tests - the default behavior is already well covered, but we should write a test with a good template, a bad template, and a non-existent template file. The error message when the file can't be read should be improved instead of presented directly to the user.

Since this code was originally designed with a template that we were confident of, it's using template.Must(), which panics if the template does not parse. There's no longer a guarantee that the supplied template will be valid. It should be refactored removing template.Must() so we can catch the error and present it to the user instead of panicking.

What I'm asking for is quite a bit more than your original PR. If you don't have the time (or interest) to do the additional work, that's completely fine and understandable - we can take a look and add to your PR.

[moatra]

Hey @mildwonkey!

I'm happy to do the legwork here. I've got most of the refactoring done, but having picked up golang a month ago I'm not quite up to speed on some of the best practices.

For the unit testing, do you have a preference between:

• Unit tests that invoke a subcommand and look at the actual output/status codes
• Refactoring the file loading/template parsing to helper functions and test those, and leave the actual fmt.Fprintf/os.Exit statements uncovered by tests

[mildwonkey]

Wonderful, thanks @moatra ! I'll answer any questions you've got.

I ended up writing more code then I intended while trying to think this through. I can push a starting main_test.go file to your repository, if you'd like, but I completely understand if you'd rather do it yourself for the learning experience!

---

For the test: since the default behavior is already well-covered, you can add a test just for showModuleMarkdown in a main_test.go file where you pass it a simple example module and a few different template strings, then test that the output looks like you expect it to (or properly returns an error if the template is invalid).

To simplify tests, you could refactor showModuleMarkdown to take an io.Writer as an argument and return an error: func showModuleMarkdown(module *tfconfig.Module, markdownTemplate string, w io.Writer) error { ... }

You'd pass it os.Stdout in main.go, but in the test you could pass a bytes.Buffer and The call in main.go would then look something like this:

err := showModuleMarkdown(module, markdownTemplate, os.Stdout)
if err != nil {
  fmt.Fprintf(os.Stderr, "error rendering template: %s\n", err)
  os.Exit(2)
}

Let me know if you have any questions.

[moatra]

@mildwonkey - I've pushed another commit with the changes and tests. Could you take another pass when you have a moment?

[tbugfinder]

@mildwonkey Could you take another pass to this PR?

[mildwonkey]

HI again @moatra! Thank you for working on this, and apologies for the long silence.

When I initially reviewed your PR, I was not considering our prior statement that this project is feature complete. I created more work for you when instead I should have known that we were not likely to accept this PR. I am sorry for this, it was not fair to you or your work. Please do not take the following as a reflection on your submission.

This tool is used by a number of systems inside HashiCorp and we must be very cautious when making changes. As states in our contributing guidelines, we cannot accept external PRs for this codebase that add support for additional Terraform language features.

While we consider terraform-config-inspect package feature complete, it would be great to see these extra concepts implemented in third-party codebases. For instance, here's an example of an existing tool for putting JSON data through HCL templates - you might find use in this, or in moving your code into its own wrapper program.

Based on all of that, I am going to close this PR. Thank you again!

[moatra]

Hey @mildwonkey! While I am saddened to hear that the PR will not be merged, I appreciate the resolution and wanted to thank you for the time you have also put into it.

Could I ask for one last favor? The README.md "Contributing" section focuses on the "no adding support for new TF language features". Since this PR did not touch the Terraform language, it would be helpful to future contributors if that section reflected the feature complete nature of the repo and said "No feature additions accepted" (with appropriate wordsmithing to properly convey your upbeat and friendly approach 😄).

Again, thanks for you time and input! Enjoy your Friday!

[mildwonkey]

Yes absolutely, you make an excellent suggestion and I shall do that immediately! Thank you so much for your kindness @moatra Cheers!