-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
genfstab: add page #5215
genfstab: add page #5215
Conversation
The build for this PR failed with the following error(s):
Please fix the error(s) and push again. |
The build for this PR failed with the following error(s):
Please fix the error(s) and push again. |
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.
add maintainer suggestions
EDIT: do not know what to do, still reluctantly learning Github
EDIT2: somehow added maintainer suggestions into "batch" and committed them, hopefully this was expected
Co-authored-by: Axel Navarro <navarroaxel@gmail.com>
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.
Hi @peterbabic, thank you for the new page! I've left some comments below for you to review.
Unfortunately I do not agree wit any of the changes @bl-ue suggested, not meaning to be rude by any way. I have provided explanations to all of them. |
@peterbabic no problem! Sometimes we have difficulty communicating through a purely textual medium :) I've responded to your comments. |
Co-authored-by: bl-ue <54780737+bl-ue@users.noreply.github.com>
@bl-ue no poblem as well! Thanks for suggestions. Did not know about the nature of "an ef-es-tab file" 😄 |
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.
Okay, thanks for the help @navarroaxel. @peterbabic I have only two more comments.
I have added another commit where I believe the changes are the most consistent. I have also removed backticks around |
@peterbabic - Looks good to me, except the sentence "A usual way to generate an fstab file, requires root permissions:" is a little inconsistent with the rest of our pages. If that is the most common example, then I think you should:
|
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.
Looks pretty much good to go to me - I think we can probably merge this.
I've left a comment on the requires root permissions
issue, but that doesn't necessarily need to block the merge of this PR - and I can see some significant discussion has already happened - so I'm leaning on the side of getting this merged sooner rather than later.
@sbrl that was actually already resolved :) - #5215 (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.
Thank you @peterbabic, and welcome to tldr-pages!
Ok, whatever was done there is fine then :-) |
We picked not to use |
common/
,linux/
, etc.)