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

Fixed numbering for installation steps in the tip #13778

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

archups
Copy link
Contributor

@archups archups commented Aug 28, 2023

Please provide a description for what this PR is for.

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

  • Ambient
  • [ x] Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

@archups archups requested review from a team as code owners August 28, 2023 19:16
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@istio-policy-bot
Copy link

😊 Welcome! This is either your first contribution to the Istio documentation repo, or
it's been awhile since you've been here. A few things you should know:

  • You can learn about how we write and maintain documentation, about our style guidelines,
    and about all the available web site features by visiting Contributing to the Docs.

  • In the next few minutes, an automatic preview of your change will be
    built as a full copy of the istio.io website. You can find this preview by clicking on
    the Details link next to the deploy/netlify entry in the Status section of this
    page.

  • We care about quality, so we've put in place a number of checks to ensure our documentation
    is top notch. We do spell checking, we sanitize the markdown, we ensure all hyperlinks point
    to valid location, and more. If your PR doesn't pass one of these checks, you'll see a red X in the
    status section of the page. Click on the Details link to get a list of the problems with your PR.
    Fix those problems and push an update to your PR. This will automatically rerun the tests and
    hopefully this time everything will be perfect.

  • Once your changes are accepted and merged into the repository, they will initially show up
    on https://preliminary.istio.io. The changes will be published to https://istio.io
    the next time we do a major release (which typically happens every 3 months or so).

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test labels Aug 28, 2023
@istio-testing
Copy link
Contributor

Hi @archups. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ericvn
ericvn previously requested changes Aug 29, 2023
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.

The original numbers are correct. All items should be labeled 1., and the will be automatically numbered,

Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Aug 29, 2023
@archups
Copy link
Contributor Author

archups commented Aug 29, 2023

The original numbers are correct. All items should be labeled 1., and the will be automatically numbered,

@ericvn Yes, it works usually that way, but not within tip. Please check the rendered number on the doc site itself https://istio.io/latest/docs/setup/install/operator/.
Hence PR raised for fix. Thanks for review!

@ericvn
Copy link
Contributor

ericvn commented Aug 29, 2023

If numbering doesn't work, I'd maybe recommend just using - instead of #. and just have bullets. @frankbu thoughts?

@frankbu
Copy link
Collaborator

frankbu commented Aug 29, 2023

Using 1) and 2) for the list items, instead of 1. and 2., might work.

@ericvn
Copy link
Contributor

ericvn commented Aug 29, 2023

Using 1) and 2) for the list items, instead of 1. and 2., might work.

I did try that locally, and it didn't seem to work for me. Also tried some indentation changes.

@frankbu
Copy link
Collaborator

frankbu commented Aug 29, 2023

I did try that locally, and it didn't seem to work for me.

What did it display? It should work.

@ericvn
Copy link
Contributor

ericvn commented Aug 29, 2023

I did try that locally, and it didn't seem to work for me.

What did it display? It should work.

Using 1. It showed both as 1. which is the problem to solve. Changing to 1) left them both showing as 1. Changing to - showed each as a bullet. All this with no other indentation. Looking at the public index.html with the )s:

<div>
    <aside class="callout tip">
        <div class="type">
            <svg class="large-icon"><use xlink:href="/img/icons.svg#callout-tip"/></svg>
        </div>
        <div class="content"><p>You can alternatively deploy the operator using Helm:</p>
<ol>
<li>
<p>Create a namespace <code>istio-operator</code>.</p>
 <pre><code class='language-bash' data-expandlinks='true' data-repo='istio' >$ kubectl create namespace istio-operator
</li>
</ol>
<p></code></pre></p>
<ol>
<li>
<p>Install operator using Helm.</p>
 <pre><code class='language-bash' data-expandlinks='true' data-repo='istio' >$ helm install istio-operator manifests/charts/istio-operator \
 --set watchedNamespaces=&#34;istio-namespace1\,istio-namespace2&#34; \
 -n istio-operator
</li>
</ol>
<p></code></pre></p>
<p>Note that you need to <a href="/docs/setup/getting-started/#download">download the Istio release</a>
to run the above command.</p>
</div>
    </aside>
</div>

@ericvn
Copy link
Contributor

ericvn commented Aug 29, 2023

Looking through other parts of the doc, I do see #) used, most notably it seems when we need to restart numbering at some number. And it seems to be around tabsets. I really didn't see anything that related to numbering within a tip, and only see 2. used within a section header.

Looking at this particular page, I don't really see numbering used at all, except in this tip, so removing the numbering makes sense to me. Keeping a bullet (using -) does seem like an acceptable change if we wanted to call out the steps separately.

@frankbu
Copy link
Collaborator

frankbu commented Sep 5, 2023

Looking through other parts of the doc, I do see #) used, most notably it seems when we need to restart numbering at some number.

Markdown lets you continue a list that otherwise is disconnected from the previous items by specifying something other than 1 for the entry, but the lint rule we've enabled for the docs (MD029) requires all list entries to be 1, so it won't allow 2.. The work around is that the lint rule only enforces the rule for the . syntax, but markdown allows either . or ) for ordered lists. So using 2) won't cause a lint error, and will continue the list at 2. Here's what I get when I tried:
image

@@ -77,7 +77,7 @@ You can alternatively deploy the operator using Helm:
$ kubectl create namespace istio-operator
{{< /text >}}

1. Install operator using Helm.
2. Install operator using Helm.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
2. Install operator using Helm.
2) Install operator using Helm.

@@ -75,7 +75,7 @@ $ istioctl operator init --watchedNamespaces=istio-namespace1,istio-namespace2
$ kubectl create namespace istio-operator
{{< /text >}}

1. 使用 Helm 安装 Operator。
2. 使用 Helm 安装 Operator。
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
2. 使用 Helm 安装 Operator。
2) 使用 Helm 安装 Operator。

@archups archups requested review from frankbu, ericvn and Arhell September 8, 2023 09:54
@frankbu frankbu dismissed ericvn’s stale review September 8, 2023 20:06

dismiss obsolete review

@istio-testing istio-testing merged commit e2adf57 into istio:master Sep 8, 2023
@ericvn
Copy link
Contributor

ericvn commented Sep 11, 2023

/cherry-pick release-1.19

@istio-testing
Copy link
Contributor

@ericvn: new pull request created: #13850

In response to this:

/cherry-pick release-1.19

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/docs ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants