-
Notifications
You must be signed in to change notification settings - Fork 117
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
chore: Tweak APPROVALNOTIFIER markdown template to render consistent HTML #1639
chore: Tweak APPROVALNOTIFIER markdown template to render consistent HTML #1639
Conversation
Hi @azuwis. Thanks for your PR. I'm waiting for a jenkins-x member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. |
/ok-to-test |
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.
Tests need to be updated accordingly. Run make test
9fb1591
to
8b4b164
Compare
@msvticket Tests are fixed. Also modified the markdown so that the generate HTML on Github will be exact the same before this change. |
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.
To me it doesn't look like the change is matching the images in the description of this PR
The problemGithub does not handle the Gitlab does the right thing, https://about.gitlab.com/blog/2014/02/21/markdown-newline-behaviour/, https://gitlab.com/gitlab-org/gitlab-foss/-/issues/57809, and is different from Github. The fixUse DetailsGenerate the markdownBeforeUse https://go.dev/play/p/35RYE-Nkkh8 to generate, copied from lighthouse/pkg/plugins/approve/approvers/approvers_test.go Lines 711 to 730 in efe21a2
AfterUse https://go.dev/play/p/YlXcBuaHj0H to generate, copied from lighthouse/pkg/plugins/approve/approvers/approvers_test.go Lines 711 to 730 in 8b4b164
Rendered on GithubBefore[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Bill No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing After[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Bill No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing Render on GitlabCopy the markdown, and preview on any Gitlab issue. BeforeAfterConclusionThe rendered HTML on Github is exact the same before and after this change. The rendered HTML on Gitlab is confusing before this change, and give similar result as Github after. |
Failed to merge this PR due to:
|
12 similar comments
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Need to rebase to current main branch? |
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
9 similar comments
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
yes |
Failed to merge this PR due to:
|
12 similar comments
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
Failed to merge this PR due to:
|
…HTML Currently the rendered HTML on gitlab is confusing: ``` This pull-request has been approved by: To complete the pull request process, please assign <user> You can assign the PR to them by writing /lh-assign @<user> in a comment when ready. ``` The above content are in one paragraph without new lines or `.`. Tweak according to [CommonMark Spec][1], since both [GitHub Flavored Markdown Spec][2] and [GitLab Flavored Markdown][3] are based on it. [1]: https://spec.commonmark.org/0.31.2/#hard-line-breaks [2]: https://github.github.com/gfm/#hard-line-breaks [3]: https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/user/markdown.md#newlines
8b4b164
to
83e4aa6
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: msvticket The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
Currently the rendered HTML on gitlab is confusing:
The above content are in one paragraph without new lines or
.
.Tweak according to CommonMark Spec, since both GitHub Flavored Markdown Spec and GitLab Flavored Markdown are based on it.
Gitlab before:
Gitlab after: