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

Add manual pages #26

Merged
merged 23 commits into from
Nov 19, 2024
Merged

Add manual pages #26

merged 23 commits into from
Nov 19, 2024

Conversation

mozzieongit
Copy link
Member

@mozzieongit mozzieongit commented Nov 14, 2024

Adds manual pages for the six commands based on the help output (of dnst and ldns) and the ldns manuals. I only included arguments actually supported by the dnst reimplementation. Once everything is ready, this will very likely need updating.

This also incorporates and updates the changes from #25.

@mozzieongit mozzieongit self-assigned this Nov 14, 2024
@mozzieongit mozzieongit added documentation Improvements or additions to documentation needs-review labels Nov 14, 2024
@AlexanderBand
Copy link
Member

Maybe "The manual goes here ..." in index.rst should be replaced with something describing the project. I'm not sure what but perhaps these pages can provide some inspiration:
https://www.nlnetlabs.nl/projects/ldns/about/
https://www.nlnetlabs.nl/documentation/ldns/index.html

@AlexanderBand AlexanderBand requested a review from a team November 14, 2024 17:46
@tertsdiepraam
Copy link
Contributor

tertsdiepraam commented Nov 14, 2024

I think we should merge this because this is fantastic and we can fix all my nitpicky issues later, but...

NITPICK MODE ACTIVATED

I don't like how the synopsis renders as:

dnst [options] command [args]

I'd expect

dnst [options] <command> [args]

The main dnst docs should also mention how the ldns tools fit into this and how to use them with symlinks.

The styling is sometimes inconsistent:

  • ldns-nsec3-hash doesn't use <> around values
  • ldns-keygen is also missing some <> and uses lowercase
  • A few places can put some more in code blocks, like K<name>+<alg>+<id> in ldns-keygen

I think that the ldns tools should refer to the dnst tools to motivate people to update their scripts.

NITPICK MODE DEACTIVATED

That being said, these are all written very well. Amazing!

Copy link
Contributor

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I put yesterdays nitpicks into comments.

Something for later: we should add examples and document exit codes.

doc/manual/source/man/dnst-keygen.rst Show resolved Hide resolved
doc/manual/source/man/dnst-notify.rst Outdated Show resolved Hide resolved
doc/manual/source/man/ldns-nsec3-hash.rst Outdated Show resolved Hide resolved
doc/manual/source/man/ldns-signzone.rst Outdated Show resolved Hide resolved
doc/manual/source/man/ldns-signzone.rst Outdated Show resolved Hide resolved
doc/manual/source/man/ldns-nsec3-hash.rst Outdated Show resolved Hide resolved
Synopsis
--------

:program:`dnst key2ds` [``OPTIONS``] ``<KEYFILE>``
Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout the docs it's at least inconsistent whether its [``OPTIONS``] or ``[OPTIONS]``.

In my opinion, it should be like in the uutils docs (which makes sense that I like it because I made it):

image

Just one big code block with all possible invocations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Throughout the docs it's at least inconsistent whether its [OPTIONS] or [OPTIONS].

I'll solve the inconsistencies.

The big code block with possible invocations sound nice, but might not be able to implement that today.

doc/manual/source/man/dnst-key2ds.rst Outdated Show resolved Hide resolved
doc/manual/source/man/ldns-keygen.rst Outdated Show resolved Hide resolved
doc/manual/source/man/dnst-signzone.rst Outdated Show resolved Hide resolved

.. option:: -o <domain>

Set the origin for the zone (for zonefiles with relative names and no
Copy link
Member

@ximon18 ximon18 Nov 15, 2024

Choose a reason for hiding this comment

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

This is correct but perhaps it would be better to say that this is only needed for zones that both contain relative labels AND lack an $ORIGIN directive. I think it also has to be an absolute/fully qualified domain name, not a relative domain name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It needs to be an absolute domain name. Also, signzone or rather domain's Name (::from_str?) turns any non-dot-terminated domain into an absolute/dot-terminated domain. So dnst signzone -o origin and dnst signzone -o origin. do the same.

@mozzieongit
Copy link
Member Author

Left over TODOs (though not merge-blocking I would say):

  • Add more comprehensive project description (like goals, ...)
  • Make ldns tools refer to dnst tools to motivate users to switch
  • Maybe add code blocks showing all possible invocations for a command (only for showing "permutations" with optional positional arguments)

Also I left some comments unresolved that could potentially turn into separate issues (for dnst sub commands or domain).

@tertsdiepraam
Copy link
Contributor

If this matches the current main then I'd say we should merge this so we can update it along with other changes.

@mozzieongit
Copy link
Member Author

Hm, the nsec3-hash default iterations for dnst is not yet merged into main. I'll change that to the current defaults in main, and the change would be done in #6 then, I suppose.

@tertsdiepraam
Copy link
Contributor

Alright I'll add it there!

@mozzieongit mozzieongit merged commit bfbf492 into main Nov 19, 2024
29 checks passed
@mozzieongit mozzieongit mentioned this pull request Nov 20, 2024
3 tasks
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