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

Make Markdown rendering 10 times faster #2232

Merged
merged 3 commits into from
Aug 2, 2019

Conversation

kraih
Copy link
Member

@kraih kraih commented Jul 31, 2019

This is the promised followup to #2228. It replaces Text::Markdown with CommonMark (which uses the standard cmark library) for comment rendering. Similar to how OBS already handles Markdown. It is at least 10 times faster for most cases, and has very solid security features.

While this should resolve our performance bottlenecks when rendering comments, there are a few changes to be aware of. Raw HTML is no longer allowed in comments (it is simply removed), since there is no way to sanitize HTML fast enough. Instead we now have custom template tags to generate text in different colors.

## This is an example comment
{{color:#ff0000|This text will be red with a white background}}
{{color:#00ff00|This text will be green with a white background}}
{{color:#ffffff|This text will be white with a black background}}
This text will have the standard color.

There is also new logic to change the background color to black if a light text color is chosen.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

(see next comment)

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Looks very nice and clean. As mentioned in before this would break any currently generated test reports by http://github.com/os-autoinst/openqa_review pasted into comment fields on an openQA instance which has this. You can prevent this by either patching this or maybe it is just enough to announce on an according mailing list about this change. Maybe it is not even relevant anymore for test and reporting workflows.

@codecov
Copy link

codecov bot commented Jul 31, 2019

Codecov Report

Merging #2232 into master will increase coverage by 0.06%.
The diff coverage is 97.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
+ Coverage   85.89%   85.96%   +0.06%     
==========================================
  Files         164      165       +1     
  Lines       10807    10823      +16     
==========================================
+ Hits         9283     9304      +21     
+ Misses       1524     1519       -5
Impacted Files Coverage Δ
lib/OpenQA/Utils.pm 92.5% <ø> (ø) ⬆️
lib/OpenQA/Schema/Result/JobGroups.pm 86.95% <100%> (-0.19%) ⬇️
lib/OpenQA/Schema/Result/Comments.pm 100% <100%> (ø) ⬆️
lib/OpenQA/Markdown.pm 100% <100%> (ø)
lib/OpenQA/Schema/Result/JobGroupParents.pm 76.66% <75%> (+3.33%) ⬆️
lib/OpenQA/Worker/Engines/isotovideo.pm 93.68% <0%> (-0.49%) ⬇️
lib/OpenQA/Worker/WebUIConnection.pm 80.1% <0%> (+2.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac734d5...044392c. Read the comment docs.

@Martchus
Copy link
Contributor

Patch for openQA review: os-autoinst/openqa_review#126

@AdamWill
Copy link
Contributor

AdamWill commented Aug 1, 2019

It seems like the second commit is kind of unnecessary now, as everything it does is replaced by the third commit - at least AFAICS. Couldn't it just be ditched?

@AdamWill
Copy link
Contributor

AdamWill commented Aug 1, 2019

Or, well, I guess, more the last two commits could be sort of squashed together and the bit where HTML::Restrict is used and then immediately ripped out again could be avoided.

@coolo
Copy link
Contributor

coolo commented Aug 2, 2019

squashing is if one commit fixes another - these commits are just revisions. So I don't see the requirement to squash

@kalikiana kalikiana merged commit e584843 into os-autoinst:master Aug 2, 2019
@kraih kraih deleted the safe_markdown branch May 12, 2020 13:19
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.

None yet

6 participants