-
Notifications
You must be signed in to change notification settings - Fork 108
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
update readthedocs config and add installation docs #3436
Conversation
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.
Thanks; a few more. 😆
"sphinx.ext.autosectionlabel", | ||
"sphinx_design", | ||
"myst_parser", | ||
"sphinx_copybutton", |
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.
It would be good to keep these entries in order.
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.
These still need to be ordered, e.g.
"myst_parser",
"sphinx.ext.autosectionlabel",
"sphinx_copybutton",
"sphinx_design",
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.
Looks good, although there are a couple more opportunities to polish.
In the following, we describe how to install Pbench Agent using an ansible playbook. | ||
|
||
:::{note} | ||
The same Pbench Agent version must be installed on all the test systems that participate in a benchmark run, there is no support for mixed installations. |
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 wasn't sufficiently specific...my apologies, @vishalvvr!
The comma at line 3 was fine; it's line 6 which needs the change. (Please change line 3 back.... 😊)
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.
Looks good 👍
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 agree that it'd be good to have a consistent style in referring to Ansible. I also found one link that bothers me; but otherwise this looks good.
7a5beee
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 resolved all my previous open conversations, but there's one you seem to have missed. Instead of just referring back to it on the old commit, I'm entering it again ...
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'm not going to hold the merge for them, but I've brought forward some of my comments from earlier in the review which are still open.
2. Install the `pbench.agent` ANSIBLE collection from Ansible Galaxy. | ||
|
||
```console | ||
ansible-galaxy collection install pbench.agent | ||
``` | ||
|
||
3. Tell ansible where to find these roles. |
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.
Line 19 still references "ansible" with a spelling different from that used in lines 13, 11, and 3.
In the following, we describe how to install Pbench Agent using an ansible playbook. | ||
|
||
:::{note} | ||
The same Pbench Agent version must be installed on all the test systems that participate in a benchmark run, there is no support for mixed installations. |
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.
Line 3: remove the colon, and possibly remove the "In the following" phrase, since it's kind of obvious. If the result doesn't read well, try something like "This document describes" (which allows us to escape using the first-person pronoun).
Line 6: the comma should be replaced by a colon or m-dash (or period).
"sphinx.ext.autosectionlabel", | ||
"sphinx_design", | ||
"myst_parser", | ||
"sphinx_copybutton", |
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.
These still need to be ordered, e.g.
"myst_parser",
"sphinx.ext.autosectionlabel",
"sphinx_copybutton",
"sphinx_design",
|
||
:::{note} | ||
- We release Pbench Agent RPMs under the `ndokos` COPR account with repos following the pattern `pbench-<release>`. | ||
- There are some RPMs that are shared between versions (e.g. pbench-sysstat). We maintain those in [ndokos/pbench](https://copr.fedorainfracloud.org/coprs/ndokos/pbench) repo. |
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.
As per the earlier discussion, we should probably omit the hyperlink here. (If you want the user to be able to browse, we should direct them to the ndokos
account page rather to any specific "project" under it; in terms of finding specific packages, that will be handled automagically one the user enables the repo using the DNF copr
pluging, so the reader doesn't need the hyperlink for that.)
This text would read better if you inserted the word "the" before "nodokos".
I would put ndokos/pbench
in monospace font.
- update readthedocs config to support copy action in command palette. - add installation guide for pbench-agent via rpm, ansible and containers. - mapped end-to-end workflow page to index.rst - update docs/readme Demo [link](https://pb-readthedocs.readthedocs.io/en/latest/Agent/installation/index.html)
Demo page
PBENCH-1152