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[pr-check-links]: dump links from baseline commit for pr link checking #180

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

f-hollow
Copy link
Collaborator

@f-hollow f-hollow commented Oct 31, 2024

Description

More recent updates in main

In the check_links_in_diffs workflow, the feature branch links are compared to the main branch links. As finalizing changes in a feature branch might take a while, its links get outdated compared to main. This can lead to false positives.

In the proposed updates, the feature branch links are compared to the main branch commit at which the feature branch was created. In this way, the more recent changes in main do not affect link checking.

Anchors / fragments affected by updates

All links are dumped into .lycheeignore from the main branch commit at which the feature branch was created. File names or headings to which the anchor / fragment point might get updated. Yet, checking of fragment links is bypassed. This PR adds steps to identify the files with updated file names and heading titles and removes all links in these files from .lycheeignore.

The downside is that if somebody alters a filename of a heading title, all anchor links in this file will be checked anew. On the other hand, if somebody touches an existing file, that would most likely be a maintainer or the author who be in the position to make sure that all links in these files are valid.

Cleaning up artifacts and any changes to files done in CI

Also, there were a couple of occasions when the check still failed after corrections were pushed. Creating a new PR with these changes had all checks passed successfully. A culprit might have been a cached file with old links that were not overridden (no other ideas for now). A step has been added to clean up all changes done in CI after the check completes.

Related

This updates the changes introduced in PR #107. The changes are tracked is issue #101.

Testing

Tests run:

  • Ensure the workflow does not check main in more recent commits compared to baseline commit
    • Injecting an invalid link in main in a more recent commit should not affect pr-check-links
  • Ensure that invalid links before the baseline commmit on main do not affect pr-check-links
    • Injecting an invalid link in main in the baseline commit should not affect pr-check-links
  • Ensure that anchors/fragments in a file pointing to heading titles of a file removed or modified in a PR get identified
    • Anchor/fragment links pointing to removed or modified files should not be present in .lycheeignore
  • Ensure that anchors/fragments in a file pointing to heading titles of a file renamed in a PR get identified
    • Anchor/fragment links pointing to renamed files should not be present in .lycheeignore

Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@f-hollow f-hollow force-pushed the ci/fix_check_links_in_pr branch 5 times, most recently from 8f39c75 to bf5b107 Compare October 31, 2024 12:09
@f-hollow f-hollow force-pushed the ci/fix_check_links_in_pr branch from bf5b107 to 917e7e2 Compare November 1, 2024 07:04
@f-hollow f-hollow changed the title fix: dump links from baseline commit for pr link checking ci[pr-check-links]: dump links from baseline commit for pr link checking Nov 1, 2024
Copy link
Member

@pedrominatel pedrominatel left a comment

Choose a reason for hiding this comment

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

LGTM

@f-hollow f-hollow force-pushed the ci/fix_check_links_in_pr branch 15 times, most recently from 9b77d73 to 0579990 Compare November 4, 2024 11:06
@f-hollow
Copy link
Collaborator Author

f-hollow commented Nov 4, 2024

@horw PTAL

@f-hollow f-hollow force-pushed the ci/fix_check_links_in_pr branch from 0579990 to 220772b Compare November 5, 2024 07:00
@f-hollow f-hollow force-pushed the ci/fix_check_links_in_pr branch from 220772b to d70afda Compare November 5, 2024 07:35
@pedrominatel pedrominatel merged commit 56d3737 into espressif:main Nov 5, 2024
3 checks passed
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