-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(docs): update links in isolation docs #13295
Conversation
Signed-off-by: Eric Shen <ericshenyuhao@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave one comment. PTAL
@@ -31,7 +31,7 @@ pulsar-admin ns-isolation-policy set options | |||
|
|||
``` | |||
|
|||
For more information about the command `pulsar-admin ns-isolation-policy set options`, see [here](https://pulsar.apache.org/tools/pulsar-admin/). | |||
For more information about the command `pulsar-admin ns-isolation-policy set`, see [here](https://pulsar.apache.org/tools/pulsar-admin/2.8.0-SNAPSHOT/#-em-set-em-). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add a 2.8.0 link in 2.7.2 release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a mistake, i'll update it.
Signed-off-by: Eric Shen <ericshenyuhao@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -22,7 +22,7 @@ You can set a namespace isolation policy for a cluster using one of the followin | |||
pulsar-admin ns-isolation-policy set options | |||
``` | |||
|
|||
For more information about the command `pulsar-admin ns-isolation-policy set options`, see [here](https://pulsar.apache.org/tools/pulsar-admin/). | |||
For more information about the command `pulsar-admin ns-isolation-policy set`, see [here](https://pulsar.apache.org/tools/pulsar-admin/2.8.0-SNAPSHOT/#-em-set-em-). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericsyh thanks for your consideration.
However, please do not change these general links to specified links, or else we need to update them for each release, there might be hundreds of occurrences. So here, we use general links on purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But it is confusing to me that why we need to have docs for each patch version? In my understanding, patch version onlly including bug fix and performance improvements and new features are only included in major and minor verion. So, we just need to keep one version docs for each minor version which means all 2.8.x verions just use a unified docs 2.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor versions include code improvements which need to update docs as well
For example,
- Websocket should pass the encryption context to the consumers #12539 need to update docs for 2.8.2 and 2.9.1, which means no need to update doc versions before 2.8.1
- Another example is, let's say, after a minor version is released, three months later, there is a patch to that release and needs to update docs, we need to make specific doc changes to that release as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emmmm, #12539 seems a function enhenment PR and actually i think it should be released in a new minor version like 2.10.0 rathar than patch version 2.8.2 and 2.9.1.
I take a look at other open source projects, most of them just maintain the minor version docs:
- K8s:
It's really a heavy burden to maintain the docs for each patch version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
The code changes Websocket should pass the encryption context to the consumers #12539 go into major and minor versions, so doc changes go those versions as well
-
Yes, Pulsar maintains versioned docs as well. For each minor version, for example, 2.7.x, you never know whether there might be code (doc) changes after it is released, so we maintain all of them for completeness
Signed-off-by: Eric Shen ericshenyuhao@outlook.com
Motivation
The command links in https://pulsar.apache.org/docs/en/administration-isolation/ are using https://pulsar.apache.org/tools/pulsar-admin/ which is not clear for users to get to the target command.
Modifications
Update the links in https://pulsar.apache.org/docs/en/administration-isolation/ to target command docs.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
doc-required
(If you need help on updating docs, create a doc issue)
no-need-doc
(Please explain why)
doc
(If this PR contains doc changes)