-
-
Notifications
You must be signed in to change notification settings - Fork 5.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
ci: update & simplify docs contributing - docs: update + cosmetics #6468
base: master
Are you sure you want to change the base?
Conversation
I only touched the actions I am familiar with - so: checkout, cache, setup-python and upload-artefact. I did not touched stale, github-script, dessant/lock-threads and JamesIves/github-pages-deploy-action Node.js 16 actions are deprecated. For more information see: https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/. Signed-off-by: Joshua Schlicker <[email protected]>
Simplifies contributing to the klipper docs. You fork the klipper repo, enable actions in your repo settings, commit your changes, select the "klipper3d deploy" workflow in the actions tap, click on "Run workflow" and set the "Source branch for page deployment" to the branch you commit your changes to if you do not commit to your master branch Signed-off-by: Joshua Schlicker <[email protected]>
https://squidfunk.github.io/mkdocs-material/reference/admonitions Signed-off-by: Joshua Schlicker <[email protected]>
I did not touched the @Arksine The |
Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html There are some steps that you can take now:
Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available. Best regards, PS: I'm just an automated script, not a human being. |
Thanks. I appreciate you taking the time to review and make fixes to the documentation. As high-level feedback, there are a lot of changes in this PR and a handful of changes look incorrect to me. That makes it hard to review and provide feedback. I appreciate that you've split this PR into commits (as that helps review). I did see some commits where cosmetic changes (eg, whitespace fixes) were mixed with functional changes (eg, a different path) - that makes it hard to review. I'll follow up with a few things that I noticed during review. Thanks again for working on this. |
permissions: | ||
contents: write |
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 leery of making a permission change like this. My fear is that one of the many build scripts could alter the master branch (which would then propagate to thousands of printers to within a few days). Not sure the trade-off on this commit is worthwhile - might be better to just add documentation on how one can build the documentation (I use a docker instance, for example).
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.
Ok but your current setup is less secure.
Without write permissions all current workflows in this repo should fail. Therefore I guess at the repo or organization level the default github workflow permission setting is set to "read & write".
https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token
If a threaded harmfull gets merged now it has write permissions to everything.
If your current default token permissions would be set to read only the pages deployment action would error like this:
Error: The deploy step encountered an error: The process '/usr/bin/git' failed with exit code 128 ❌
--- | ||
|
||
## Notes | ||
|
||
### STM32 DFU Warning | ||
|
||
Note that on some boards, like the Octopus Pro v1, entering DFU mode can cause | ||
undesired actions (such as powering the heater while in DFU mode). It is | ||
recommended to disconnect heaters, and otherwise prevent undesired operations | ||
when using DFU mode. Consult the documentation for your board for more details. | ||
!!! danger "STM32 DFU Warning" |
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.
Heh, a line, a header, another header, and a danger.. I think we may be overdoing this a bit. :-)
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.
Year but otherwise without the heading it does not get listed at the TOC.
And iirc the current used mkdocs-material version does not support admonitions without title.
docs/Bootloaders.md
Outdated
``` | ||
This will return UUIDs for all connected nodes not currently assigned a UUID. | ||
This should include all nodes currently in the bootloader. | ||
## STM32F4 micro-controllers with DFU bootloader |
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.
This section discusses hidflash - so changing this to say "DFU bootloader" is incorrect.
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 removed the bootloader in the header
sudo apt-get update | ||
sudo apt-get install autoconf libtool telnet | ||
sudo apt update | ||
sudo apt install autoconf libtool telnet |
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 sure about the implications of making this change. As far as I know, apt-get
is always installed, but I'm not sure apt
is always 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.
I have checked apt
and apt-get
on my pi and both are included in the apt
package.
I have also searched apt at the other docs and it is a mix of both again.
Afaik apt is the more interactive version of both.
Should I make this consistent across all pages?
/dev/serial/by-id/*` from an ssh terminal on the host machine. It will | ||
likely produce output similar to the following: | ||
The general way to find a USB serial port is to run | ||
``` shell |
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.
It seems odd to place a space between the backticks and the designator - all the other places in the docs do not place a space there.
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 am following the mkdos-material guide here.
docs/FAQ.md
Outdated
a higher speed. So, for a Z axis with a high gearing ratio or high | ||
microsteps setting the actual obtainable max_z_velocity may be smaller | ||
than what is configured in Marlin. | ||
Short answer: \ |
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 don't think it is a good idea to end lines with a backslash - it isn't portable markdown and it looks confusing to users reading the text directly.
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.
Year but a backslash or two whitespaces are the only methods I know of making a linebreak without a heel breaking (if this is the right word)
Eg: Return vs Shift + Return
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.
Ok a backslash is not the best practise but the spec two trailing whitespaces are not allowed by the check_whitespace script.
So what to do here?
Source:
https://spec.commonmark.org/0.31.2/#hard-line-break
https://www.markdownguide.org/basic-syntax/#line-breaks
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 have replaced " \"
by <br>
as the first source suggests it.
It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket. Best regards, PS: I'm just an automated script, not a human being. |
Signed-off-by: Joshua Schlicker <[email protected]>
Signed-off-by: Joshua Schlicker <[email protected]>
Signed-off-by: Joshua Schlicker <[email protected]>
- fix some typos and formating stuff - flash_can.py uses as the only command absolute paths, switch to relative paths for klippers docs. - use apt instead of apt-get for interactive instructions Signed-off-by: Joshua Schlicker <[email protected]>
Signed-off-by: Joshua Schlicker <[email protected]>
I hope I have got everything now. Please give your updated opinion on the still open review comments. Happy 🐇🥚 |
My start focus was to update all sites regarding the CanBoot -> katapult transition.
Stuff I catched on the lines/pages I fixed too.
Changed site can be viewed here: https://docgalaxyblock.github.io/klipper
Note: It looks like some cometic customization stuff got not applied
I stopped my work today since the used mkdocs-material version is not uptodate and the official mkdocs-material docs differ a bit.
Will try to update mkdocs-material without breaking anything and then update more pages.
Greetings
Doc