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

chore: added hack/verify-reports.sh script #3167

Merged
merged 2 commits into from
Aug 9, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions hack/verify-reports.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
#!/bin/bash
Copy link
Member

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?

Copy link
Member Author

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


# Copyright 2014 The Kubernetes Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

error() {
echo "ERROR: $*" 1>&2
}

info() {
echo "INFO: $*" 1>&2
}

# Check if the provided Gateway API version is greater than or equal to v1.1.0
check_ge_v1.1.0() {
local version=$1
local minimum_version="v1.1.0"

# Normalize versions to remove leading 'v' and compare using sort -V
if [[ $(echo -e "${version#v}\n${minimum_version#v}" | sort -V | head -n1) == "${minimum_version#v}" ]]; then
return 0
else
return 1
fi
}

# Check if the report fields are valid according to the rules defined in https://github.com/kubernetes-sigs/gateway-api/blob/release-1.1/conformance/reports/README.md
check_report_fields() {
local report=$1
local expected_gateway_api_version=$2

# Check if the implementation version is a valid semver
local gateway_api_version=$(yq eval '.gatewayAPIVersion' "$report")
local gateway_api_channel=$(yq eval '.gatewayAPIChannel' "$report")
local mode=$(yq eval '.mode' "$report")

if [[ ${gateway_api_version} != ${expected_gateway_api_version} ]]; then
mlavacca marked this conversation as resolved.
Show resolved Hide resolved
error "$report gatewayAPIVersion does not match Gateway API version folder"
EXIT_VALUE=1
fi
if [[ $gateway_api_channel != "standard" && $gateway_api_channel != "experimental" ]]; then
error "$report gatewayAPIChannel is neither standard nor experimental"
EXIT_VALUE=1
fi
if [[ $mode == "" ]]; then
error "$report mode must be set"
EXIT_VALUE=1
fi
}

REPORTS_DIR=$(dirname "${BASH_SOURCE}")/../conformance/reports
# Regex to match the report file name pattern defined in https://github.com/kubernetes-sigs/gateway-api/blob/release-1.1/conformance/reports/README.md#how-this-folder-is-structured
FILE_NAMING_PATTERN="^(standard|experimental)-v?[0-9]+\.[0-9]+(\.[0-9]+)?-[^-]+-report\.yaml$"
EXIT_VALUE=0

for dir in ${REPORTS_DIR}/*
do
element="${dir##*/}"
if check_ge_v1.1.0 "${element}"; then
if [[ -d "${dir}" ]]; then
gateway_api_version="${element}"
for implementation_dir in ${dir}/*
do
implementation=$(basename "${implementation_dir}")
info "Checking ${implementation} project directory for Gateway API version ${gateway_api_version}"

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
mlavacca marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +83 to +84
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

@mlavacca mlavacca Aug 13, 2024

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.

Copy link
Contributor

@jgao1025 jgao1025 Aug 13, 2024

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.

Copy link
Member Author

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!

else
error "missing README.md in ${implementation_dir}"
EXIT_VALUE=1
fi
for report in ${implementation_dir}/*.yaml
do
report_name="${report##*/}"
if [[ ! $report_name =~ $FILE_NAMING_PATTERN ]]; then
error "$report_name does not match the naming pattern defined in the README.md"
EXIT_VALUE=1
fi
check_report_fields "${report}" "${gateway_api_version}"
done
done
fi
fi
done

exit ${EXIT_VALUE}