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 Markdown rendering #39

Merged
merged 1 commit into from
Feb 10, 2020
Merged

Fix Markdown rendering #39

merged 1 commit into from
Feb 10, 2020

Conversation

radeksimko
Copy link
Member

Fixes #38


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

@radeksimko radeksimko added the bug Something isn't working label Feb 10, 2020
@radeksimko radeksimko requested a review from a team February 10, 2020 13:15
@mildwonkey
Copy link
Contributor

mildwonkey commented Feb 10, 2020

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
Copy link
Member Author

  1. The bug is described in Regression: Wrong type during markdown rendering #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.


Provider Requirements:
{{- range $name, $req := .RequiredProviders }}
* **{{ $name }}{{ if $req.Source }} ({{ $req.Source | tt }}){{ end }}:** {{ if $req.VersionConstraints }}{{ commas $req.VersionConstraints | tt }}{{ else }}(any version){{ end }}
Copy link
Member Author

Choose a reason for hiding this comment

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

@mildwonkey These two lines are the only difference from the previous template.

@mildwonkey
Copy link
Contributor

🤔 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
Copy link
Contributor

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
Copy link
Member Author

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 radeksimko requested a review from a team February 10, 2020 13:51
@radeksimko
Copy link
Member Author

radeksimko commented Feb 10, 2020

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.

Copy link

@findkim findkim left a comment

Choose a reason for hiding this comment

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

Thanks for the ping! This looks fine from the Registry perspective since it uses tfconfig as a library for module parsing and renders its own template based on the parsed components.

@radeksimko radeksimko merged commit dd27d0d into master Feb 10, 2020
@radeksimko radeksimko deleted the fix-markdown-rendering branch February 10, 2020 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Wrong type during markdown rendering
3 participants