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

fix "go vet" issues, check as part of golangci-lint #116166

Merged
merged 3 commits into from Mar 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions hack/golangci.yaml
Expand Up @@ -22,6 +22,7 @@ linters:
enable: # please keep this alphabetized
- ginkgolinter
- gocritic
- govet
- ineffassign
- logcheck
- staticcheck
Expand Down
25 changes: 3 additions & 22 deletions hack/make-rules/vet.sh
Expand Up @@ -19,26 +19,7 @@ set -o nounset
set -o pipefail

KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/../..
source "${KUBE_ROOT}/hack/lib/init.sh"

cd "${KUBE_ROOT}"

# Filter out arguments that start with "-" and move them to goflags.
targets=()
goflags=()
for arg; do
if [[ "${arg}" == -* ]]; then
goflags+=("${arg}")
else
targets+=("${arg}")
fi
done

if [[ ${#targets[@]} -eq 0 ]]; then
# Do not run on third_party directories or generated client code or build tools.
while IFS='' read -r line; do
targets+=("${line}")
done < <(go list -e ./... | grep -E -v "/(build|third_party|vendor|staging|clientset_generated|hack)/")
fi

go vet "${goflags[@]:+${goflags[@]}}" "${targets[@]}"
# Ignore the usual golangci.yaml config because it would
# enable additional linters, then enable just "go vet".
"${KUBE_ROOT}/hack/verify-golangci-lint.sh" -c none -- --disable-all --enable=govet "$@"
52 changes: 46 additions & 6 deletions hack/verify-golangci-lint.sh
Expand Up @@ -16,7 +16,16 @@

# This script checks coding style for go language files in each
# Kubernetes package by golint.
# Usage: `hack/verify-golangci-lint.sh`.

usage () {
cat <<EOF >&2
Usage: $0 [-- <golangci-lint run flags>] [packages]"
-c <config|"none">: use the specified configuration or none instead of the default hack/golangci.yaml
[packages]: check specific packages or directories instead of everything
EOF
exit 1
}


set -o errexit
set -o nounset
Expand All @@ -41,7 +50,35 @@ invocation=(./hack/verify-golangci-lint.sh "$@")
# _output/local/bin/golangci-lint cache clean
golangci=(env LOGCHECK_CONFIG="${KUBE_ROOT}/hack/logcheck.conf" "${GOBIN}/golangci-lint" run)
golangci_config="${KUBE_ROOT}/hack/golangci.yaml"
golangci+=(--config="${golangci_config}")
while getopts "c:" o; do
case "${o}" in
c)
if [ "${OPTARG}" = "none" ]; then
golangci_config=""
else
golangci_config="${OPTARG}"
fi
;;
*)
usage
;;
esac
done

if [ "${golangci_config}" ]; then
golangci+=(--config="${golangci_config}")
fi

# Filter out arguments that start with "-" and move them to the run flags.
shift $((OPTIND-1))
targets=()
for arg; do
if [[ "${arg}" == -* ]]; then
golangci+=("${arg}")
else
targets+=("${arg}")
fi
done

kube::golang::verify_go_version

Expand All @@ -52,15 +89,18 @@ export GO111MODULE=on
echo "installing golangci-lint and logcheck plugin from hack/tools into ${GOBIN}"
pushd "${KUBE_ROOT}/hack/tools" >/dev/null
go install github.com/golangci/golangci-lint/cmd/golangci-lint
go build -o "${GOBIN}/logcheck.so" -buildmode=plugin sigs.k8s.io/logtools/logcheck/plugin
if [ "${golangci_config}" ]; then
# This cannot be used without a config.
go build -o "${GOBIN}/logcheck.so" -buildmode=plugin sigs.k8s.io/logtools/logcheck/plugin
fi
popd >/dev/null

cd "${KUBE_ROOT}"

res=0
if [[ "$#" -gt 0 ]]; then
echo "running ${golangci[*]} $*" >&2
"${golangci[@]}" "$@" >&2 || res=$?
if [[ "${#targets[@]}" -gt 0 ]]; then
echo "running ${golangci[*]} ${targets[*]}" >&2
"${golangci[@]}" "${targets[@]}" >&2 || res=$?
else
echo "running ${golangci[*]} ./..." >&2
"${golangci[@]}" ./... >&2 || res=$?
Expand Down
8 changes: 7 additions & 1 deletion hack/verify-govet-levee.sh
Expand Up @@ -39,4 +39,10 @@ popd >/dev/null
LEVEE_BIN="$(which levee)"
CONFIG_FILE="${KUBE_ROOT}/hack/testdata/levee/levee-config.yaml"

"${KUBE_ROOT}"/hack/verify-govet.sh -vettool="${LEVEE_BIN}" -config="${CONFIG_FILE}"
# Do not run on third_party directories or generated client code or build tools.
targets=()
while IFS='' read -r line; do
targets+=("${line}")
done < <(go list --find -e ./... | grep -E -v "/(build|third_party|vendor|staging|clientset_generated|hack)/")

go vet -vettool="${LEVEE_BIN}" -config="${CONFIG_FILE}" "${targets[@]}"
41 changes: 0 additions & 41 deletions hack/verify-govet.sh

This file was deleted.

Expand Up @@ -301,7 +301,7 @@ func (c *DiscoveryController) Run(stopCh <-chan struct{}, synchedCh chan<- struc
}
for _, crd := range crds {
for _, v := range crd.Spec.Versions {
gv := schema.GroupVersion{crd.Spec.Group, v.Name}
gv := schema.GroupVersion{Group: crd.Spec.Group, Version: v.Name}
if err := c.sync(gv); err != nil {
utilruntime.HandleError(fmt.Errorf("failed to initially sync CRD version %v: %v", gv, err))
return false, nil
Expand Down
Expand Up @@ -787,7 +787,7 @@ unknown: foo`
t.Fatal(err)
}
structuralSchemas[v] = structuralSchema
delegate := serializerjson.NewSerializerWithOptions(serializerjson.DefaultMetaFactory, unstructuredCreator{}, nil, serializerjson.SerializerOptions{tc.yaml, false, tc.strictDecoding})
delegate := serializerjson.NewSerializerWithOptions(serializerjson.DefaultMetaFactory, unstructuredCreator{}, nil, serializerjson.SerializerOptions{Yaml: tc.yaml, Strict: tc.strictDecoding})
decoder := &schemaCoercingDecoder{
delegate: delegate,
validator: unstructuredSchemaCoercer{
Expand Down
Expand Up @@ -1087,6 +1087,8 @@ func TestCelCostStability(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
for validRule, expectedCost := range tt.expectCost {
validRule := validRule
expectedCost := expectedCost
testName := validRule
if len(testName) > 127 {
testName = testName[:127]
Expand Down
Expand Up @@ -168,18 +168,18 @@ func compileRule(rule apiextensions.ValidationRule, env *cel.Env, perCallLimit u
}
ast, issues := env.Compile(rule.Rule)
if issues != nil {
compilationResult.Error = &apiservercel.Error{apiservercel.ErrorTypeInvalid, "compilation failed: " + issues.String()}
compilationResult.Error = &apiservercel.Error{Type: apiservercel.ErrorTypeInvalid, Detail: "compilation failed: " + issues.String()}
return
}
if ast.OutputType() != cel.BoolType {
compilationResult.Error = &apiservercel.Error{apiservercel.ErrorTypeInvalid, "cel expression must evaluate to a bool"}
compilationResult.Error = &apiservercel.Error{Type: apiservercel.ErrorTypeInvalid, Detail: "cel expression must evaluate to a bool"}
return
}

checkedExpr, err := cel.AstToCheckedExpr(ast)
if err != nil {
// should be impossible since env.Compile returned no issues
compilationResult.Error = &apiservercel.Error{apiservercel.ErrorTypeInternal, "unexpected compilation error: " + err.Error()}
compilationResult.Error = &apiservercel.Error{Type: apiservercel.ErrorTypeInternal, Detail: "unexpected compilation error: " + err.Error()}
return
}
for _, ref := range checkedExpr.ReferenceMap {
Expand All @@ -198,12 +198,12 @@ func compileRule(rule apiextensions.ValidationRule, env *cel.Env, perCallLimit u
cel.InterruptCheckFrequency(checkFrequency),
)
if err != nil {
compilationResult.Error = &apiservercel.Error{apiservercel.ErrorTypeInvalid, "program instantiation failed: " + err.Error()}
compilationResult.Error = &apiservercel.Error{Type: apiservercel.ErrorTypeInvalid, Detail: "program instantiation failed: " + err.Error()}
return
}
costEst, err := env.EstimateCost(ast, estimator)
if err != nil {
compilationResult.Error = &apiservercel.Error{apiservercel.ErrorTypeInternal, "cost estimation failed: " + err.Error()}
compilationResult.Error = &apiservercel.Error{Type: apiservercel.ErrorTypeInternal, Detail: "cost estimation failed: " + err.Error()}
return
}
compilationResult.MaxCost = costEst.Max
Expand Down
Expand Up @@ -2003,6 +2003,7 @@ func TestValidationExpressionsAtSchemaLevels(t *testing.T) {
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
ctx := context.TODO()
Expand Down
Expand Up @@ -35,13 +35,13 @@ func TestDefault(t *testing.T) {
{"empty", "null", nil, "null"},
{"scalar", "4", &structuralschema.Structural{
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"foo"},
Default: structuralschema.JSON{Object: "foo"},
},
}, "4"},
{"scalar array", "[1,2]", &structuralschema.Structural{
Items: &structuralschema.Structural{
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"foo"},
Default: structuralschema.JSON{Object: "foo"},
},
},
}, "[1,2]"},
Expand All @@ -50,17 +50,17 @@ func TestDefault(t *testing.T) {
Properties: map[string]structuralschema.Structural{
"a": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
"b": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"B"},
Default: structuralschema.JSON{Object: "B"},
},
},
"c": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"C"},
Default: structuralschema.JSON{Object: "C"},
},
},
},
Expand All @@ -73,12 +73,12 @@ func TestDefault(t *testing.T) {
Properties: map[string]structuralschema.Structural{
"a": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
"b": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"B"},
Default: structuralschema.JSON{Object: "B"},
},
},
},
Expand All @@ -88,12 +88,12 @@ func TestDefault(t *testing.T) {
Properties: map[string]structuralschema.Structural{
"a": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"N"},
Default: structuralschema.JSON{Object: "N"},
},
},
"b": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"O"},
Default: structuralschema.JSON{Object: "O"},
},
},
},
Expand All @@ -105,12 +105,12 @@ func TestDefault(t *testing.T) {
Properties: map[string]structuralschema.Structural{
"a": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"alpha"},
Default: structuralschema.JSON{Object: "alpha"},
},
},
"b": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"beta"},
Default: structuralschema.JSON{Object: "beta"},
},
},
},
Expand All @@ -120,7 +120,7 @@ func TestDefault(t *testing.T) {
},
"foo": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"bar"},
Default: structuralschema.JSON{Object: "bar"},
},
},
},
Expand All @@ -130,7 +130,7 @@ func TestDefault(t *testing.T) {
Properties: map[string]structuralschema.Structural{
"a": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
},
Expand All @@ -144,7 +144,7 @@ func TestDefault(t *testing.T) {
Properties: map[string]structuralschema.Structural{
"a": {
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
},
Expand All @@ -156,7 +156,7 @@ func TestDefault(t *testing.T) {
},
Items: &structuralschema.Structural{
Generic: structuralschema.Generic{
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
}, `["A"]`},
Expand All @@ -166,7 +166,7 @@ func TestDefault(t *testing.T) {
"a": {
Generic: structuralschema.Generic{
Nullable: true,
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
},
Expand All @@ -176,7 +176,7 @@ func TestDefault(t *testing.T) {
"a": {
Generic: structuralschema.Generic{
Nullable: false,
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
},
Expand All @@ -187,7 +187,7 @@ func TestDefault(t *testing.T) {
Structural: &structuralschema.Structural{
Generic: structuralschema.Generic{
Nullable: true,
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
},
Expand All @@ -199,7 +199,7 @@ func TestDefault(t *testing.T) {
Structural: &structuralschema.Structural{
Generic: structuralschema.Generic{
Nullable: false,
Default: structuralschema.JSON{"A"},
Default: structuralschema.JSON{Object: "A"},
},
},
},
Expand Down