-
Notifications
You must be signed in to change notification settings - Fork 30
Add authctl docs #1179
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
base: main
Are you sure you want to change the base?
Add authctl docs #1179
Conversation
|
@edibotopic do you understand why the build of the docs fails in the CI but not when I build it locally via In the CI I these warnings which I assume are the reason the build fails: I don't see those locally. |
FTR, we were able to resolve that - @edibotopic figured out how to ignore these warnings |
Thanks @adombeck . I think it's OK to ignore these warnings for now so that we are better able to support users with these docs. In the long run, we should fix the header levels so that they conform with the rest of the docs. In future, we can work on improving the doc generation and formatting of the rendered output, depending on capacity, etc. |
|
@edibotopic I think this is now ready for an initial review. Please ignore all the commits which are not touching the documentation, those are from #1087. The documentation is very rudimentary for now, we might want to write a more extensive overview of |
For later reference, I pushed a WIP commit which uses gencodo to https://github.com/adombeck/authd/tree/authctl-docs-gencodo |
edibotopic
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.
Excellent work @adombeck .
I've left a few minor suggestions and comments.
Some of the more elaborate synopses tend to push the basic usage example (Use) quite far down, and visually pair it with a separate command, example:
I think that's probably just a limitation of the native Cobra docgen, but it's something we can look at in future.
| Files outside the user's home directory are not updated and must be changed | ||
| manually. Note that changing a UID can be unsafe if files on the system are | ||
| still owned by the original UID: those files may become accessible to a different | ||
| account that is later assigned that UID. To change ownership of all files on the | ||
| system from the old UID to the new UID, run: |
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.
| Files outside the user's home directory are not updated and must be changed | |
| manually. Note that changing a UID can be unsafe if files on the system are | |
| still owned by the original UID: those files may become accessible to a different | |
| account that is later assigned that UID. To change ownership of all files on the | |
| system from the old UID to the new UID, run: | |
| Files outside the user's home directory are not updated and must be changed | |
| manually. | |
| Note that changing a UID can be unsafe if files on the system are | |
| still owned by the original UID: those files may become accessible to a different | |
| account that is later assigned that UID. To change ownership of all files on the | |
| system from the old UID to the new UID, run: |
I think these are separate paragraphs.
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 see that starting the sentence with "Note ..." makes it feel like a new paragraph, but conceptually it's about the exact same thing: ownership of files outside the user's home directory must be updated manually
Maybe we could rephrase things to make better fit the sentences into the same paragraph?
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 it just needs something like this to provide a link between the first and second sentences:
Note that changing a UID manually can be unsafe if files on the system are
still owned by the original UID
Note that changing a UID in this way can be unsafe if files on the system are
still owned by the original UID
What do you think?
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, that's not the intended meaning. It's not the "manual change [of file ownership]" in the first sentence which can be unsafe. It's the change of the UID, i.e. the authctl user set-uid command which can be unsafe if there are files on the system still owned by that UID. So what we want to say is "make sure that you do update file ownerships manually for files outside the home directory". Does that make it clearer? If not, I can try to explain the issue in more detail.
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, indeed, that makes it more clear.
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.
How do we best rephrase this sentence to also make it clear to the user?
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 we should leave it as the original. The misinterpretation was my own and I understand that after your clarification.
If this was a regular doc, I would probably put the note in its own block so that the first and third sentences flow together more clearly but we can revisit that another time.
| Files outside users' home directories are not updated and must be changed | ||
| manually. Note that changing a GID can be unsafe if files on the system are | ||
| still owned by the original GID: those files may become accessible to a | ||
| different group that is later assigned that GID. To change group ownership of | ||
| all files on the system from the old GID to the new GID, run: |
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.
| Files outside users' home directories are not updated and must be changed | |
| manually. Note that changing a GID can be unsafe if files on the system are | |
| still owned by the original GID: those files may become accessible to a | |
| different group that is later assigned that GID. To change group ownership of | |
| all files on the system from the old GID to the new GID, run: | |
| Files outside users' home directories are not updated and must be changed | |
| manually. | |
| Note that changing a GID can be unsafe if files on the system are | |
| still owned by the original GID: those files may become accessible to a | |
| different group that is later assigned that GID. To change group ownership of | |
| all files on the system from the old GID to the new GID, run: |
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.
Can reconsider based on discussion of similar case above.
Yeah, that also annoyed me. I think the description just doesn't belong in the "Synopsis" section. My understanding of a "Synopsis" is that it's only the formal description of how to run the command what command line options it takes (like This is an issue which we could fix with gencodo. |
Thanks for the context @adombeck . I agree, let's wait for gencodo. It's OK for now. |
There's nothing encrypted in this string.
... instead of prefixing the error message with "permission denied"
Required to change the ownership of the user's home directory when changing the user's UID.
We use this to recursively change the owner and group of the user's home directory when changing the user's UID.
Needed to test fileutils.ChownRecursiveFrom. We can't use bubblewrap for
that because bubblewrap only creates UID mapping for one user, using
chown with a different UID fails with:
chown: changing ownership of 'file': Invalid argument
Do the same usermod does when changing a UID of a user: If the home directory is currently owned by the user, recursively change the owner and group of the home directory and all files in the home directory from the old UID and GID to the new UID and GID.
We need that for the SetUserID tests
We now support chown in bubblewrap, so we don't have to run the test as root anymore.
When executing `unshare --map-user` via exec.Command and connecting the process's stdout or stderr, the command hangs forever if unprivileged user namespaces are disabled. We avoid that by checking via `unshare --user` if unprivileged user namespaces are enabled.
The "Run autopkgtests" CI job runs the tests in an LXD container which
doesn't allow using bubblewrap. It fails with:
bwrap: Failed to make / slave: Permission denied
To avoid that these jobs fail, we allow them to skip the bubblewrap
tests. We still run the tests in the "Go Tests" CI jobs.
Running our tests with -v produces so much output that it makes it harder to inspect test failures, for example when viewing the logs of the "Run autopkgtests" CI job in GitHub. Running the tests without -v still prints the logs of the failed tests which should include all the information we need to debug test failures.
As suggested by reviewer. It's not implemented for now, warnings are always returned in English.
Produces prettier docs
So that it can be imported by the docgen tool we're about to add.
Indenting with four spaces formats the command as a code block in the generated markdown document.
Having the "This command requires root privileges." sentence after the `chown -R` command was confusing, it sounded like it was referring to that command instead of the authctl command.
To generate the CLI docs
The auto-generated Cobra markdown docs use headings starting at H2, not H1. That results in Sphinx printing warnings, which causes the build to fail in the CI (but not locally). Let's ignore these kind of warnings for now.
Co-authored-by: Shane Crowley <[email protected]>
Co-authored-by: Shane Crowley <[email protected]>
Thanks to Shane for spotting that.
Via `go generate ./docs/...`
e52807e to
62920d5
Compare
edibotopic
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.
Great work! Nice to have this in the docs.
Important
This is based on #1087
Generate authctl docs via cobra.
UDENG-8760