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

ci: update & simplify docs contributing - docs: update + cosmetics #6468

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

docgalaxyblock
Copy link
Contributor

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

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]>
@docgalaxyblock docgalaxyblock changed the title ci: update & simplify contributing - docs: update + cosmetics ci: update & simplify docs contributing - docs: update + cosmetics Jan 25, 2024
@docgalaxyblock
Copy link
Contributor Author

I did not touched the lib/canboot folder. May that be renamed too and a symlink added for legacy docs forks?

@Arksine The lib/README lists a rev I could not find in your katapult repo - how old is the flash_can.py in klipper?
Are there be any changes making an update a benefit for the user?

Copy link

github-actions bot commented Feb 9, 2024

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:

  1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
    If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
  2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
  3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

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,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@KevinOConnor
Copy link
Collaborator

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.
-Kevin

Comment on lines +21 to +22
permissions:
contents: write
Copy link
Collaborator

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).

Copy link
Contributor Author

@docgalaxyblock docgalaxyblock Feb 17, 2024

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 ❌

Comment on lines +115 to +121
---

## 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"
Copy link
Collaborator

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. :-)

Copy link
Contributor Author

@docgalaxyblock docgalaxyblock Feb 17, 2024

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.

```
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
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Comment on lines -556 to +562
sudo apt-get update
sudo apt-get install autoconf libtool telnet
sudo apt update
sudo apt install autoconf libtool telnet
Copy link
Collaborator

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.

Copy link
Contributor Author

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
Copy link
Collaborator

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.

Copy link
Contributor Author

@docgalaxyblock docgalaxyblock Feb 17, 2024

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 Show resolved Hide resolved
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: \
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@docgalaxyblock docgalaxyblock Feb 19, 2024

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

Copy link
Contributor Author

@docgalaxyblock docgalaxyblock Mar 6, 2024

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.

docs/SDCard_Updates.md Outdated Show resolved Hide resolved
docs/FAQ.md Outdated Show resolved Hide resolved
@KevinOConnor KevinOConnor added the pending feedback Topic is pending feedback from submitter label Feb 16, 2024
@github-actions github-actions bot added the inactive Not currently being worked on label Mar 28, 2024
Copy link

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,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@github-actions github-actions bot closed this Mar 28, 2024
@KevinOConnor KevinOConnor removed inactive Not currently being worked on pending feedback Topic is pending feedback from submitter labels Mar 28, 2024
@KevinOConnor KevinOConnor reopened this Mar 28, 2024
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]>
@docgalaxyblock
Copy link
Contributor Author

I hope I have got everything now.

Please give your updated opinion on the still open review comments.

Happy 🐇🥚
Doc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants