-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: trim space markdown renderer #2947
Conversation
{{$result.Rendered}} | ||
{{ range $i, $result := .Results -}} | ||
### {{ add $i 1 }}. {{ if $result.ProjectName }}project: `{{ $result.ProjectName }}` {{ end }}dir: `{{ $result.RepoRelDir }}` workspace: `{{ $result.Workspace }}` | ||
{{ $result.Rendered }} |
There was a problem hiding this comment.
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}) |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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.
atlantis/server/events/markdown_renderer.go
Line 282 in e4473ce
return strings.Count(output, "\n") > maxUnwrappedLines |
--- | ||
{{ end -}} | ||
{{- template "log" . -}} | ||
{{ end -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with #2880 (comment)
...s/events/testfixtures/test-repos/workspace-parallel-yaml/exp-output-apply-all-production.txt
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
<details><summary>Show Output</summary> | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
</details> | ||
Plan: 1 to add, 0 to change, 0 to destroy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with above.
yeah, just one question about |
There was a problem hiding this 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
There was a problem hiding this 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 !
@krrrr38 ah, small merge conflict. |
what
renderTemplateTrimSpace
.why
some messages contain needless break lines.
tests
references