-
Notifications
You must be signed in to change notification settings - Fork 451
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
chore: added hack/verify-reports.sh script #3167
Conversation
Skipping CI for Draft Pull Request. |
/test pull-gateway-api-verify |
/test pull-gateway-api-verify |
982dcaa
to
5bbaf47
Compare
2a10efc
to
b64f6cc
Compare
The CI job is intended to fail until #3168 does not get merged |
That PR merged, this one may also need a rebase, we'll see. /retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mlavacca!
# Check if the README.md has broken links | ||
docker run -v $(readlink -f "$implementation_dir"):/${implementation}:ro --rm -i ghcr.io/tcort/markdown-link-check:stable /${implementation}/README.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this utility work? Can/should we use this for all of our docs? I think some of our links are only valid with mkdocs-material context, would that be able to understand this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It checks that all the links (internal and external) in a .md
file are not broken. for what concerns mkdocs-material
, all the internal links should already be checked when building the documentation. For what concerns the internal links of the documentation generated by mkdocs
, I checked and this tool cannot be used, because as you pointed out, it is not able to properly resolve them. We could use such a tool for all the other .md
files.
I've tried to run find . -path ./site-src -prune -o -name '*.md' -print0 | xargs -0 -n1 markdown-link-check -q
(checks all the .md
files but the ones under ./site-src
) and many links are broken. I think this is out of scope for this PR, would you agree with me opening an issue to improve the verify.sh
script to fix them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the internal links should already be checked when building the documentation
Not that I know of?
I think this is out of scope for this PR, would you agree with me opening an issue to improve the verify.sh script to fix them?
Agree it's out of scope for this PR, but a follow up would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I've created #3258 for this. For what concerns internal links with mkdocs, I asked confirmation directly to the community: https://matrix.to/#/!agmMfyRRWLhOnjmeFl:gitter.im/$sc_nZQIzGCVAu3hLKj0hzLLCeroYTEbn2Wfc12kUfGU?via=gitter.im&via=matrix.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mlavacca!
# Check if the README.md has broken links | ||
docker run -v $(readlink -f "$implementation_dir"):/${implementation}:ro --rm -i ghcr.io/tcort/markdown-link-check:stable /${implementation}/README.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the internal links should already be checked when building the documentation
Not that I know of?
I think this is out of scope for this PR, would you agree with me opening an issue to improve the verify.sh script to fix them?
Agree it's out of scope for this PR, but a follow up would be great.
@@ -0,0 +1,109 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be run automatically as a presubmit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's intended to be run as part of the make verify
target. Some magic hack in verify-all.sh
runs all the scripts matching the pattern verify-*.sh
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
b64f6cc
to
cfd2bd0
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mlavacca The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@robscott, have you got any more thoughts here? What's our path forward for this one? |
Thanks for the ping on this @youngnick and for all the work @mlavacca! I think this is good to go. /lgtm |
|
||
if [[ -f "${implementation_dir}/README.md" ]]; then | ||
# Check if the README.md has broken links | ||
docker run -v $(readlink -f "$implementation_dir"):/${implementation}:ro --rm -i ghcr.io/tcort/markdown-link-check:stable /${implementation}/README.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So installing docker
has become a dev prerequisite of this project? Otherwise we can't manfully run make verify
on my local machine before submitting a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was already a prerequisite, as other verify-*.sh
scripts make use of it (https://github.com/kubernetes-sigs/gateway-api/tree/main/hack/verify-docker-build.sh#L25-L26, https://github.com/kubernetes-sigs/gateway-api/blob/main/hack/verify-golint.sh#L33-L41). That said, I 100% agree we should clearly state it in the verify section of our docs. Given the PR you already raised on that side, would you be interested in updating such a section as well with all the deps? That would be great and much appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was already a prerequisite, as other
verify-*.sh
scripts make use of it (https://github.com/kubernetes-sigs/gateway-api/tree/main/hack/verify-docker-build.sh#L25-L26, https://github.com/kubernetes-sigs/gateway-api/blob/main/hack/verify-golint.sh#L33-L41). That said, I 100% agree we should clearly state it in the verify section of our docs. Given the PR you already raised on that side, would you be interested in updating such a section as well with all the deps? That would be great and much appreciated.
Unfortunately my PR has just been merged, so I can't just change it in my old PR. But I have created a new one (#3267 ). I added background and steps on how to update the doc. It's should be pretty easy and straightforward to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating such an issue, @jgao1025!
What type of PR is this?
/area conformance
What this PR does / why we need it:
Recently, many submitted reports were not compliant with the rules stated in this document. This PR aims to create a script to automatically check the correctness of the uploaded reports and the structure. The following are the checks performed:
<experimental | standard>-<semver>-<mode>-report.yaml
the projectversion
is a valid semverGatewayAPIVersion
field matches the folder in which the report has been createdGatewayAPIChannel
is either "standard" or "experimental"Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: