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

167 add support to link check #173

Merged
merged 3 commits into from
Jun 27, 2023

Conversation

jenkin
Copy link
Contributor

@jenkin jenkin commented Jun 22, 2023

Using markdown-link-validator from github repo.

Build the docker image and run validation with make check.

WARNING: this PR makes #171 deprecated.

@jenkin jenkin requested a review from stickgrinder June 22, 2023 07:55
@jenkin jenkin mentioned this pull request Jun 22, 2023
@jenkin jenkin changed the title refs #167: add support to link check 167 add support to link check Jun 22, 2023
@paolomainardi
Copy link
Member

paolomainardi commented Jun 22, 2023

I am wondering if we should add this to a github actions as well

Like this one: https://github.com/marketplace/actions/markdown-link-check cc @jenkin

@stickgrinder
Copy link
Member

I was asking for a pre-commit hook in the meantime, so no broken links are committed to the repo.

@jenkin
Copy link
Contributor Author

jenkin commented Jun 22, 2023

For pre-commit hooks or github actions, be aware of links to not (yet) existing pages. We have to create them as empty files?

Moreover, some remote links always fail because of bot protection systems (ie. drupal.org), so we need to actively add them to exclusion list (-i options)?

Example

markdown-link-validator -q -e -o -i ^/downloads/ -i drupal.org -i udemy.com -i sparkfabrik.loc -i acme.sparkfabrik.com ./content

If links validation is mandatory for commit or delivery, a clear policy for these cases should be enforced and communicated to developers, imho.

Using [markdown-link-validator](https://github.com/jenkin/markdown-link-validator/tree/feature/sparkfabrik-enhancements) from github repo.

Build the docker image and run validation with `make check`.
@jenkin jenkin force-pushed the 167-markdown-link-validator-enhanced branch from 155c657 to 4129c4d Compare June 26, 2023 08:58
"raneto": "0.17.3",
"gulp": "4.0.2"
"gulp": "4.0.2",
"markdown-link-validator": "github:jenkin/markdown-link-validator#feature/sparkfabrik-enhancements",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

come mai stiamo usando un fork ? devo aver perso qualche passaggio

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paolomainardi Ho provato alcune soluzioni già pronte (come il markdown-link-check che hai menzionato anche tu o l'originale markdown-link-validator), ma nessuna soddisfa pienamente i nostri requisiti. Vedi #167 (in particolare la sezione drawbacks).

Alla fine ho optato per forkare markdown-link-validator e aggiungere le funzionalità che ci servono.

Tutte le modifiche fatte sono retrocompatibili per quanto riguarda le API della CLI e a mio avviso di interesse abbastanza generale da poter essere anche rilasciate (ho anche corretto alcune issue ancora aperte). Ma prima di proporre eventuali PR al repo originale aspettavo di integrare il tutto nel playbook e fare più test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok 👍 grazie!

@jenkin jenkin force-pushed the 167-markdown-link-validator-enhanced branch from 89ed165 to ce4bb49 Compare June 26, 2023 12:46
@jenkin
Copy link
Contributor Author

jenkin commented Jun 26, 2023

@stickgrinder Ho aggiunto il check dei link al pre-commit con husky. Per fare il commit ho dovuto creare i tre file markdown mancanti (per ora vuoti) menzionati in /organization/administration.md:

  • /organization/role-isc-supporter.md:8:145
  • /organization/role-isc-lead-developer.md:15:46
  • /organization/role-isc-professional.md:15:105

@stickgrinder
Copy link
Member

This seems ok to me, the bulk of the logic here lies in the markdown link checker, so let's merge this and do some real-world testing.

Thanks @jenkin

@stickgrinder stickgrinder merged commit aa1d280 into master Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants