Skip to content

Conversation

zirain
Copy link
Member

@zirain zirain commented Oct 12, 2022

fix: istio/istio#41377

running make containers-test, got image gcr.io/istio-testing/build-tools:master-latest-amd64
then running istio/api with this image, everything looks good.

@zirain zirain requested a review from a team as a code owner October 12, 2022 08:46
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 12, 2022
@istio-policy-bot
Copy link

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 12, 2022
@zirain zirain changed the title [WIP]markdown: move BlackFriday to goldmark markdown: move BlackFriday to goldmark Oct 12, 2022
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 12, 2022
Copy link
Contributor

@ericvn ericvn left a comment

Choose a reason for hiding this comment

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

Thanks.

@ericvn
Copy link
Contributor

ericvn commented Oct 12, 2022

Looking into this change more I think we need to change the rendering option removing xhtml and breaks and add unsafe. Testing with that now.

@ericvn
Copy link
Contributor

ericvn commented Oct 12, 2022

Above from looking at the istio.io config.tml

@ericvn
Copy link
Contributor

ericvn commented Oct 12, 2022

Not necessarily sure why, but making the changes above did not reflect differently in the generated make gen output.

Figured out the issue. The tools Dockerfile install Protocol-gen-docs from the istio/tools master branch and doesn't use the locally updated code. The workaround is to create a new Dockerfile and copy a locally generated protoc-gen-docs binary over the one in a build-tools image.

@ericvn
Copy link
Contributor

ericvn commented Oct 12, 2022

Creating a simple program and running it:

package main

import (
	"bytes"
	"fmt"

	"github.com/yuin/goldmark"
	"github.com/yuin/goldmark/extension"
	"github.com/yuin/goldmark/parser"
	"github.com/yuin/goldmark/renderer/html"
)

func main() {
	var buf bytes.Buffer
	md := goldmark.New(
		goldmark.WithExtensions(extension.GFM),
		goldmark.WithParserOptions(
			parser.WithAutoHeadingID(),
		),
		goldmark.WithRendererOptions(
			html.WithUnsafe(),
		),
	)
	source := []byte("AnalysisMessageWeakSchema is the set of information \n that&rsquo;s needed to define a weakly-typed schema. \n The purpose of this proto is to provide a mechanism for validating \n istio/istio/galley/pkg/config/analysis/msg/messages.yaml to make sure that we \n don&rsquo;t allow committing underspecified types. \n {{<tab name=\"v1beta1\" category-value=\"v1beta1\">}}")
	if err := md.Convert(source, &buf); err != nil {
		panic(err)
	}
	fmt.Println(buf.String())
	return
}

seems to show a better output:

<p>AnalysisMessageWeakSchema is the set of information
that’s needed to define a weakly-typed schema.
The purpose of this proto is to provide a mechanism for validating
istio/istio/galley/pkg/config/analysis/msg/messages.yaml to make sure that we
don’t allow committing underspecified types.
{{<tab name="v1beta1" category-value="v1beta1">}}</p>

With the current code:

<p>AnalysisMessageWeakSchema is the set of information<br />
that’s needed to define a weakly-typed schema.<br />
The purpose of this proto is to provide a mechanism for validating<br />
istio/istio/galley/pkg/config/analysis/msg/messages.yaml to make sure that we<br />
don’t allow committing underspecified types.<br />
{{<!-- raw HTML omitted -->}}</p>

which shows the two differences.

@zirain zirain deleted the BlackFriday branch October 13, 2022 00:59
@zirain
Copy link
Member Author

zirain commented Oct 13, 2022

I will take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change from BlackFriday to Goldmark for generated docs in istio/tools.
4 participants