Add external markup render support #2570

lunny · 2017-09-22T05:17:41Z

Will close #374.

Usage:
Add the below to your custom/conf/app.ini file. You can add one or more sections. The section name should markup.xxxxx, which you can name it as you like. Follow the below config items then restart your gitea, it's done. For example:

On MacOS,

brew install asciidoc

Then add below config to your custom/conf/app.ini file and restart. Migration https://github.com/maxandersen/gdoc2adoc.git and then view the .adoc file.

[markup.asciidoc]
ENABLED = true
; List of file extensions that should be rendered by an external command
FILE_EXTENSIONS = .adoc,.asciidoc
; External command to render all matching extensions
RENDER_COMMAND = "asciidoc --out-file=- -"
; Input is not a standard input but a file
IS_INPUT_FILE = false

codecov-io · 2017-09-22T05:42:28Z

Codecov Report

Merging #2570 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2570   +/-   ##
=======================================
  Coverage   26.85%   26.85%           
=======================================
  Files          89       89           
  Lines       17607    17607           
=======================================
  Hits         4728     4728           
  Misses      12193    12193           
  Partials      686      686

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 ddb7519...640eaba. Read the comment docs.

jonasfranz · 2017-09-22T08:12:36Z

conf/app.ini

@@ -551,3 +551,12 @@ SHOW_FOOTER_BRANDING = false
 SHOW_FOOTER_VERSION = true
 ; Show time of template execution in the footer
 SHOW_FOOTER_TEMPLATE_LOAD_TIME = true
+
+[markup.asciidoc]
+ENABLED = true


I think that this example markup renderer should be disabled by default or in this example because it requires third party tools.

@JonasFranzDEV this file is an example file. It will not effect anything if you upgrade.

The example should follow the default values, shouldn't it?

[markup.xxxx] are dynamic sections. Default values is no any [markup.xxxx] section just like this PR did. I put this section on the example ini file for others know how to add one or more tools. Also I will add these to config cheatsheet but I think put it in example ini file is easier to find for users.

jonasfranz · 2017-09-22T08:13:48Z

modules/markup/external/external.go

+// RegisterParsers registers all supported third part parsers according settings
+func RegisterParsers() {
+	for _, render := range setting.ExternalMarkupRenders {
+		if render.Enabled && render.Command != "" && len(render.FileExtensions) > 0 {


It might be good to validate the file extension to deny illegal file extensions.

For example?

.<iframe>

But if you can change the ini file. I will trust you.

We could only allow the regex \.\w ? This would prevent misconfiguration.

@sapk Have checked that on https://github.com/go-gitea/gitea/pull/2570/files#diff-60f5fca6d29eafbb30eb645a0cd4041fR1074

lafriks · 2017-09-22T13:29:58Z

Just a random thought... somehow I don't like that external tool is called (and I know there is no go alternatives)

lunny · 2017-09-22T14:04:23Z

@lafriks any name proposed if you don't like external?

Morlinest · 2017-09-22T14:08:19Z

@lunny I think he means calling exec.Command(command, args...) from gitea.

lunny · 2017-09-22T14:11:55Z

@Morlinest oh, I see.

ethantkoenig · 2017-09-25T04:30:59Z

modules/markup/external/external.go

+		}
+
+		_, err = io.Copy(f, rd)
+		f.Close()


File.Close() returns an error

I don't think this is needed. We have handle the err returned above line.

What if the copy is successful, but there's an error closing the file?

@ethantkoenig we will ignore that since it's a temp file. It will not effect the rendering.

Maybe good to check to prevent leaks?

We still don't check the error

ethantkoenig · 2017-09-25T04:34:03Z

modules/markup/external/external.go

+		fPath := filepath.Join(os.TempDir(), gouuid.NewV4().String())
+		f, err := os.Create(fPath)
+		if err != nil {
+			log.Error(4, "%s render run command %s failed: %v", p.Name(), p.Command, err)


Incorrect error message

ethantkoenig · 2017-09-25T04:36:27Z

modules/markup/external/external.go

+	var rd = bytes.NewReader(rawBytes)
+	commands := strings.Fields(p.Command)
+	var command = commands[0]
+	var args []string


Can just do args := commands[1:], since we're already assuming commands is not empty

Morlinest · 2017-09-25T10:50:34Z

modules/markup/external/external.go

+	return p.MarkupName
+}
+
+// Extensions implements markup.Parser


Rewrite comments please, it should tell what is the function doing.

Morlinest · 2017-09-25T10:51:26Z

modules/markup/external/external.go

+		}
+
+		_, err = io.Copy(f, rd)
+		f.Close()


Maybe good to check to prevent leaks?

lunny · 2017-10-11T04:00:31Z

@sapk @JonasFranzDEV @Morlinest all done.

lunny · 2017-10-25T16:23:57Z

@sapk @JonasFranzDEV @Morlinest rebased.

ethantkoenig · 2017-10-31T03:15:31Z

modules/markup/external/external.go

+
+		_, err = io.Copy(f, rd)
+		f.Close()
+		os.Remove(fPath)


Why do we delete the file immediately after writing to it?

ethantkoenig · 2017-10-31T03:15:46Z

modules/markup/external/external.go

+		}
+
+		_, err = io.Copy(f, rd)
+		f.Close()


We still don't check the error

lunny · 2017-10-31T05:52:01Z

@ethantkoenig done.

jonasfranz · 2017-11-01T20:06:08Z

modules/markup/external/external.go

+
+	if p.IsInputFile {
+		// write to templ file
+		fPath := filepath.Join(os.TempDir(), gouuid.NewV4().String())


You should check if the file already exists although it is very unlikely to happen. Or as an alternative you could add this check to your err != nil check at line 60.

lunny · 2017-11-02T06:04:51Z

@JonasFranzDEV done.

Morlinest · 2017-11-04T10:22:52Z

@lunny IMO it would be better to use parser/parsers instead of render/renders (render is verb, don't know if it can be used as noun and if renderer is correct word):
MarkupRender -> MarkupParser, ExternalMarkupRenders -> ExternalMarkupParsers

I don't rly like the way how it is initialized (hard dependency on setting package), I want to look if there is no better way.

lunny · 2017-11-05T02:38:59Z

@Morlinest did you mean put this note on the setting file?

BTW what about this: // NOTE: do not print any log except error.?

Morlinest · 2017-11-05T10:40:38Z

@lunny No, it is already there. As comment on function where we set setting variables.

// NewContext initializes configuration context.
// NOTE: do not print any log except error.
func NewContext() {

ethantkoenig · 2017-11-07T06:00:22Z

modules/setting/setting.go

@@ -60,6 +60,15 @@ const (
 	LandingPageExplore LandingPage = "/explore"
 )

+// MarkupParser defines the external parser configed on ini


nit: configured in

ethantkoenig · 2017-11-07T06:01:01Z

modules/markup/external/external.go

+	)
+
+	if p.IsInputFile {
+		// write to templ file


ethantkoenig · 2017-11-07T06:02:08Z

LGTM just two typos

lunny · 2017-11-07T06:24:25Z

@ethantkoenig done.

kellpossible · 2017-11-09T22:21:14Z

just wondering, is the expected output of the render command html?

lunny · 2017-11-10T00:17:24Z

@kellpossible Yes.

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 22, 2017

lunny added this to the 1.3.0 milestone Sep 22, 2017

lunny force-pushed the lunny/external_markup branch from d1e36e9 to 97c28e9 Compare September 22, 2017 05:30

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 22, 2017

jonasfranz suggested changes Sep 22, 2017

View reviewed changes

lunny mentioned this pull request Sep 24, 2017

Add reStructedText support #2564

Closed

ethantkoenig suggested changes Sep 25, 2017

View reviewed changes

lunny force-pushed the lunny/external_markup branch from 97c28e9 to 0900445 Compare September 25, 2017 05:00

ethantkoenig approved these changes Sep 25, 2017

View reviewed changes

Morlinest suggested changes Sep 25, 2017

View reviewed changes

lunny force-pushed the lunny/external_markup branch from 0900445 to f0f93e0 Compare September 25, 2017 12:13

lunny force-pushed the lunny/external_markup branch from f0f93e0 to 662bff9 Compare October 11, 2017 03:50

lunny mentioned this pull request Oct 20, 2017

External markup renderer [$15] #374

Closed

lunny force-pushed the lunny/external_markup branch from 662bff9 to 6a274c7 Compare October 25, 2017 02:48

ethantkoenig suggested changes Oct 31, 2017

View reviewed changes

lunny force-pushed the lunny/external_markup branch from 6a274c7 to 72817ce Compare October 31, 2017 05:50

ethantkoenig approved these changes Nov 1, 2017

View reviewed changes

jonasfranz reviewed Nov 1, 2017

View reviewed changes

lunny force-pushed the lunny/external_markup branch from 72817ce to 4bcca9d Compare November 2, 2017 06:04

jonasfranz approved these changes Nov 4, 2017

View reviewed changes

ethantkoenig approved these changes Nov 7, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 7, 2017

lunny added 10 commits November 7, 2017 14:23

add external markup render support

c5940d0

bug fixed

0feec65

refacotr codes and fix wrong error log

f39e5ab

fix comments and add check to prevent leaks

754332b

add check for config file and improve the example

c3f0803

check file close error

c44ff2e

use ioutil.TempFile instead uuid

4f0155b

correct Render -> Parser

83f9a3a

improve warning when incorrect markup setting

9c18491

fix typos

640eaba

lunny force-pushed the lunny/external_markup branch from b43874b to 640eaba Compare November 7, 2017 06:24

lafriks merged commit 62d0a4d into go-gitea:master Nov 7, 2017

lunny deleted the lunny/external_markup branch November 7, 2017 08:01

lunny mentioned this pull request Nov 10, 2017

Feature Request: render IPython/Jupyter Notebooks in repo view pane #2877

Closed

7 tasks

lunny mentioned this pull request Nov 29, 2017

Repo search could support files which is rendered by external tools. #3021

Open

sapk mentioned this pull request Jan 10, 2018

[feature] mermaid.js #3340

Closed

sapk mentioned this pull request Feb 2, 2018

LaTeX syntax support in wiki #3445

Closed

7 tasks

lunny mentioned this pull request Aug 14, 2018

Asciidoc support #4699

Closed

JedMeister mentioned this pull request Aug 30, 2018

Gitea - support for markup (e.g. rst & markdown) rendering turnkeylinux/tracker#1187

Closed

juanpablo-santos mentioned this pull request Nov 22, 2018

wish: document external renderers support on website #3758

Closed

6 tasks

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Add external markup render support #2570

Add external markup render support #2570

Conversation

lunny commented Sep 22, 2017 • edited Loading

codecov-io commented Sep 22, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Sep 22, 2017

lunny commented Sep 22, 2017

Morlinest commented Sep 22, 2017

lunny commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Oct 11, 2017

lunny commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Oct 31, 2017

Choose a reason for hiding this comment

lunny commented Nov 2, 2017

Morlinest commented Nov 4, 2017

lunny commented Nov 5, 2017

Morlinest commented Nov 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig commented Nov 7, 2017

lunny commented Nov 7, 2017

kellpossible commented Nov 9, 2017

lunny commented Nov 10, 2017

lunny commented Sep 22, 2017 •

edited

Loading

codecov-io commented Sep 22, 2017 •

edited

Loading

lunny Sep 22, 2017 •

edited

Loading

sapk Sep 25, 2017 •

edited

Loading