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

Fix list formatting in kubeadm setup tutorial #8663

Merged

Conversation

tfogo
Copy link
Contributor

@tfogo tfogo commented May 21, 2018

There is an issue with ordered lists in the Creating HA clusters with
kubeadm
setup tutorial. All list items are labeled 1. This is because
the underlying markdown processor for Hugo, blackfriday, requires
list items with multiple paragraphs to have four space indentation.
(See russross/blackfriday#244)

This commit adds an extra space before paragraphs in lists so they are
correctly interpreted as multi-paragraph lists.

However, this exposes a bug in blackfriday where lines beginning with
- in codefenced code blocks inside lists are parsed as sub-lists.
This means code fenced code blocks inside lists that contain yaml are
malformed. (See russross/blackfriday#239)

So this commit mitigates this bug in two situations. It uses a 4-space
indented code block in one situation. In another situation where a list
only contained one element, it is no longer a list.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 21, 2018
@tfogo
Copy link
Contributor Author

tfogo commented May 21, 2018

/assign @MistyHacks

@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented May 21, 2018

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

Built with commit 51c1e7f

https://deploy-preview-8663--kubernetes-io-master-staging.netlify.com

@neolit123
Copy link
Member

yikes, this is horrible. not your efforts @tfogo, but rather the underlying bugs...
how about not using ordered lists at all if they are that "demanding" / buggy and use unordered lists or manual ordering (somehow) until the issues are resolved at the markdown processor?

@tfogo @MistyHacks WDYT?

@tfogo
Copy link
Contributor Author

tfogo commented May 21, 2018

@neolit123 I know! It's a pretty awkward bug, particularly for our yaml heavy use case. There's been open PR to fix it for several months (russross/blackfriday#372).

Unordered lists are also affected by the bug, unfortunately.

I think manual ordering might work well in some cases. We could manually number each item in the list, but multi-paragraph list items won't be properly indented. Which means they aren't visually grouped together. Alternatively, we could signpost with bolded text (e.g. Step 1:, Step 2:).

Whatever is decided, it might be useful to add a note to the style guide (similar to the Common Shortcode Issues section).

@neolit123
Copy link
Member

Alternatively, we could signpost with bolded text (e.g. Step 1:, Step 2:).

i personally i like this solution. if the ordering is broken at the backend we should write the steps ourselves.
more difficult to maintain, yet the steps aren't that many.

@mdlinville
Copy link
Contributor

There is a slightly cleaner way to fix this. Let me add a commit.

@mdlinville
Copy link
Contributor

OK, I added my commit. What I did was this:

  • Always line up text after the step numbers with tabstops. This helps with debugging later, even though it means that single-digit steps numbers get an extra space after the period.
  • Indent all block-level things within list items to an extra tabstop. In my testing for the new smoke test page, this was the way to get the most consistent results.
  • While I was in there, I wrapped some admonitions in the correct short tags, rewrapped some long lines, and did light rewording in places that really stood out to me.
  • I also fixed some of the code fences so that they don't break Atom so badly when editing the files there. This has to do with wrapping things in shell that don't look like shell output (like all the HEREDOC stuff, especially HEREDOC that is YAML or JSON). This is purely to help future editors of this file.

@mdlinville mdlinville force-pushed the tf-install-kubeadm-formatting branch from ed6dd39 to 973eb96 Compare May 22, 2018 00:42
@mdlinville
Copy link
Contributor

/lgtm

@tfogo please ack my content changes here. The output looks fine to me now, though the sub-lists look kind of ugly. If you agree, maybe file a GH issue for CSS.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2018
@tfogo
Copy link
Contributor Author

tfogo commented May 22, 2018

@MistyHacks Thanks, looks cleaner for sure!

One awkward thing about indenting block-level things inside lists an extra tabstop is that code blocks all have two spaces before each line. This is a bit awkward for copy-paste. Particularly with commands that are piping a heredoc to a file. Do you have any thoughts on this?

```

{{< note >}}
**Optional:** You can modify `ca-csr.json` to add a section for `names`. See [the CFSSL wiki](https://github.com/cloudflare/cfssl/wiki/Creating-a-new-CSR) for an example.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This outputs the contents in a code block due to the indentation. Putting it all on one line would fix this.


**Note:** If you are using Kubernetes 1.9+, you can replace the `apiserver-count: 3` extra argument with `endpoint-reconciler-type: lease`. For more information, see [the documentation](/docs/admin/high-availability/#endpoint-reconciler).
{{< note >}}
**Note:** If you are using Kubernetes 1.9+, you can replace the `apiserver-count: 3` extra argument with `endpoint-reconciler-type: lease`. For more information, see [the documentation](/docs/admin/high-availability/#endpoint-reconciler).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same issue with this one.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2018
@tfogo
Copy link
Contributor Author

tfogo commented May 22, 2018

Added a fix for the shortcodes - I know it's ugly that they're on the same line. If you know of a better fix, let me know :)

```shell
export PEER_NAME=$(hostname)
export PRIVATE_IP=$(ip addr show eth1 | grep -Po 'inet \K[\d.]+')
```
Copy link
Member

Choose a reason for hiding this comment

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

can we just leave everything aligned to the left? i don't think it looks bad without it.
once the ordered list bugs are fixed we just have to modify the digits and the extra space and the alignment will be auto-added; if i understand this correctly, that is...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think leaving everything aligned to the left is necessary. The sublist bug only affects two code blocks and we've edited it in such a way that they're no longer affected. So we have the list indentation being rendered correctly and there are no sublist bugs.

And we have no idea when a fix to this bug will make its way into Hugo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving everything aligned to the left seems to sometimes work and sometimes not. Indenting everything a level seems to always work. I think we have to teach contributors how to contribute docs in the way that creates the least amount of cognitive load, so it's easier to give guidance to always indent things inside lists another level.

@mdlinville
Copy link
Contributor

mdlinville commented May 23, 2018

There are multiple bugs with Hugo / BlackFriday and list handling. One is the "bullet lists" inside code blocks (which has the likelihood of screwing up lots of YAML code blocks, cc/ @steveperry-53 who raised this yesterday). The other is the very loosely deterministic way that nested lists and nested content inside lists is handled, depending on the indentation level. The second one seems to be "working as intended" / "not defined in the Markdown spec", so I have no optimism about it getting fixed.

cc/ @spf13 @bep for visibility.

{{% /tab %}}
{{< /tabs >}}



Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This probably can be reverted (did I do this?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! I'll fix that.

@tfogo
Copy link
Contributor Author

tfogo commented May 23, 2018

@MistyHacks I'm working with the author of this PR (russross/blackfriday#372) to fix the lists inside code blocks issue with blackfriday. I'm hopeful that we can get that fix merged in soon. :)

@tfogo tfogo force-pushed the tf-install-kubeadm-formatting branch from aba6da3 to e972e52 Compare May 23, 2018 18:04
@tfogo
Copy link
Contributor Author

tfogo commented May 23, 2018

@MistyHacks I fixed that newline and rebased.

Do you have any thoughts on the code blocks having an extra two spaces? I mentioned above that it can be awkward for copy-pasting heredocs. I don't think it's a huge issue - but it might be good to revisit it when blackfriday is acting more predictable.

@bep
Copy link
Contributor

bep commented May 23, 2018

As to code blocks vs copy and paste, you may consider using the "table format", which should give you copy-and-paste friendly blocks.

See example here:

https://github.com/linode/docs

A big Hugo docs site (bigger than Kubernetes) with lots of code examples. No complaints about extra spaces, so might be a good source of inspiration.

@zacharysarah
Copy link
Contributor

More on removing code formatting from headers: this is what was happening.
screen shot 2018-06-04 at 12 43 47 pm

@MistyHacks Any recommendation about how to proceed here?

@mdlinville
Copy link
Contributor

I thought @bep fixed this in #8797? I don't think you should have to remove them.

@Bradamant3
Copy link
Contributor

@tfogo if you could please rebase/resolve conflicts, we can get this merged. It also looks as though your HTML comment about code fencing appears in the config file example -- please remove :-)

@mdlinville mdlinville force-pushed the tf-install-kubeadm-formatting branch from eebee10 to 5345332 Compare June 8, 2018 18:52
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 8, 2018
@mdlinville
Copy link
Contributor

This became a mess because a lot of these fixes have actually already been made in master. I worked with @Bradamant3 to figure out which changes were unique to this PR and I re-applied those, and also fixed an issue I saw with the tabs in the content and another with a skip in heading order level.

@mdlinville mdlinville force-pushed the tf-install-kubeadm-formatting branch from 5345332 to a28f5aa Compare June 8, 2018 18:59
@mdlinville mdlinville force-pushed the tf-install-kubeadm-formatting branch from a28f5aa to 51c1e7f Compare June 8, 2018 19:09
@Bradamant3
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bradamant3

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2018
@k8s-ci-robot k8s-ci-robot merged commit 6608a4c into kubernetes:master Jun 8, 2018
@tfogo
Copy link
Contributor Author

tfogo commented Jun 10, 2018

@MistyHacks @Bradamant3 Thank you for going through and doing that.

FYI, we've fixed the blackfriday issue in v2 (russross/blackfriday#372) but still need to backport to v1 for Hugo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants