-
Notifications
You must be signed in to change notification settings - Fork 272
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
Reframe CLI, revise quickstart, and reorganize tutorials #849
Conversation
f1af517
to
c5b4306
Compare
|
- correctly frame the CLI's current state as a tutorial toy. - provide a friendlier quickstart that puts what it's doing into perspective and guides you to next steps. - provide a better sense of what each tutorial/quickstart doc is for. - make the getting started page slightly more friendly. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
c5b4306
to
907186e
Compare
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 think your changes/additions to the quickstart guide are really good (left a few minor comments inline). The overall structure of the various documents can still be improved as you point out aptly in #808.
docs/QUICKSTART.md
Outdated
|
||
Unlike the underlying TUF modules that the CLI uses, the CLI itself is a bit | ||
bare-bones. Using the CLI is the easiest way to familiarize yourself with | ||
how TUF works, however. It will serve as a very basic update system and use |
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 think this is missing the end of the sentence.
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. ^_^
docs/QUICKSTART.md
Outdated
`tufclient` directories are created in the current working directory. | ||
**Step (0)** - Make sure TUF is installed | ||
|
||
See the [installation instructions for TUF](docs/INSTALLATION.rst). |
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.
We are in the same directory INSTALLATION.rst
. Remove docs
form relative path or use absolute path.
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.
Also, would it be bad to just provide the snippet here? So that quickstarters don't have to go through INSTALLATION.rst?
pip install tuf securesystemslib[crypto,pynacl]
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.
They do need to go through the installation instructions to install TUF. Prior text was a bit misleading in that it suggested that they only needed to run the ssl install. In any case, that step is listed two lines down, to emphasize it, since it would otherwise be optional.
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.
But isn't the line I'm suggesting all that's required for the quick start?
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, you need TUF installed.
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.
Simplified. Installing pip produces everything you need already on a basic Ubuntu platform, so I didn't delve into python-dev or libssl-dev, etc. Just provided the single command in the QUICKSTART.
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.
python-dev or libssl-dev
To contextualize, you need this to build the native support for the openssl backend. I think it's better to keep this from becoming a transient dependency as that may change at some point in the future...
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 clear on your suggestion, sorry: are you saying that we should avoid implicitly requiring that libssl-dev
or similar packages be installed, or that we should avoid advising it explicitly since it may not be necessary? (Or something else?)
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.
oh, the opposite. I understood that you were advising to remove these deps because something else is pulling them in. I'd rather have them explicitly just in case this is not the default in the future.
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.
Currently, those requirements are now only in the collaborator docs, not in the installation docs. I was wondering if I should move them back into the installation docs. You're suggesting that we include them, which is good to hear, so I'll pull them back in to the natural place to put them (the installation instructions). I'll frame them as as-necessary installations, I guess, to try to keep things simple.
That's beyond the scope of this PR, so I'll just do that in the next one, but for this PR, I'll split the difference in QUICKSTART.md
in anticipation of that change....
``` | ||
|
||
**Step (4)** - Fetch a target file from the repo. The client downloads | ||
any required metadata and the requested target file. | ||
**Step (4)** - Obtain and verify the `testfile` update on a client. |
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.
minuscule nit-pick: You end some heading sentences with a period and other not.
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.
Fixed
docs/GETTING_STARTED.rst
Outdated
- `CLI <CLI.md>`_ | ||
- `CLI Usage Examples <CLI_EXAMPLES.md>`_ | ||
- `Tutorial <TUTORIAL.md>`_ | ||
- Beginner Tutorials (using the basic command-line interface): `Quickstart <QUICKSTART.md>`_; `CLI Tutorial <CLI.md>`_; `CLI Further Examples <CLI_EXAMPLES.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.
The long line looks a little daunting now. I would leave QUICKSTART.md separate and see if the cli tutorials could be combined, or restructured? From a quick glance, CLI.md looks like a documentation of the interface, whereas CLI_EXAMPLES.md looks like a slightly more in-depth version of QUICKSTART.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.
Fixed, and I've now combined the two CLI documents.
- `CLI Usage Examples <CLI_EXAMPLES.md>`_ | ||
- `Tutorial <TUTORIAL.md>`_ | ||
- Beginner Tutorials (using the basic command-line interface): `Quickstart <QUICKSTART.md>`_; `CLI Tutorial <CLI.md>`_; `CLI Further Examples <CLI_EXAMPLES.md>`_ | ||
- `Advanced Tutorial <TUTORIAL.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.
If you call it advanced tutorial here, which I think is a good idea, you maybe also want to call it that in the document heading?
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.
Fixed there and elsewhere, thanks.
in the QUICKSTARD.md Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
46d3c95
to
1f3e5b6
Compare
Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
@awwad It's because |
Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
This PR:
CLI.md
andCLI_EXAMPLES.md
The larger task this PR is part of is Issue #808.