change hugo version unsafe #19907

jimangel · 2020-03-27T23:40:58Z

Ref: #19028
Ref: #19905

Umbrella issue for tracking problems here: #20335

More details captured here: #19028 (comment):

/cc @kbarnard10 @bradtopol @kbhawkey @sftim @lucperkins
/hold

k8s-ci-robot · 2020-03-27T23:41:03Z

@jimangel: GitHub didn't allow me to request PR reviews from the following users: lucperkins.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Ref: #19028
Ref: #19905

This is removing blackfriday params and using the non-default (WITH inline HTML).

Details captured from #19028 (comment):
@zacharysarah I was playing around with updating the site to Hugo 67.0 and discovered a few (related) bugs.

The feature-state partial template not loading properly is because Hugo v0.60.0+ disables inline HTML in >markdown.>

See this Hugo v0.60.0 release post from @bep.

It can ignored by adding the following to config.toml:
[markup]
 [markup.goldmark]
   [markup.goldmark.renderer]
     unsafe = true
We might do this to temporarily avoid heartburn, but it would be great to remove all unsafe inline HTML.

Why it was disabled by default: gohugoio/hugo#6581

After the change is made in config.toml the feature state button loads fine, but the feature state is expanded by default. For example:

I saw that @kbhawkey had some experience modifying the template script (in PR #9994). Any suggestion on the best way to resolve this would be appreciated!

Other things discovered

Pygments is removed as highlighter.

We might need to update our config.toml to be more inline with https://gohugo.io/getting-started/configuration-markup#highlight

Plain HTML documents are not supported

This causes issues with any files that are 100% HTML. As of now, the only files Hugo errors about are:
content/de/includes/user-guide-migration-notice.md
content/en/includes/user-guide-migration-notice.md
content/fr/includes/user-guide-migration-notice.md
content/ja/includes/user-guide-migration-notice.md
content/zh/includes/user-guide-migration-notice.md

# you can easily delete them with: 

find content/ -name "*user-guide-migration-notice.md" -exec git rm {} \;

# otherwise convert them to markdown if they are needed
We should probably delete blackfriday config.toml references
#[blackfriday]
#hrefTargetBlank = true
#fractions = false
Next steps

Let me know if you want to continue out of this PR or want me to open up a new one.

Update netlify.toml to 0.67.0

Delete or fix (convert to markdown) offending user-guide-migration-notice.md files in each localization

Decide if it's worth back-filling old releases (release-x.xx)

Remove blackfriday stuff in config.toml

Considerations

Kubernetes v1.18 it tentative to be released on Tuesday, March 24, 2020 which involves a lot of docs work.

Does this impact the current work being done to update our theme?

Removing all unsafe html should not fall off our radar.

Does goldmark / commonmark do anything else unknown to our existing docs?

Other

I found the CommonMark markdown spec useful to answer any formatting / HTML questions. CommonMark is the spec replacing the currently used Blackfriday spec.

Lastly, I encourage folks to read the release notes from 0.59.1 -> v0.67.0
/cc @kbarnard10 @bradtopol @kbhawkey @sftim @lucperkins
/hold

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 kubernetes/test-infra repository.

netlify · 2020-03-27T23:55:05Z

Deploy preview for kubernetes-io-master-staging ready!

Built with commit fdb2318

https://deploy-preview-19907--kubernetes-io-master-staging.netlify.app

kbhawkey · 2020-03-29T20:35:59Z

@jimangel , Do you know if Hugo provides details about what safe prevents versus what unsafe enables or permits?
Even with unsafe enabled, it appears that Markdown files that include some raw HTML no longer render,
Some kubectl pages:
https://deploy-preview-19907--kubernetes-io-master-staging.netlify.com/docs/reference/kubectl/kubectl/
https://deploy-preview-19907--kubernetes-io-master-staging.netlify.com/docs/tasks/tools/install-kubectl/

Most of the kubeadm pages:
https://deploy-preview-19907--kubernetes-io-master-staging.netlify.com/docs/reference/setup-tools/kubeadm/kubeadm-init/

Minikube:
https://deploy-preview-19907--kubernetes-io-master-staging.netlify.com/docs/tutorials/hello-minikube/

A while ago, I investigated the use of multiple feature state shortcodes on a single page.
Currently there is a small bug: only the first feature state shortcode on the page will activate the dialog to open (need to uniquely identify the feature state shortcodes on the page).
Clicking the feature state dialog link is supposed to provide the information about the state. With this change, it appears that the content is permanently displayed on the page (instead of displaying when clicked).

https://kubernetes.io/docs/concepts/storage/persistent-volumes/#expanding-persistent-volume-claims

A somewhat unrelated question, will the latest version of Hugo play well with the future docs theme?

jimangel · 2020-03-29T22:06:11Z

Great catch! Yea, this move is going to be tough...

Do you know if Hugo provides details about what safe prevents versus what unsafe enables or permits?

There is a great overview in this comment: gohugoio/hugo#6581 (comment)

We can also try to stick with blackfriday rendering - but I would really like us to transition to goldmark since there's been other formatting issues with blackfriday.

A somewhat unrelated question, will the latest version of Hugo play well with the future docs theme?

The theme change should mainly be CSS related but that is 100% something to consider as part of this upgrade.

jimangel · 2020-04-07T23:00:08Z

Adding in @bep's #19905 (comment):

A quick note from me re this:

I have some plans to add the Bluemonday HTML sanitizer to Hugo as an option -- which should be more flexible (and configurable). But time ...

You're in control of the content (to some extend, it is easy to miss stuff), so I'm not saying that running with "unsafe" is really unsafe.

You could also consider adding a Content Security Policy to your Netlify config. Note that I recently added support for custom headers in the Hugo server, which allows for testing these locally. The syntax matches Netlifys.

zacharysarah · 2020-04-07T23:16:58Z

I would really like us to transition to goldmark since there's been other formatting issues with blackfriday.

🙌 👍

tengqm · 2020-04-08T02:05:27Z

+1 on bumping the version. The issue related to nested lists is annoying.

bep · 2020-04-08T08:09:24Z

So, I looked at 1 example above:

https://deploy-preview-19907--kubernetes-io-master-staging.netlify.com/docs/reference/kubectl/kubectl/

And I'm guessing that this is hit by the 4 character indendation = code/preformatted rule. GoldMark is CommonMark compliant, which is mostly a good thing ...

You see it also when previewing in GitHub:

https://github.com/kubernetes/website/edit/master/content/en/docs/reference/kubectl/kubectl.md

Look how the HTML is greyed out ...

This should be fixable with some script, though ...

kbhawkey · 2020-04-08T15:52:30Z

So, I looked at 1 example above:

https://deploy-preview-19907--kubernetes-io-master-staging.netlify.com/docs/reference/kubectl/kubectl/

And I'm guessing that this is hit by the 4 character indendation = code/preformatted rule. GoldMark is CommonMark compliant, which is mostly a good thing ...

You see it also when previewing in GitHub:

https://github.com/kubernetes/website/edit/master/content/en/docs/reference/kubectl/kubectl.md

Look how the HTML is greyed out ...

This should be fixable with some script, though ...

Hi. Thanks for the tip. This reference page (a component tool page) is generated. I gather that the page would render if the HTML table is formatted to the CommonMark spec?
The HTML output should be cleaned up (validated). I logged an issue (kubernetes-sigs/reference-docs#148).

bep · 2020-04-08T21:06:55Z

I gather that the page would render if the HTML table is formatted to the CommonMark spec?

It would render if you use some other indentation than 4. So, 3 should be good...

kbhawkey · 2020-04-08T21:25:20Z

I just tested the rendering of the table with no indent HTML table elements.
Good time to do some cleanup.
The table renders if the Goldmark field unsafe = true is set.
With the non-compliant HTML, the parser (safe or unsafe) displays the raw HTML.

jimangel · 2020-05-09T17:55:49Z

I've rebased and bumped Hugo up to latest (0.70).
/cc @zacharysarah @kbarnard10 @kbhawkey @sftim @onlydole

onlydole · 2020-05-11T04:10:47Z

I like it! This seems great! I did some local tests and it works for me - I'll kick the tires a bit more tomorrow 👍🏼

/lgtm

sftim · 2020-05-11T09:06:41Z

When reviewing, what should I look at / for?

sftim · 2020-05-11T10:07:46Z

@jimangel I spotted that https://deploy-preview-19907--kubernetes-io-master-staging.netlify.app/docs/contribute/style/hugo-shortcodes/ doesn't render as expected.

Not sure what the fix is there; thought I'd mention it.

sftim · 2020-05-11T10:09:04Z

Essentially, the style guide previously suggested that tabs can contain raw HTML, and with CommonMark I suspect that they can't.

zacharysarah · 2020-05-11T21:33:14Z

/lgtm
/approve

k8s-ci-robot · 2020-05-11T21:33:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zacharysarah

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:

~~OWNERS~~ [zacharysarah]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zacharysarah · 2020-05-11T21:47:32Z

/hold cancel

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2020

k8s-ci-robot requested review from sftim and kbarnard10 March 27, 2020 23:41

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 27, 2020

k8s-ci-robot requested a review from bradtopol March 27, 2020 23:41

k8s-ci-robot requested a review from kbhawkey March 27, 2020 23:41

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Mar 27, 2020

jimangel force-pushed the hugo-unsafe-version branch from ac0cfab to 35d147a Compare March 27, 2020 23:46

gavinfish mentioned this pull request Apr 5, 2020

Issue with k8s.io/docs/reference/access-authn-authz/authentication/ #20086

Closed

jimangel mentioned this pull request Apr 7, 2020

change hugo version safe #19905

Closed

kbhawkey mentioned this pull request Apr 8, 2020

Make HTML table output Commonmark compliant kubernetes-sigs/reference-docs#148

Closed

jimangel force-pushed the hugo-unsafe-version branch from f699050 to fdb2318 Compare May 9, 2020 17:42

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2020

k8s-ci-robot requested review from onlydole and zacharysarah May 9, 2020 17:55

zacharysarah mentioned this pull request May 11, 2020

Revise the Kubernetes website to use Docsy #20874

Merged

k8s-ci-robot assigned onlydole May 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2020

This was referenced May 11, 2020

reformat kubelet ref page #20599

Merged

reformat tabs shortcode #20245

Merged

jimangel mentioned this pull request May 11, 2020

fix for feature state shortcode #20893

Merged

k8s-ci-robot assigned zacharysarah May 11, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2020

k8s-ci-robot merged commit de78f49 into kubernetes:master May 11, 2020

zacharysarah mentioned this pull request May 12, 2020

HTML breaks in /docs/concepts/cluster-administration/flow-control/ #20930

Closed

irvifa mentioned this pull request May 13, 2020

Change notes templating to be able to comply with Hugo 0.70. #20946

Merged

sftim mentioned this pull request May 14, 2020

Issue with k8s.io/docs/tasks/administer-cluster/kubeadm/kubeadm-upgrade/ #20942

Closed

change hugo version unsafe #19907

change hugo version unsafe #19907

Uh oh!

Conversation

jimangel commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 27, 2020

Other things discovered

Pygments is removed as highlighter.

Plain HTML documents are not supported

We should probably delete blackfriday config.toml references

Next steps

Considerations

Other

Uh oh!

netlify bot commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbhawkey commented Mar 29, 2020

Uh oh!

jimangel commented Mar 29, 2020

Uh oh!

jimangel commented Apr 7, 2020

Uh oh!

zacharysarah commented Apr 7, 2020

Uh oh!

tengqm commented Apr 8, 2020

Uh oh!

bep commented Apr 8, 2020

Uh oh!

kbhawkey commented Apr 8, 2020

Uh oh!

bep commented Apr 8, 2020

Uh oh!

kbhawkey commented Apr 8, 2020

Uh oh!

jimangel commented May 9, 2020

Uh oh!

onlydole commented May 11, 2020

Uh oh!

sftim commented May 11, 2020

Uh oh!

sftim commented May 11, 2020

Uh oh!

sftim commented May 11, 2020

Uh oh!

zacharysarah commented May 11, 2020

Uh oh!

k8s-ci-robot commented May 11, 2020

Uh oh!

zacharysarah commented May 11, 2020

Uh oh!

Uh oh!

jimangel commented Mar 27, 2020 •

edited

Loading

We should probably delete blackfriday `config.toml` references

netlify bot commented Mar 27, 2020 •

edited

Loading