-
Notifications
You must be signed in to change notification settings - Fork 35
toolkit: install guide: refer to section about supported platforms, tweak headings #158
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
toolkit: install guide: refer to section about supported platforms, tweak headings #158
Conversation
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
Documentation preview |
|
Sweeet! 🎈 Looking at that. Edit: the cross ref is correct, the anchor works. |
container-toolkit/install-guide.md
Outdated
|
|
||
| Alternatively, you can install the driver by downloading a `.run` installer. | ||
| Refer to the NVIDIA [Official Drivers](https://www.nvidia.com/Download/index.aspx?lang=en-us) page. | ||
| 2. Read [this section](./supported-platforms.md) about platform support. |
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.
Nit: Should this be first?
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.
+1 i think it should be first as well
and an additional nit to embed the text into the link, so something like
Make sure you are using a [supported platform](./supported-platforms.md).
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.
Nit: Should this be first?
Yes, I was hoping for that feedback!
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.
something like Make sure you are using a supported platform.
In general I like this approach. Here I opted for a subtly different approach because:
It's really a section about platform support in general, and we precisely now want to be mindful to not communicate something in the lines of "do not continue if you do not use a strictly supported platform" :).
Related: #62 and e.g. NVIDIA/nvidia-container-toolkit#482 (comment).
container-toolkit/install-guide.md
Outdated
| 2. Read [this section](./supported-platforms.md) about platform support. | ||
|
|
||
| ### Installing with Apt | ||
| ### Debian-based distributions (Ubuntu): |
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.
Does this read as if we're only targetting Ubuntu?
| ### Debian-based distributions (Ubuntu): | |
| ### Debian-based distributions (e.g. Ubuntu): |
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.
Right, I forgot to add ", ..." like I did below. In this case I think I like this a little better than "e.g."
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.
Now I have clarity: we don't need generic enumerations. In these headings, we want to make sure we list all those distros that are in the table of "supported platforms". That's all we need. Pushed a commit.
container-toolkit/install-guide.md
Outdated
| ``` | ||
|
|
||
| ### Installing with Yum or Dnf | ||
| ### RPM-based distributions (RHEL/CentOS, Fedora, Amazon Linux, ...): |
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.
| ### RPM-based distributions (RHEL/CentOS, Fedora, Amazon Linux, ...): | |
| ### RPM-based distributions (e.g. RHEL/CentOS, Fedora, Amazon Linux, ...): |
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 started to overthink this. Removed the "RPM-based distributions" again.
Pushed a new commit, let's see how this looks when its rendered! :) I like to first-class the distribution names instead of the tooling names (that was my main angle here). There's no perfect solution though.
Some inspiration from..
chrome
vscode
gcloud cli
signal
container-toolkit/install-guide.md
Outdated
| ``` | ||
|
|
||
| ### Installing with Zypper | ||
| ### OpenSUSE/SLE: |
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's a little strange that this is also RPM-based but has its own section. Should we call that out above or have subsections for the two package manager types?
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.
Yes you have a strong point here. Will think about that.
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.
Happy with the current state: https://nvidia.github.io/cloud-native-docs/review/pr-158/container-toolkit/latest/install-guide.html.
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
container-toolkit/install-guide.md
Outdated
| Refer to the NVIDIA [Official Drivers](https://www.nvidia.com/Download/index.aspx?lang=en-us) page. | ||
|
|
||
| ### Installing with Apt | ||
| ### Ubuntu and Debian |
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 happy with this change for ubuntu and debian since I think this covers most users of apt. My concern is that using the same construct for RPM-based distributions where there are other prominent ones that are not listed. (Rocky comes to mind). This will definitely lead to GitHub issues along the lines of "RockyLinux support when?". At present my response is to link the "Installing with Yum or Dnf" section, but I can see more questions being raised if I link a section that's titled "RHEL/CentOS, Fedora, Amazon Linux".
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.
yes. note: added a commit after working on this with Evan
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
|
Pushed more commits after iterating on this with Evan. Will want to double-check the next rendered version before merge. Will report back. |
|
Good: the legacy anchors now work. Example: https://nvidia.github.io/cloud-native-docs/review/pr-158/container-toolkit/latest/install-guide.html#installing-with-apt |
elezar
left a comment
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 happy with these. Thanks @jgehrcke
| Alternatively, you can install the driver by [downloading](https://www.nvidia.com/en-us/drivers/) a `.run` installer. | ||
|
|
||
| Alternatively, you can install the driver by downloading a `.run` installer. | ||
| Refer to the NVIDIA [Official Drivers](https://www.nvidia.com/Download/index.aspx?lang=en-us) page. |
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.
Is there a reason why we got ride of this link? I dont remember if this has come up before in a previous comments. thanks
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.
No removal. I replaced it with a more modern URL. You will notice in your browser that the old URL redirects to /en-us/drivers.
Personally, I like to look at changes like this in the rendered view.
We should also work towards using a concept often called "semantic line breaks" to keep the markdown source diff better reviewable.
|
Thanks for discussion, let's certainly iterate more soon! |





I find it important that the installation section informs about supported platforms. We have a section for that (in a different doc) so all we are missing is a cross-reference.
Is Sphinx our documentation generator? I think / hope that cross-references in md-sphinx work as I am trying here, but I did not test this locally.
Also tweaking the headings to shift attention to the distribution names more than the tooling.