search

hashicorp / terraform-config-inspect

A helper library for shallow inspection of Terraform configurations
Mozilla Public License 2.0
376 stars 76 forks source link

Fix Markdown rendering #39

Closed radeksimko closed 4 years ago

radeksimko commented 4 years ago

Fixes #38

I took the liberty to decouple the markdown rendering logic and added Markdown test data for all relevant test cases.

mildwonkey commented 4 years ago

thanks @radeksimko! This looks good. I have a couple of questions but they aren't blockers:

1: What was the markdown bug you fixed? The branch and commit message indicate a bug. 2: Did you mean to omit a test out.md for basics-json?

radeksimko commented 4 years ago
  1. The bug is described in #38 - basically most (all?) Markdown rendering logic was broken, probably a side effect of that logic being entirely untested. 🤷‍♂
  2. Kind of. That test seems to be testing the parser (whether it decodes JSON and HCL equally well), but not the actual rendering logic - be it JSON or Markdown. So I assume the existing test already covers that and additional Markdown test would bring little value (if it ever breaks I suppose we'd see it in the JSON test). I'm happy to add it though, if you think it's safer.
mildwonkey commented 4 years ago

🤔 Thanks for pointing out the change - can you add a test example with a provider source attribute, and (if you haven't already) make sure the registry folks know that output is changing? I believe they parse that field.

mildwonkey commented 4 years ago

also I am so sorry, you linked the issue right in this PR 🙄 I need to invite all my braincells to monday morning PR reviews next time

radeksimko commented 4 years ago

can you add a test example with a provider source attribute

I think this test covers that already? https://github.com/hashicorp/terraform-config-inspect/pull/39/files#diff-76fee4570d37301e18d103509279bacfR5

radeksimko commented 4 years ago

make sure the registry folks know that output is changing

I requested review from @hashicorp/tf-self-serve - I'm not sure if we have any better-matching team on GitHub covering the Registry.

FWIW the output is not changing for providers that do not have a source defined. It's rather the internal representation which broke markdown rendering.