Skip to content

🌱 Add broken link check: pr and scheduled #966

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

peppi-lotta
Copy link
Member

@peppi-lotta peppi-lotta commented Feb 26, 2025

These workflows can be called from the other Metal3 repos to test for broken links.
.github/workflows/broken-link-check.yml is for testing all md files in a repo
.github/workflows/pr-link-check.yml is for testing all .md files that have changed in a PR. This does not check all files but only the ones that have changed.

I've tested the pr workflow in situations listed below and it is giving the output I expect

  • .md file is edited and doesn't have any link (pass)
  • .md file is edited and has working links (pass)
  • .md file is edited and has broken link (fail)
  • pr does not have any edited .md files (pass)

Fixes broken links.

@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tuminoid for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 26, 2025
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch from 89f2ba7 to a725c9c Compare February 26, 2025 11:08
@peppi-lotta peppi-lotta marked this pull request as draft February 26, 2025 11:24
@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2025
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Just quick comment about the filenames at this point.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch 2 times, most recently from ae6abc5 to fa0e146 Compare February 26, 2025 13:01
@peppi-lotta peppi-lotta marked this pull request as ready for review February 26, 2025 13:01
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2025
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch from fa0e146 to a6602f7 Compare February 26, 2025 13:02
@Rozzii
Copy link
Member

Rozzii commented Mar 3, 2025

/override metal3-ubuntu-e2e-integration-test-main

@metal3-io-bot
Copy link
Collaborator

@Rozzii: Overrode contexts on behalf of Rozzii: metal3-ubuntu-e2e-integration-test-main

In response to this:

/override metal3-ubuntu-e2e-integration-test-main

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2025
@tuminoid
Copy link
Member

tuminoid commented Mar 3, 2025

/hold

Since we're adding only the callable workflows here, we have no test to see if they work and multi-repo actions are painful to test. Should we have link checking enabled in project-infra (added in this same PR), which verified they work?

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2025
@peppi-lotta
Copy link
Member Author

/hold

Since we're adding only the callable workflows here, we have no test to see if they work and multi-repo actions are painful to test. Should we have link checking enabled in project-infra (added in this same PR), which verified they work?

I have created two repos for myself to test the PR link check workflow.
Repo containing workflows: https://github.com/peppi-lotta/workflow-test (should have pretty much the same link check workflows as I've added here)
Repo where work flows have been called: https://github.com/peppi-lotta/test-pr (This project has open PRs where the link check has been ran)

This is what I was using to test that workflows are working when I was writhing them. Can these be used as proof that these workflows are working? Other wise I can add changes to enable workflows here.

@tuminoid
Copy link
Member

tuminoid commented Mar 3, 2025

/hold
Since we're adding only the callable workflows here, we have no test to see if they work and multi-repo actions are painful to test. Should we have link checking enabled in project-infra (added in this same PR), which verified they work?

I have created two repos for myself to test the PR link check workflow. Repo containing workflows: https://github.com/peppi-lotta/workflow-test (should have pretty much the same link check workflows as I've added here) Repo where work flows have been called: https://github.com/peppi-lotta/test-pr (This project has open PRs where the link check has been ran)

This is what I was using to test that workflows are working when I was writhing them. Can these be used as proof that these workflows are working? Other wise I can add changes to enable workflows here.

It is good that you have working setup to test this. I think we would like to have links checked here as well, so IMO we could add it here too.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch from a6602f7 to 6392ee8 Compare March 6, 2025 07:19
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2025
@metal3-io-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2025
@peppi-lotta peppi-lotta changed the title 🌱 Add broken link check: general and pr 🌱 Add broken link check: pr and scheduled Mar 6, 2025
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch from f21878a to a3528bc Compare March 7, 2025 07:35
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch from a3528bc to 6c82a78 Compare March 7, 2025 07:38
@tuminoid
Copy link
Member

tuminoid commented Mar 7, 2025

I'll review properly next week, but initially it looks as agreed and clearly finds some issues in the repo :)

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch 5 times, most recently from 12c2a77 to 0e35aca Compare April 29, 2025 09:20
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Minor nits.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch from 0e35aca to ce80447 Compare May 5, 2025 10:37
@peppi-lotta
Copy link
Member Author

/retest

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch 4 times, most recently from f65b5fd to eb3584d Compare May 5, 2025 12:00
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/add-link-check-work-flows branch from eb3584d to 33dcca0 Compare May 5, 2025 12:07
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Some further nits, but this is otherwise looking good now.

run: |
git diff --name-only origin/${{ github.event.pull_request.base.ref }}...${{ github.head_ref }} -- '*.md' > changed-files.txt
cat changed-files.txt
if [ -s changed-files.txt ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [ -s changed-files.txt ]; then
if [[ -s "changed-files.txt" ]]; then

- name: Get list of changed Markdown files
id: changed-files
run: |
git diff --name-only origin/${{ github.event.pull_request.base.ref }}...${{ github.head_ref }} -- '*.md' > changed-files.txt
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
git diff --name-only origin/${{ github.event.pull_request.base.ref }}...${{ github.head_ref }} -- '*.md' > changed-files.txt
git diff --name-only "origin/${{ github.event.pull_request.base.ref }}...${{ github.head_ref }}" -- "*.md" > changed-files.txt

cat changed-files.txt
if [ -s changed-files.txt ]; then
echo "Changed md files found"
echo "foundFiles=true" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo "foundFiles=true" >> $GITHUB_ENV
echo "foundFiles=true" >> "${GITHUB_ENV}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants