Skip to content

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.

Sign up for GitHub

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

Merged
jgehrcke merged 7 commits into from
Feb 19, 2025
Merged

toolkit: install guide: refer to section about supported platforms, tweak headings #158

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8316505
Tweak headings in toolkit install guide
jgehrcke Feb 13, 2025
550b98a
Refer to supported platforms section in toolkit install guide
jgehrcke Feb 13, 2025
c85c37c
review feedback: work on section headings
jgehrcke Feb 13, 2025
3b8d4df
simplify
jgehrcke Feb 13, 2025
06bd2b8
collab with evan: headings, notes
jgehrcke Feb 14, 2025
ad56540
use legacy heading anchors
jgehrcke Feb 14, 2025
cd38c8b
remove todo
jgehrcke Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions container-toolkit/install-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,16 @@

### Prerequisites

Install the NVIDIA GPU driver for your Linux distribution.
1. Install the NVIDIA GPU driver for your Linux distribution.
NVIDIA recommends installing the driver by using the package manager for your distribution.

For information about installing the driver with a package manager, refer to
the [_NVIDIA Driver Installation Quickstart Guide_](https://docs.nvidia.com/datacenter/tesla/tesla-installation-notes/index.html).
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.
Copy link
Collaborator

@a-mccarthy a-mccarthy Feb 19, 2025

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

Copy link
Contributor Author

@jgehrcke jgehrcke Feb 19, 2025 •
edited
Loading

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.

2. Read [this section](./supported-platforms.md) about platform support.
Copy link
Member

@elezar elezar Feb 13, 2025

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?

Copy link
Collaborator

@a-mccarthy a-mccarthy Feb 13, 2025

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).

Copy link
Contributor Author

@jgehrcke jgehrcke Feb 13, 2025

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!

Copy link
Contributor Author

@jgehrcke jgehrcke Feb 13, 2025

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).


### Installing with Apt
### Debian-based distributions (Ubuntu):
Copy link
Member

@elezar elezar Feb 13, 2025

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?

Suggested change
### Debian-based distributions (Ubuntu):
### Debian-based distributions (e.g. Ubuntu):

Copy link
Contributor Author

@jgehrcke jgehrcke Feb 13, 2025

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."

Copy link
Contributor Author

@jgehrcke jgehrcke Feb 13, 2025

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.


1. Configure the production repository:

Expand Down Expand Up @@ -53,7 +53,7 @@ Refer to the NVIDIA [Official Drivers](https://www.nvidia.com/Download/index.asp
$ sudo apt-get install -y nvidia-container-toolkit
```

### Installing with Yum or Dnf
### RPM-based distributions (RHEL/CentOS, Fedora, Amazon Linux, ...):
Copy link
Member

@elezar elezar Feb 13, 2025

Choose a reason for hiding this comment

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

Suggested change
### RPM-based distributions (RHEL/CentOS, Fedora, Amazon Linux, ...):
### RPM-based distributions (e.g. RHEL/CentOS, Fedora, Amazon Linux, ...):

Copy link
Contributor Author

@jgehrcke jgehrcke Feb 13, 2025 •
edited
Loading

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

image

vscode

image

gcloud cli

image

signal

image


1. Configure the production repository:

Expand All @@ -74,7 +74,7 @@ Refer to the NVIDIA [Official Drivers](https://www.nvidia.com/Download/index.asp
$ sudo yum install -y nvidia-container-toolkit
```

### Installing with Zypper
### OpenSUSE/SLE:
Copy link
Member

@elezar elezar Feb 13, 2025

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?

Copy link
Contributor Author

@jgehrcke jgehrcke Feb 13, 2025

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.

Copy link
Contributor Author

@jgehrcke jgehrcke Feb 14, 2025

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.


1. Configure the production repository:

Expand Down