Conversation
|
@ArthurSens kindly review it. |
jan--f
left a comment
jan--f
left a comment
There was a problem hiding this comment.
Hi, thanks a lot!
I have two requests:
- Can you add a Makefile target that runs mdox and change the action to use that new target?
- I don't think we need to report uploaded anywhere. It would be better to fail the run when broken links are detected. We can then run the check locally and deal with broken links in a PR.
Signed-off-by: kairvee <145572028+kairveeehh@users.noreply.github.com>
|
hii @ArthurSens , @jan--f I have made the changes as requested and tested the CI manually on my forked repo too , it fails as expected. |
There was a problem hiding this comment.
Apologies for the delayed review here! I was traveling during the last week and couldn't keep up with GitHub notifications.
Just two comments, and it's good to merge in my opinion!
I can see some improvement opportunities, but let's tackle them in follow-up PRs and issues:
- The Makefile assumes whoever runs this has mdox installed. We can use some cool tricks to download mdox for the user automatically if it's not there
- Some links don't work locally, but work on the website... these warnings could be ignored.
| schedule: | ||
| - cron: '0 6 * * 1' # Mondays at 06:00 UTC |
There was a problem hiding this comment.
| schedule: | |
| - cron: '0 6 * * 1' # Mondays at 06:00 UTC |
As Jan mentioned, maybe let's remove the schedule since we don't know how often we can keep up with notifications. If we use manual triggers, at least we can control when we want to see the reports.
| .PHONY: mdox-check | ||
|
|
||
| mdox-check: | ||
| @mdox fmt --links.validate --check $$(find . -type d -name node_modules -prune -o -type d -name .next -prune -o -type d -name .git -prune -o -name '*.md' -print) |
There was a problem hiding this comment.
| .PHONY: mdox-check | |
| mdox-check: | |
| @mdox fmt --links.validate --check $$(find . -type d -name node_modules -prune -o -type d -name .next -prune -o -type d -name .git -prune -o -name '*.md' -print) | |
| .PHONY: mdox-check | |
| mdox-check: | |
| @mdox fmt --links.validate --check $$(find . -type d -name node_modules -prune -o -type d -name .next -prune -o -type d -name .git -prune -o -name '*.md' -print) |
This empty line isn't needed
scripts/remove-broken-links.sh
Outdated
| @@ -0,0 +1,25 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Could we remove this script? I'm not sure removing broken links is the way we wanna fix everything. I suspect we'll need to manually review everything and decide on a case-by-case approach, whether we want to remove or replace the link
No problem , it is completely ok :) did the suggested changes as well, can be reviewed for the same. |
|
I've created #2828 to discuss next steps |
3 participants
solves #2794
@ArthurSens , it checks and lets you have an artifact after it runs , scheduled weekly and I have tested manually as well.
There are certain cases where you might want to ignore the fact that the link is broken but those are very specific to be added inside the rules for the CI , kindly review and let me know if anything is to be modified.