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: trim space markdown renderer #2947

Merged
merged 9 commits into from
Jan 11, 2023

Conversation

krrrr38
Copy link
Contributor

@krrrr38 krrrr38 commented Jan 7, 2023

what

why

some messages contain needless break lines.

tests

  • tested on markdown_renderer_test.go and e2e

references

{{$result.Rendered}}
{{ range $i, $result := .Results -}}
### {{ add $i 1 }}. {{ if $result.ProjectName }}project: `{{ $result.ProjectName }}` {{ end }}dir: `{{ $result.RepoRelDir }}` workspace: `{{ $result.Workspace }}`
{{ $result.Rendered }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

change template by following ways

  • explicit break line: from {{ range $i, $result := .Results }}### {{add $i 1}} to {{ range $i, $result := .Results -}}\n### {{ add $i 1 }}
  • unified format: from {{$result.RepoRelDir}} to {{ $result.RepoRelDir }}

@@ -258,7 +263,7 @@ func (m *MarkdownRenderer) renderProjectResults(results []command.ProjectResult,
default:
return "no template matched–this is a bug"
}
return m.renderTemplate(tmpl, resultData{resultsTmplData, common})
return m.renderTemplateTrimSpace(tmpl, resultData{resultsTmplData, common})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added 2 kinds of TrimSpace.

  • terraform logs TrimSpace like resultData.Rendered = m.renderTemplateTrimSpace(templates.Lookup("importSuccessUnwrapped"), result.ImportSuccess)
  • full results TrimSpace (here)

@@ -4,10 +4,7 @@ Ran Plan for 2 projects:
1. dir: `infrastructure/production` workspace: `default`

### 1. dir: `infrastructure/staging` workspace: `default`
<details><summary>Show Output</summary>
Copy link
Contributor Author

@krrrr38 krrrr38 Jan 7, 2023

Choose a reason for hiding this comment

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

wrapped results need maxUnwrappedLines = 12. After TrimSpace, this results become unwrapped.

return strings.Count(output, "\n") > maxUnwrappedLines

@krrrr38 krrrr38 marked this pull request as ready for review January 7, 2023 09:47
@krrrr38 krrrr38 requested a review from a team as a code owner January 7, 2023 09:47
---
{{ end -}}
{{- template "log" . -}}
{{ end -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same with #2880 (comment)

@krrrr38 krrrr38 changed the title chore: trim space markdown renderer feat: trim space markdown renderer Jan 7, 2023
Copy link
Member

@nitrocode nitrocode left a comment

Choose a reason for hiding this comment

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

Looks good to me. Lots of changes here so it would help to get a second review

chenrui333
chenrui333 previously approved these changes Jan 8, 2023
Comment on lines -7 to -8
<details><summary>Show Output</summary>

Copy link
Member

Choose a reason for hiding this comment

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

actually, this should not be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be removed. Same with #2947 (comment).

Terraform logs are trimmed spaces, so output log contains less than 12lines. So become unwrapped.

Comment on lines -32 to -33
</details>
Plan: 1 to add, 0 to change, 0 to destroy.
Copy link
Member

Choose a reason for hiding this comment

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

same for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same with above.

@chenrui333
Copy link
Member

yeah, just one question about <details> markdown

@chenrui333 chenrui333 dismissed their stale review January 8, 2023 16:21

some question

@nitrocode nitrocode added this to the 0.22.3 milestone Jan 8, 2023
@nitrocode nitrocode changed the title feat: trim space markdown renderer fix: trim space markdown renderer Jan 9, 2023
@krrrr38 krrrr38 requested a review from chenrui333 January 9, 2023 23:07
Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks again @krrrr38

Copy link
Member

@nitrocode nitrocode left a comment

Choose a reason for hiding this comment

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

Yes! Thank you again @krrrr38 !

@nitrocode
Copy link
Member

@krrrr38 ah, small merge conflict.

@nitrocode nitrocode merged commit 6f28b6a into runatlantis:main Jan 11, 2023
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.

4 participants