Feature #3247 link check - #3325
Conversation
…o dtcenter/metplus-action-linkcheck
Coverage Report for CI Build 29365648870Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.01%) to 92.054%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…d ModuleNotFoundError during linkcheck
There was a problem hiding this comment.
@jprestop this is great! Thanks for making progress on this. Here's some feedback:
-
metplus-action-linkcheck repository. I compared to other
metplus-action-*repositories and note:
- Why is Python 3.11 the default on this line? I would have expected 3.12 or 3.14. Not a big deal. Just curious why 3.11?
- I'd say that thefail-on-broken-links,upload-artifact, andartifact-nameoptions are probably overkill and may not actually NEED to be configurable. But having them configurable doesn't hurt anything.
- Recommend improving the README.md with a level of detail similar to metplus-action-release-checksum or metplus-action-scan-docker-images. -
Reviewing the run of this action for this PR:
- Note the warning about Node.js 20. In the link checker action just update to
setup-python@v6andupload-artifact@v7. - I reviewed some of the 203 links reported as being broken. I agree that we don't have to fix them immediately, but they definitely inform how the link checker action should be configured for each repo.
- 3 are just examples that include
vX.Y.Zormain_vX.Yand will always be "broken". - 56 are "internal" relative links that include
../that require more investigation. I think RTD is already checking for broken internal links. So we prob just want to ignore those. - 101 are redirects that we could tighten up.
- 15 are "anchor links" that include a # which the link checker does not seem to like!
- 7 are links to MET GitHub project pages but no idea why those are flagged.
- 21 might just be legit broken links that should be updated.
- 3 are just examples that include
- I'd recommend renaming
output.txttolinkcheck-output.txt. Since it gets unzipped directly in the download directory, and it's easy to loseoutput.txtin there. - I note that the artifact actually includes 4 files (see below). Perhaps the
_sphinx_design_staticdirectory should be omitted?:
inflating: _sphinx_design_static/design-tabs.js
inflating: _sphinx_design_static/sphinx-design.min.css
inflating: output.json
inflating: output.txt
- I reviewed the doc updates and added a handful of inline recommendations.
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
…of other METplus actions per reviewer suggestion.
|
Thanks for the thorough review and the feedback, @JohnHalleyGotway.
I'll request a re-review now, but please feel free to follow up with any other suggestions/comments. |
JohnHalleyGotway
left a comment
There was a problem hiding this comment.
@jprestop this looks great to me. Thanks for making all the updates you described in this PR comment. I reviewed the changes for this PR and your recent updates in dtcenter/metplus-action-linkcheck. I proposed two very minor formatting changes.
I approve of these changes, whether you accept those mods or not.
Thanks for the additional catches @JohnHalleyGotway. I made the changes to remove the extra whitespace and additional dash. Thanks for the approval. I'll get this merged and will submit the other PRs. |
This PR is the same as this PR in the METplus repository, [Feature #3247 link check #3325](dtcenter/METplus#3325), minus the Contributor's Guide documentation. See the METplus PR for further details. ## Pull Request Checklist ## See the [METplus Workflow](https://metplus.readthedocs.io/en/latest/Contributors_Guide/github_workflow.html) for details. - [ ] Review the source issue metadata (required labels, projects, and milestone). - [ ] Complete the PR definition above. - [ ] Ensure the PR title matches the feature or bugfix branch name. - [ ] Define the PR metadata, as permissions allow. Select: **Reviewer(s)** and **Development** issue Select: **Milestone** as the version that will include these changes Select: **Coordinated METplus-X.Y Support** project for bugfix releases or **METexpress-X.Y.Z Development** project for official releases - [ ] After submitting the PR, select the ⚙️ icon in the **Development** section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected. - [ ] After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge. - [ ] Close the linked issue and delete your feature or bugfix branch from GitHub.
Added automated link checking for METplus component documentation using Sphinx's built-in
linkcheckbuilder, run via a new custom composite GitHub Action rather than duplicating workflow logic across each component repo.Where linkcheck runs:
Linkcheck runs via GitHub Actions in each component repo, using a new workflow file,
linkcheck.yml, which runs weekly (Monday 06:00 UTC via cron), fails the job on broken links so failures surface via GitHub's workflow notifications and runs on PRs touchingdocs/**, fails the job on broken links so new/edited links are checked before merge.New shared component:
sphinx-build -b linkcheck, uploads the report as a workflow artifact, and fails/passes based on afail-on-broken-linksinput (default true).metplus-action-*repos (release-checksum, trigger-use-cases, data-update, etc.) in that they are Docker-based custom actions which predate composite actions being available on GitHub. Composite was the right choice here as the linkcheck action only needs Python, pip, andsphinx-build, all things thatactions/setup-pythonand a shellrun:step handle natively. A Docker-based approach would add image build/pull overhead and Linux-only constraints for no functional benefit, since there's no dependency here that needs container isolation.Per-repo configuration still required:
docs/conf.pyneeds alinkcheckconfiguration block (linkcheck_timeout,linkcheck_retries,linkcheck_workers,linkcheck_ignore, etc.). The contents oflinkcheck_ignoreare repo-specific, since each component links to different external resources, so this can't be centralized in the shared action and should be populated based on the firstlinkcheckrun's actual output.docs/requirements.txtin any repository.linkcheckis part of core Sphinx, and the existing dependency list already covers everything needed to parse the doc tree.Pull Request Testing
Describe testing already performed for these changes:
Ran
sphinx-build -b linkchecklocally against the updateddocs/conf.py. The newlinkcheck.ymlworkflow (via thepull_requesttrigger) will run automatically once this PR is opened, sincepull_request-triggered workflows execute from the PR branch rather than requiring the workflow to already exist on the default branch. This will serve as the first end-to-end test of the composite action against real repo content. The composite action itself (dtcenter/metplus-action-linkcheck) has not yet been tested independently prior to this PR.workflow_dispatchmanual triggering is not testable pre-merge.Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
linkcheckworkflow run (triggered automatically viapull_request) and confirm it completed (either passing, or failing with a legitimate broken-link report rather than an action/configuration error). Note that the linkcheck workflow failed at the step "Run dtcenter/metplus-action-linkcheck@v1" with a legitimate broken-link report.[linkcheck-output](https://github.com/dtcenter/METplus/actions/runs/28908061175/artifacts/8155006783)artifact from that run and review output.txt to confirm the results look reasonable. I assumed we'd have a separate issue to go through each repository's broken links and clean them up and/or add them tolinkcheck-ignore).continuous_integration.rst.linkcheck.ymlfor the trigger conditions (schedule, pull_request with paths: ['docs/**'], workflow_dispatch).NOTE/QUESTION:
workflow_dispatch(the manual trigger) won't be available to test until after this PR merges to the default branch (Do we want this added to the main_vX.Y branches also, or do we want to wait until for the METplus-13.0.0 coordinated release?)Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Do these changes include sufficient testing updates? [Yes]
Will this PR result in changes to the test suite? [No]
If yes, describe the new output and/or changes to the existing output:
Do these changes introduce new SonarQube findings? [No]
If yes, please describe:
Please complete this pull request review by [20260722].
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: Coordinated METplus-X.Y Support project for bugfix releases or METplus-Wrappers-X.Y.Z Development project for official releases