From 2adb95c3763e447bed57b9142fb0ae24a593a91c Mon Sep 17 00:00:00 2001 From: Hao Ruan Date: Tue, 10 Jan 2023 09:21:26 +0800 Subject: [PATCH 1/3] Add linter to check if api docs match field tag names --- .../field_name_docs_check.go | 105 ++++++++++++++++++ hack/.fieldname_docs_failures | 1 + hack/verify-fieldname-docs.sh | 71 ++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 cmd/fieldnamedocscheck/field_name_docs_check.go create mode 100644 hack/.fieldname_docs_failures create mode 100755 hack/verify-fieldname-docs.sh diff --git a/cmd/fieldnamedocscheck/field_name_docs_check.go b/cmd/fieldnamedocscheck/field_name_docs_check.go new file mode 100644 index 00000000000..9271883c9e8 --- /dev/null +++ b/cmd/fieldnamedocscheck/field_name_docs_check.go @@ -0,0 +1,105 @@ +/* +Copyright 2023 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. +*/ + +package main + +import ( + "fmt" + "os" + "regexp" + "strings" + + flag "github.com/spf13/pflag" + kruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/klog/v2" +) + +var ( + typeSrc = flag.StringP("type-src", "s", "", "From where we are going to read the types") + re = regexp.MustCompile("`(\\b\\w+\\b)`") +) + +// kubeTypesMap is a map from field name to its tag name and doc. +type kubeTypesMap map[string]kruntime.Pair + +func main() { + flag.Parse() + + if *typeSrc == "" { + klog.Fatalf("Please define -s flag as it is the api type file") + } + + docsForTypes := kruntime.ParseDocumentationFrom(*typeSrc) + rc := false + + for _, ks := range docsForTypes { + typesMap := make(kubeTypesMap) + + for _, p := range ks[1:] { + // skip the field with no tag name + if p.Name != "" { + typesMap[strings.ToLower(p.Name)] = p + } + } + + structName := ks[0].Name + + rc = checkFieldNameAndDoc(structName, "", ks[0].Doc, typesMap) + for _, p := range ks[1:] { + rc = checkFieldNameAndDoc(structName, p.Name, p.Doc, typesMap) + } + } + + if rc { + os.Exit(1) + } +} + +func checkFieldNameAndDoc(structName, fieldName, doc string, typesMap kubeTypesMap) bool { + rc := false + visited := sets.Set[string]{} + + // The rule is: + // 1. Get all back-tick quoted names in the doc + // 2. Skip the name which is already found mismatched. + // 3. Skip the name whose lowercase is different from the lowercase of tag names, + // because some docs use back-tick to quote field value or nil + // 4. Check if the name is different from its tag name + + // TODO: a manual pass adding back-ticks to the doc strings, then update the linter to + // check the existence of back-ticks + nameGroups := re.FindAllStringSubmatch(doc, -1) + for _, nameGroup := range nameGroups { + name := nameGroup[1] + if visited.Has(name) { + continue + } + if p, ok := typesMap[strings.ToLower(name)]; ok && p.Name != name { + rc = true + visited.Insert(name) + + fmt.Fprintf(os.Stderr, "Error: doc for %s", structName) + if fieldName != "" { + fmt.Fprintf(os.Stderr, ".%s", fieldName) + } + + fmt.Fprintf(os.Stderr, " contains: %s, which should be: %s\n", name, p.Name) + } + } + + return rc +} diff --git a/hack/.fieldname_docs_failures b/hack/.fieldname_docs_failures new file mode 100644 index 00000000000..eec53d163dd --- /dev/null +++ b/hack/.fieldname_docs_failures @@ -0,0 +1 @@ +./staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1 diff --git a/hack/verify-fieldname-docs.sh b/hack/verify-fieldname-docs.sh new file mode 100755 index 00000000000..62dd32574b5 --- /dev/null +++ b/hack/verify-fieldname-docs.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash + +# Copyright 2023 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. + +# This script checks API-related files for mismatch in docs and field names, +# and outputs a list of fields that their docs and field names are mismatched. +# Usage: `hack/verify-fieldname-docs.sh`. + +set -o errexit +set -o nounset +set -o pipefail + +KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. +source "${KUBE_ROOT}/hack/lib/init.sh" +source "${KUBE_ROOT}/hack/lib/util.sh" + +kube::golang::setup_env + +make -C "${KUBE_ROOT}" WHAT=cmd/fieldnamedocscheck + +# Find binary +fieldnamedocscheck=$(kube::util::find-binary "fieldnamedocscheck") + +result=0 + +find_files() { + find . -not \( \ + \( \ + -wholename './output' \ + -o -wholename './_output' \ + -o -wholename './_gopath' \ + -o -wholename './release' \ + -o -wholename './target' \ + -o -wholename '*/third_party/*' \ + -o -wholename '*/vendor/*' \ + -o -wholename './pkg/*' \ + \) -prune \ + \) \ + \( -wholename './staging/src/k8s.io/api/*/v*/types.go' \ + -o -wholename './staging/src/k8s.io/kube-aggregator/pkg/apis/*/v*/types.go' \ + -o -wholename './staging/src/k8s.io/apiextensions-apiserver/pkg/apis/*/v*/types.go' \ + \) +} + +versioned_api_files=$(find_files) || true + +failure_file="${KUBE_ROOT}/hack/.fieldname_docs_failures" +failing_groups=() +while IFS='' read -r line; do failing_groups+=("${line}"); done < <(cat "${failure_file}") + +for file in ${versioned_api_files}; do + package="${file%"/types.go"}" + if ! kube::util::array_contains "${package}" "${failing_groups[@]}"; then + echo "Checking ${package}" + ${fieldnamedocscheck} -s "${file}" || result=$? + fi +done + +exit ${result} From b64dcf862d96b33822297b62fa0addfef80f4bdf Mon Sep 17 00:00:00 2001 From: Hao Ruan Date: Thu, 19 Jan 2023 22:08:29 +0800 Subject: [PATCH 2/3] Fix the name violation in apiextensions.k8s.io/v1,CustomResourceConversion, remove the failures file --- hack/.fieldname_docs_failures | 1 - hack/verify-fieldname-docs.sh | 12 +++--------- .../pkg/apis/apiextensions/v1/types.go | 6 +++--- 3 files changed, 6 insertions(+), 13 deletions(-) delete mode 100644 hack/.fieldname_docs_failures diff --git a/hack/.fieldname_docs_failures b/hack/.fieldname_docs_failures deleted file mode 100644 index eec53d163dd..00000000000 --- a/hack/.fieldname_docs_failures +++ /dev/null @@ -1 +0,0 @@ -./staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1 diff --git a/hack/verify-fieldname-docs.sh b/hack/verify-fieldname-docs.sh index 62dd32574b5..2997234cf65 100755 --- a/hack/verify-fieldname-docs.sh +++ b/hack/verify-fieldname-docs.sh @@ -56,16 +56,10 @@ find_files() { versioned_api_files=$(find_files) || true -failure_file="${KUBE_ROOT}/hack/.fieldname_docs_failures" -failing_groups=() -while IFS='' read -r line; do failing_groups+=("${line}"); done < <(cat "${failure_file}") - for file in ${versioned_api_files}; do - package="${file%"/types.go"}" - if ! kube::util::array_contains "${package}" "${failing_groups[@]}"; then - echo "Checking ${package}" - ${fieldnamedocscheck} -s "${file}" || result=$? - fi + package="${file%"/types.go"}" + echo "Checking ${package}" + ${fieldnamedocscheck} -s "${file}" || result=$? done exit ${result} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types.go index 285058d77a2..59ec0e372b8 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types.go @@ -74,12 +74,12 @@ type CustomResourceDefinitionSpec struct { // CustomResourceConversion describes how to convert different versions of a CR. type CustomResourceConversion struct { // strategy specifies how custom resources are converted between versions. Allowed values are: - // - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource. - // - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information + // - `"None"`: The converter only change the apiVersion and would not touch any other field in the custom resource. + // - `"Webhook"`: API Server will call to an external webhook to do the conversion. Additional information // is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set. Strategy ConversionStrategyType `json:"strategy" protobuf:"bytes,1,name=strategy"` - // webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`. + // webhook describes how to call the conversion webhook. Required when `strategy` is set to `"Webhook"`. // +optional Webhook *WebhookConversion `json:"webhook,omitempty" protobuf:"bytes,2,opt,name=webhook"` } From 08210a1cb52c18d44aafe4648102aba9d1a99c31 Mon Sep 17 00:00:00 2001 From: Hao Ruan Date: Thu, 19 Jan 2023 22:14:40 +0800 Subject: [PATCH 3/3] Generated files --- api/openapi-spec/swagger.json | 4 ++-- .../v3/apis__apiextensions.k8s.io__v1_openapi.json | 4 ++-- pkg/generated/openapi/zz_generated.openapi.go | 4 ++-- .../pkg/apis/apiextensions/v1/generated.proto | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index 1d75d762f01..401ed9f6ff8 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -14119,12 +14119,12 @@ "description": "CustomResourceConversion describes how to convert different versions of a CR.", "properties": { "strategy": { - "description": "strategy specifies how custom resources are converted between versions. Allowed values are: - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information\n is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.", + "description": "strategy specifies how custom resources are converted between versions. Allowed values are: - `\"None\"`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `\"Webhook\"`: API Server will call to an external webhook to do the conversion. Additional information\n is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.", "type": "string" }, "webhook": { "$ref": "#/definitions/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.WebhookConversion", - "description": "webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`." + "description": "webhook describes how to call the conversion webhook. Required when `strategy` is set to `\"Webhook\"`." } }, "required": [ diff --git a/api/openapi-spec/v3/apis__apiextensions.k8s.io__v1_openapi.json b/api/openapi-spec/v3/apis__apiextensions.k8s.io__v1_openapi.json index 74a002f00f7..1050b8e2126 100644 --- a/api/openapi-spec/v3/apis__apiextensions.k8s.io__v1_openapi.json +++ b/api/openapi-spec/v3/apis__apiextensions.k8s.io__v1_openapi.json @@ -45,7 +45,7 @@ "properties": { "strategy": { "default": "", - "description": "strategy specifies how custom resources are converted between versions. Allowed values are: - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information\n is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.", + "description": "strategy specifies how custom resources are converted between versions. Allowed values are: - `\"None\"`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `\"Webhook\"`: API Server will call to an external webhook to do the conversion. Additional information\n is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.", "type": "string" }, "webhook": { @@ -54,7 +54,7 @@ "$ref": "#/components/schemas/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.WebhookConversion" } ], - "description": "webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`." + "description": "webhook describes how to call the conversion webhook. Required when `strategy` is set to `\"Webhook\"`." } }, "required": [ diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index 5991381788b..cdad46eb224 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -45157,7 +45157,7 @@ func schema_pkg_apis_apiextensions_v1_CustomResourceConversion(ref common.Refere Properties: map[string]spec.Schema{ "strategy": { SchemaProps: spec.SchemaProps{ - Description: "strategy specifies how custom resources are converted between versions. Allowed values are: - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information\n is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.", + Description: "strategy specifies how custom resources are converted between versions. Allowed values are: - `\"None\"`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `\"Webhook\"`: API Server will call to an external webhook to do the conversion. Additional information\n is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.", Default: "", Type: []string{"string"}, Format: "", @@ -45165,7 +45165,7 @@ func schema_pkg_apis_apiextensions_v1_CustomResourceConversion(ref common.Refere }, "webhook": { SchemaProps: spec.SchemaProps{ - Description: "webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`.", + Description: "webhook describes how to call the conversion webhook. Required when `strategy` is set to `\"Webhook\"`.", Ref: ref("k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.WebhookConversion"), }, }, diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/generated.proto b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/generated.proto index d0b190fd564..643dda106b0 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/generated.proto +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/generated.proto @@ -107,12 +107,12 @@ message CustomResourceColumnDefinition { // CustomResourceConversion describes how to convert different versions of a CR. message CustomResourceConversion { // strategy specifies how custom resources are converted between versions. Allowed values are: - // - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource. - // - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information + // - `"None"`: The converter only change the apiVersion and would not touch any other field in the custom resource. + // - `"Webhook"`: API Server will call to an external webhook to do the conversion. Additional information // is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set. optional string strategy = 1; - // webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`. + // webhook describes how to call the conversion webhook. Required when `strategy` is set to `"Webhook"`. // +optional optional WebhookConversion webhook = 2; }