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

Adding support for markdown in checks description #17980

Closed
wants to merge 41 commits into from

Conversation

Caellion
Copy link
Contributor

@Caellion Caellion commented Dec 14, 2021

They say image is worth a thousand words, so let me say, provides this:

q1
q2

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2021

Codecov Report

Merging #17980 (fd082dc) into main (b4782e2) will decrease coverage by 0.02%.
The diff coverage is 2.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17980      +/-   ##
==========================================
- Coverage   45.31%   45.28%   -0.03%     
==========================================
  Files         819      820       +1     
  Lines       90863    90939      +76     
==========================================
+ Hits        41173    41186      +13     
- Misses      43134    43197      +63     
  Partials     6556     6556              
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
models/user/external_login_user.go 30.26% <0.00%> (+6.55%) ⬆️
routers/web/admin/auths.go 47.71% <0.00%> (-0.51%) ⬇️
routers/web/user/auth.go 11.57% <0.00%> (-0.48%) ⬇️
routers/web/user/auth_openid.go 0.00% <0.00%> (ø)
services/auth/source/oauth2/providers_custom.go 60.56% <0.00%> (-2.68%) ⬇️
services/auth/source/oauth2/providers_openid.go 44.44% <0.00%> (-8.89%) ⬇️
services/auth/source/oauth2/providers_simple.go 40.00% <0.00%> (-2.31%) ⬇️
services/auth/source/oauth2/source.go 25.00% <ø> (ø)
services/externalaccount/link.go 0.00% <0.00%> (ø)
... and 13 more

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 790e6cf...fd082dc. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 14, 2021
@lunny
Copy link
Member

lunny commented Dec 14, 2021

Looks like a good idea but even Github just stored text and most of them are short.

@Caellion
Copy link
Contributor Author

I agree it might be a specific use case but I use jenkins with checks plugin and it seems warnings-ng publishes analysis results as markdown tables.

@Caellion Caellion changed the title WIP: Adding support for markdown in checks description Adding support for markdown in checks description Dec 14, 2021
@lafriks lafriks added this to the 1.17.0 milestone Dec 14, 2021
web_src/less/_base.less Outdated Show resolved Hide resolved
Caellion and others added 8 commits December 15, 2021 01:28
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: silverwind <me@silverwind.io>
* https://github.com/go-gitea/gitea:
  [skip ci] Updated translations via Crowdin
  Use non-expiring key. (go-gitea#17984)

Co-Authored-By: silverwind <me@silverwind.io>
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: silverwind <me@silverwind.io>
* 'main' of https://github.com/Caellion/gitea:
  Update web_src/less/_base.less
  Update templates/repo/pulls/status.tmpl
  Update templates/repo/commit_statuses.tmpl
@lunny
Copy link
Member

lunny commented Dec 15, 2021

I agree it might be a specific use case but I use jenkins with checks plugin and it seems warnings-ng publishes analysis results as markdown tables.

Yeah, so the UI should consider both long markdown content and short plain content

modules/templates/helper.go Outdated Show resolved Hide resolved
modules/templates/helper.go Outdated Show resolved Hide resolved
templates/repo/commit_statuses.tmpl Outdated Show resolved Hide resolved
modules/templates/helper.go Outdated Show resolved Hide resolved
modules/templates/helper.go Outdated Show resolved Hide resolved
Caellion and others added 2 commits December 17, 2021 15:11
@zeripath
Copy link
Contributor

I think we need to add something to the rendercontext here - for example I think it's likely that SHAs will be in these reports and you'll want to link to them.

To that end we need to pass the repo and urlmetas

@Caellion
Copy link
Contributor Author

I think we need to add something to the rendercontext here - for example I think it's likely that SHAs will be in these reports and you'll want to link to them.

To that end we need to pass the repo and urlmetas

Actually from what I have seen any links that might happen to be in the reports lead to the CI that generated them (for example to the build) rather than to somewhere in repo.

@lunny lunny added the type/enhancement An improvement of existing functionality label Dec 21, 2021
}
return template.HTML(markup.Sanitize(renderedContent))
} else if markupType == "markup" {
if renderedContent, err = markup.RenderString(&markup.RenderContext{}, raw); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This has already included markdown rendering.

Copy link
Member

Choose a reason for hiding this comment

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

And it seems we just needs markdown here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was requested by sliverwind . #17980 (review)

Personally I also prefer to keep it simple and only introduce one render.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess we can actually simplify and only support markdown, at least until we can detect other types (like RST) based on entered content.

By the way, is there speaking anything against just using Str2html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIR it didn't render markdown tables

Copy link
Member

@silverwind silverwind Feb 16, 2022

Choose a reason for hiding this comment

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

Yeah seems like Str2html is mislabeled and actually renders HTML strings into template.HMTL, so markdown.RenderString seems fine.

* 'main' of https://github.com/go-gitea/gitea: (87 commits)
  Fix template bug of LFS lock (go-gitea#18784)
  Various Mermaid improvements (go-gitea#18776)
  [skip ci] Updated translations via Crowdin
  Fix display time of milestones (go-gitea#18753)
  [skip ci] Updated translations via Crowdin
  Prevent dangling GetAttribute calls (go-gitea#18754)
  Add example to render html files (go-gitea#18736)
  Fix a broken link in `commits_list_small.tmpl` (go-gitea#18763)
  Fix broken cancel button link on patch page (go-gitea#18718)
  Ignore the migrate if u2f_registration is not exist (go-gitea#18760)
  [skip ci] Updated translations via Crowdin
  Increase the size of the webauthn_credential credential_id field 
(go-gitea#18739)
  Fix isempty detection of git repository (go-gitea#18746)
  [skip ci] Updated translations via Crowdin
  Send mail to issue/pr assignee/reviewer also when OnMention is set 
(go-gitea#18707)
  Reduce CI go module downloads, add make targets (go-gitea#18708)
  Add number in queue status to monitor page (go-gitea#18712)
  Fix source code line highlighting (go-gitea#18729)
  Fix forked repositories missed tags (go-gitea#18719)
  [skip ci] Updated translations via Crowdin
  ...
@techknowlogick
Copy link
Member

I'm not sure if "status" is the appropriate place for this as it could seriously increase page load size, perhaps another field such as how github does it (see "checks" page on PRs)

(I'm for collecting this information, just in a different field to keep the status lightweight)

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 31, 2022

FYI: another merged PR has introduced another RenderMarkdownToHtml

"RenderMarkdownToHtml": func(input string) template.HTML {

@lunny
Copy link
Member

lunny commented Jun 4, 2022

Or we can have two columns, one is for status, another is for description which allow markdown renderering.

@techknowlogick techknowlogick modified the milestones: 1.17.0, 1.18.0 Jun 8, 2022
@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022
@lunny lunny removed this from the 1.19.0 milestone Feb 1, 2023
@wxiaoguang
Copy link
Contributor

So this PR gets stale and I guess most people prefer to keep the "status description" as simple as possible, so the "markdown rendering" is not accepted, right?

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Apr 24, 2023
@lunny
Copy link
Member

lunny commented Apr 24, 2023

So this PR gets stale and I guess most people prefer to keep the "status description" as simple as possible, so the "markdown rendering" is not accepted, right?

I myself couldn't find enough reason to support markdown there.

@silverwind
Copy link
Member

Does GitHub support markdown there?

@wxiaoguang
Copy link
Contributor

If I understand correctly, GitHub only shows simple text for the status. Maybe it's good to keep that field simple.

And this PR has been stale for a long time and I can't think of a way to handling it other than closing it. Feel free to reopen if there's any new progress and I could also help.

@wxiaoguang wxiaoguang closed this May 1, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants