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

docs: add specific docs for each command and args #58

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

subhamkrai
Copy link
Collaborator

This commits add docs for each command and args that the plugin supports and also, update the readme doc.

Signed-off-by: subhamkrai srai@redhat.com

docs/health.md Outdated Show resolved Hide resolved
Copy link
Member

@parth-gr parth-gr left a comment

Choose a reason for hiding this comment

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

Looks good to me, just check the links are working fine :)

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

What about the following approach?

  • Keep a single readme in this repo where all the commands and args are basically documented
  • Create more detailed Rook docs with many examples. The Rook docs will have nice rendering, they're searchable, there can be a hierarchy of docs, and so on.

The downside is that the docs are disconnected from the krew repo, but it seems fine if it's just for more detailed examples.

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Let's discuss

@subhamkrai
Copy link
Collaborator Author

Let's discuss

yeah, let's discuss.

@travisn
Copy link
Member

travisn commented Sep 15, 2022

Another thought on the approach of using Rook docs... If we keep the single usage page in the krew repo, more detailed scenarios could be integrated to the Rook docs. For example, in the Rook DR docs we should use the krew commands and show how the DR is working end to end, based on krew status or other commands.

@subhamkrai subhamkrai force-pushed the add-docs branch 2 times, most recently from aa5b858 to ee1615f Compare September 19, 2022 12:25
Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

Overall, I think this looks really good. Just a few thoughts that are pretty minor.

docs/health.md Outdated
Comment on lines 14 to 16
1. `Info`: This is just a logging information for the users.

2. `Warning`: which mean there is some improvement required in the cluster.

3. `Error`: This reuires immediate user attentions to get the cluster in healthy state.
Copy link
Member

Choose a reason for hiding this comment

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

Are these separated by newlines for a particular reason?

Suggested change
1. `Info`: This is just a logging information for the users.
2. `Warning`: which mean there is some improvement required in the cluster.
3. `Error`: This reuires immediate user attentions to get the cluster in healthy state.
1. `Info`: This is just a logging information for the users.
2. `Warning`: which mean there is some improvement required in the cluster.
3. `Error`: This reuires immediate user attentions to get the cluster in healthy state.

Copy link
Collaborator Author

@subhamkrai subhamkrai Sep 21, 2022

Choose a reason for hiding this comment

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

yes, for the markdown files vsCode was showing errors, and suggestions to keep one newling for the heading or this similar tables.

Edit: I have made the changes the warnings are only for headings

docs/health.md Outdated Show resolved Hide resolved
docs/debug.md Outdated Show resolved Hide resolved
docs/ceph.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -6,20 +6,50 @@ Provide common management and troubleshooting tools for the [Rook Ceph](https://

## Install

Note: This required kubectl [krew](https://krew.sigs.k8s.io/docs/user-guide/setup/install/) to be installed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note: This required kubectl [krew](https://krew.sigs.k8s.io/docs/user-guide/setup/install/) to be installed.
!!! Note
This requires kubectl [krew](https://krew.sigs.k8s.io/docs/user-guide/setup/install/) to be installed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we are not re-directory these docs to the official rook doc, so not adding the ! ! ! Note format

README.md Outdated Show resolved Hide resolved
README.md Outdated

| Uses | Command | Args |
| :--: |:-------:| :---:|
| [Running ceph commands](docs/ceph.md) | `kubectl rook-ceph ceph <args>` | `<any ceph supported args>`|
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a table, how about a bullet list of the pages? The table repeats a lot of details that were already shown above in the usage. As a table of contents, they just need links to the other pages, then they can find the details of the commands and output on those pages.

Copy link
Collaborator Author

@subhamkrai subhamkrai Sep 28, 2022

Choose a reason for hiding this comment

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

changed to a bullet list.

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

just some small formatting suggestions

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/debug.md Outdated Show resolved Hide resolved
docs/debug.md Outdated Show resolved Hide resolved
docs/debug.md Outdated Show resolved Hide resolved
docs/debug.md Outdated Show resolved Hide resolved
docs/rook.md Outdated Show resolved Hide resolved
docs/rook.md Outdated
# objectbucketclaims.objectbucket.io:
```

## Status CR-Name
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Status CR-Name
## CephCluster Status

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with these we have to pass cr name for which we want the status, so kept it like this

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Looks great, just a few final nits

README.md Outdated Show resolved Hide resolved
docs/debug.md Outdated Show resolved Hide resolved
docs/debug.md Outdated Show resolved Hide resolved
docs/operator.md Outdated Show resolved Hide resolved
This commits add docs for each command and args that
the plugin supports and also, update the readme doc.

Signed-off-by: subhamkrai <srai@redhat.com>
@BlaineEXE BlaineEXE merged commit c7a0708 into rook:master Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants