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

Final vet fixes; enabling vet checks in verify scripts. #24143

Merged
merged 1 commit into from
Apr 13, 2016
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
2 changes: 1 addition & 1 deletion cmd/kube-apiserver/app/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ func TestParseRuntimeConfig(t *testing.T) {

expectedConfig := test.expectedAPIConfig()
if err == nil && !reflect.DeepEqual(actualDisablers, expectedConfig) {
t.Fatalf("%v: unexpected apiResourceDisablers. Actual: %q\n expected: %q", test.runtimeConfig, actualDisablers, expectedConfig)
t.Fatalf("%v: unexpected apiResourceDisablers. Actual: %v\n expected: %v", test.runtimeConfig, actualDisablers, expectedConfig)
}
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/libs/go2idl/client-gen/generators/client_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
return generator.Packages(packageList)
}

orderer := namer.Orderer{namer.NewPrivateNamer(0)}
orderer := namer.Orderer{Namer: namer.NewPrivateNamer(0)}
for _, gv := range customArgs.GroupVersions {
types := gvToTypes[gv]
packageList = append(packageList, packageForGroup(normalization.GroupVersion(gv), orderer.OrderTypes(types), typedClientBasePath, arguments.OutputBase, boilerplate, generatedBy))
Expand Down
28 changes: 14 additions & 14 deletions cmd/libs/go2idl/client-gen/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,15 +95,15 @@ func main() {
"k8s.io/kubernetes/cmd/libs/go2idl/client-gen/testdata/apis/testgroup",
}...)
arguments.CustomArgs = clientgenargs.Args{
[]unversioned.GroupVersion{{Group: "testgroup", Version: ""}},
map[unversioned.GroupVersion]string{
GroupVersions: []unversioned.GroupVersion{{Group: "testgroup", Version: ""}},
GroupVersionToInputPath: map[unversioned.GroupVersion]string{
unversioned.GroupVersion{Group: "testgroup", Version: ""}: "k8s.io/kubernetes/cmd/libs/go2idl/client-gen/testdata/apis/testgroup",
},
"test_internalclientset",
"k8s.io/kubernetes/cmd/libs/go2idl/client-gen/testoutput/clientset_generated/",
false,
false,
cmdArgs,
ClientsetName: "test_internalclientset",
ClientsetOutputPath: "k8s.io/kubernetes/cmd/libs/go2idl/client-gen/testoutput/clientset_generated/",
ClientsetOnly: false,
FakeClient: false,
CmdArgs: cmdArgs,
}
} else {
inputPath, groupVersions, gvToPath, err := parseInputVersions()
Expand All @@ -114,13 +114,13 @@ func main() {
arguments.InputDirs = append(inputPath, dependencies...)

arguments.CustomArgs = clientgenargs.Args{
groupVersions,
gvToPath,
*clientsetName,
*clientsetPath,
*clientsetOnly,
*fakeClient,
cmdArgs,
GroupVersions: groupVersions,
GroupVersionToInputPath: gvToPath,
ClientsetName: *clientsetName,
ClientsetOutputPath: *clientsetPath,
ClientsetOnly: *clientsetOnly,
FakeClient: *fakeClient,
CmdArgs: cmdArgs,
}
}

Expand Down
3 changes: 1 addition & 2 deletions cmd/libs/go2idl/conversion-gen/generators/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
inputs := sets.NewString(arguments.InputDirs...)
packages := generator.Packages{}
header := append([]byte(
`
// +build !ignore_autogenerated
`// +build !ignore_autogenerated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also re-run the conversion generator.


`), boilerplate...)
header = append(header, []byte(
Expand Down
3 changes: 1 addition & 2 deletions cmd/libs/go2idl/deepcopy-gen/generators/deepcopy.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
inputs := sets.NewString(arguments.InputDirs...)
packages := generator.Packages{}
header := append([]byte(
`
// +build !ignore_autogenerated
`// +build !ignore_autogenerated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would re-run the deep copy generator if you do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newline at the beginning doesn't actually make a difference in output. Same with the conversion one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. I would have imagined at least a textual difference. But if there is none, OK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The autogenerated stuff is run through gofmt.


`), boilerplate...)
header = append(header, []byte(
Expand Down
2 changes: 1 addition & 1 deletion cmd/libs/go2idl/generator/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func NewContext(b *parser.Builder, nameSystems namer.NameSystems, canonicalOrder
for name, systemNamer := range nameSystems {
c.Namers[name] = systemNamer
if name == canonicalOrderName {
orderer := namer.Orderer{systemNamer}
orderer := namer.Orderer{Namer: systemNamer}
c.Order = orderer.OrderUniverse(u)
}
}
Expand Down
4 changes: 3 additions & 1 deletion cmd/libs/go2idl/import-boss/generators/import_restrict.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,12 @@ type importRuleFile struct{}

func (importRuleFile) AssembleFile(f *generator.File, path string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lavalamp Can this whole function just go away?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it implements an interface.

return nil
}

// TODO: make a flag to enable this, or expose this information in some other way.
func (importRuleFile) listEntireImportTree(f *generator.File, path string) error {
// If the file exists, populate its current imports. This is mostly to help
// humans figure out what they need to fix.
// TODO: add a command line flag to enable this? Or require that it always stay up-to-date?
if _, err := os.Stat(path); err != nil {
// Ignore packages which haven't opted in by adding an .import-restrictions file.
return nil
Expand Down
2 changes: 1 addition & 1 deletion cmd/libs/go2idl/parser/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func construct(t *testing.T, files map[string]string, testNamer namer.Namer) (*p
if err != nil {
t.Fatal(err)
}
orderer := namer.Orderer{testNamer}
orderer := namer.Orderer{Namer: testNamer}
o := orderer.OrderUniverse(u)
return b, u, o
}
Expand Down
6 changes: 3 additions & 3 deletions contrib/mesos/pkg/node/registrator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func calledOnce(h bool, ret runtime.Object, err error) (<-chan struct{}, func(co

func TestRegister_withUnknownNode(t *testing.T) {
fc := &core.Fake{}
nodes := &fakeNodes{&fake.FakeNodes{&fake.FakeCore{Fake: fc}}}
nodes := &fakeNodes{&fake.FakeNodes{Fake: &fake.FakeCore{Fake: fc}}}
createCalled, createOnce := calledOnce(true, nil, nil)
fc.AddReactor("create", "nodes", createOnce)

Expand Down Expand Up @@ -84,7 +84,7 @@ func TestRegister_withUnknownNode(t *testing.T) {

func TestRegister_withKnownNode(t *testing.T) {
fc := &core.Fake{}
nodes := &fakeNodes{&fake.FakeNodes{&fake.FakeCore{Fake: fc}}}
nodes := &fakeNodes{&fake.FakeNodes{Fake: &fake.FakeCore{Fake: fc}}}
updateCalled, updateOnce := calledOnce(true, nil, nil)
fc.AddReactor("update", "nodes", updateOnce)

Expand Down Expand Up @@ -122,7 +122,7 @@ func TestRegister_withSemiKnownNode(t *testing.T) {
// CreateOrUpdate should proceed to attempt an update.

fc := &core.Fake{}
nodes := &fakeNodes{&fake.FakeNodes{&fake.FakeCore{Fake: fc}}}
nodes := &fakeNodes{&fake.FakeNodes{Fake: &fake.FakeCore{Fake: fc}}}

createCalled, createOnce := calledOnce(true, nil, errors.NewAlreadyExists(unversioned.GroupResource{Group: "", Resource: ""}, "nodes"))
fc.AddReactor("create", "nodes", createOnce)
Expand Down
2 changes: 1 addition & 1 deletion contrib/mesos/pkg/scheduler/queuer/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (p *Pod) GetUID() string {

// implements Deadlined
func (dp *Pod) Deadline() (time.Time, bool) {
if dp.Deadline != nil {
if dp.deadline != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. What was the vet error that caught this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, bad news. "comparison of function Deadline != nil is always true"

return *(dp.deadline), true
}
return time.Time{}, false
Expand Down
3 changes: 2 additions & 1 deletion examples/examples_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ func validateObject(obj runtime.Object) (errors field.ErrorList) {
}
errors = expvalidation.ValidateDaemonSet(t)
default:
return field.ErrorList{field.InternalError(field.NewPath(""), fmt.Errorf("no validation defined for %#v", obj))}
errors = field.ErrorList{}
errors = append(errors, field.InternalError(field.NewPath(""), fmt.Errorf("no validation defined for %#v", obj)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Didn't godep go vet fix the list alias issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I did expect the addition of godep to fix this and the one in cmd_test.go, but it still complained.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I run go vet in this directory, I get no errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this? I don't see this vet issue from my client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, you're right, no repro now

}
return errors
}
Expand Down
2 changes: 1 addition & 1 deletion federation/cmd/federated-apiserver/app/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ func TestParseRuntimeConfig(t *testing.T) {

expectedConfig := test.expectedAPIConfig()
if err == nil && !reflect.DeepEqual(actualDisablers, expectedConfig) {
t.Fatalf("%v: unexpected apiResourceDisablers. Actual: %q\n expected: %q", test.runtimeConfig, actualDisablers, expectedConfig)
t.Fatalf("%v: unexpected apiResourceDisablers. Actual: %v\n expected: %v", test.runtimeConfig, actualDisablers, expectedConfig)
}
}

Expand Down
77 changes: 77 additions & 0 deletions hack/verify-govet.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
#!/bin/bash

# Copyright 2016 The Kubernetes Authors All rights reserved.
#
# 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

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

cd "${KUBE_ROOT}"

# Use eval to preserve embedded quoted strings.
eval "goflags=(${KUBE_GOFLAGS:-})"

# Filter out arguments that start with "-" and move them to goflags.
targets=()
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.
targets=$(go list ./... | grep -v "third_party")
fi

# Do this in parallel; results in 5-10x speedup.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also launches a lot of processes. What is the runtime? And is the speed up worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally - from ~71s to ~14s.

pids=""
for i in $targets
do
(
# Run go vet using goflags for each target specified.
#
# Remove any lines go vet or godep outputs with the exit status.
# Remove any lines godep outputs about the vendor experiment.
#
# If go vet fails (produces output), grep will succeed, but if go vet
# succeeds (produces no output) grep will fail. Then we just use
# PIPESTATUS[0] which is go's exit code.
#
# The intended result is that each incantation of this line returns
# either 0 (pass) or 1 (fail).
godep go vet "${goflags[@]:+${goflags[@]}}" "$i" 2>&1 \
| grep -v -E "exit status|GO15VENDOREXPERIMENT=" \
|| fail=${PIPESTATUS[0]}
exit $fail
) &
pids+=" $!"
done

# Count and return the number of failed files (non-zero is a failed vet run).
failedfiles=0
for p in $pids; do
wait $p || let "failedfiles+=1"
done

# hardcode a healthy exit until all vet errors can be fixed
#exit $failedfiles
exit 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't keep up with the handful of vet errors that are added each day. Let me get this PR in, then do one more clean up round.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that sounds fair.

59 changes: 0 additions & 59 deletions hack/vet-go.sh

This file was deleted.

1 change: 0 additions & 1 deletion pkg/api/serialization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ func roundTrip(t *testing.T, codec runtime.Codec, item runtime.Object) {
if err != nil {
t.Errorf("0: %v: %v\nCodec: %v\nData: %s\nSource: %#v", name, err, codec, dataAsString(data), printer.Sprintf("%#v", item))
panic("failed")
return
}
if !api.Semantic.DeepEqual(item, obj2) {
t.Errorf("\n1: %v: diff: %v\nCodec: %v\nSource:\n\n%#v\n\nEncoded:\n\n%s\n\nFinal:\n\n%#v", name, diff.ObjectGoPrintDiff(item, obj2), codec, printer.Sprintf("%#v", item), dataAsString(data), printer.Sprintf("%#v", obj2))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,17 @@ type Clientset struct {
}

func (c *Clientset) Discovery() discovery.DiscoveryInterface {
return &fakediscovery.FakeDiscovery{&c.Fake}
return &fakediscovery.FakeDiscovery{Fake: &c.Fake}
}

var _ clientset.Interface = &Clientset{}

// Core retrieves the CoreClient
func (c *Clientset) Core() v1core.CoreInterface {
return &fakev1core.FakeCore{&c.Fake}
return &fakev1core.FakeCore{Fake: &c.Fake}
}

// Extensions retrieves the ExtensionsClient
func (c *Clientset) Extensions() v1beta1extensions.ExtensionsInterface {
return &fakev1beta1extensions.FakeExtensions{&c.Fake}
return &fakev1beta1extensions.FakeExtensions{Fake: &c.Fake}
}
5 changes: 4 additions & 1 deletion pkg/kubectl/cmd/cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,12 @@ func NewTestFactory() (*cmdutil.Factory, *testFactory, runtime.Codec) {

func NewMixedFactory(apiClient resource.RESTClient) (*cmdutil.Factory, *testFactory, runtime.Codec) {
f, t, c := NewTestFactory()
var multiRESTMapper meta.MultiRESTMapper
multiRESTMapper = append(multiRESTMapper, t.Mapper)
multiRESTMapper = append(multiRESTMapper, testapi.Default.RESTMapper())
f.Object = func(discovery bool) (meta.RESTMapper, runtime.ObjectTyper) {
priorityRESTMapper := meta.PriorityRESTMapper{
Delegate: meta.MultiRESTMapper{t.Mapper, testapi.Default.RESTMapper()},
Delegate: multiRESTMapper,
ResourcePriority: []unversioned.GroupVersionResource{
{Group: meta.AnyGroup, Version: "v1", Resource: meta.AnyResource},
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/kubectl/describe.go
Original file line number Diff line number Diff line change
Expand Up @@ -1696,7 +1696,7 @@ func describeNodeResource(nodeNonTerminatedPodsList *api.PodList, node *api.Node
memoryReq.String(), int64(fractionMemoryReq), memoryLimit.String(), int64(fractionMemoryLimit))
}

fmt.Fprint(out, "Allocated resources:\n (Total limits may be over 100%, i.e., overcommitted. More info: http://releases.k8s.io/HEAD/docs/user-guide/compute-resources.md)\n CPU Requests\tCPU Limits\tMemory Requests\tMemory Limits\n")
fmt.Fprint(out, "Allocated resources:\n (Total limits may be over 100 percent, i.e., overcommitted. More info: http://releases.k8s.io/HEAD/docs/user-guide/compute-resources.md)\n CPU Requests\tCPU Limits\tMemory Requests\tMemory Limits\n")
fmt.Fprint(out, " ------------\t----------\t---------------\t-------------\n")
reqs, limits, err := getPodsTotalRequestsAndLimits(nodeNonTerminatedPodsList)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions pkg/kubectl/scale_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ func TestJobScaleFailsPreconditions(t *testing.T) {
Parallelism: &ten,
},
})
scaler := JobScaler{&testclient.FakeExperimental{fake}}
scaler := JobScaler{&testclient.FakeExperimental{Fake: fake}}
preconditions := ScalePrecondition{2, ""}
count := uint(3)
name := "foo"
Expand Down Expand Up @@ -586,7 +586,7 @@ func TestDeploymentScaleFailsPreconditions(t *testing.T) {
Replicas: 10,
},
})
scaler := DeploymentScaler{&testclient.FakeExperimental{fake}}
scaler := DeploymentScaler{&testclient.FakeExperimental{Fake: fake}}
preconditions := ScalePrecondition{2, ""}
count := uint(3)
name := "foo"
Expand Down
2 changes: 1 addition & 1 deletion pkg/kubectl/stop_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ func TestSimpleStop(t *testing.T) {
}
actions := fake.Actions()
if len(test.actions) != len(actions) {
t.Errorf("unexpected actions: %v; expected %v (%s)", fake.Actions, test.actions, test.test)
t.Errorf("unexpected actions: %v; expected %v (%s)", actions, test.actions, test.test)
}
for i, action := range actions {
testAction := test.actions[i]
Expand Down
2 changes: 1 addition & 1 deletion pkg/util/flowcontrol/backoff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestBackoffReset(t *testing.T) {
lastUpdate := tc.Now()
tc.Step(2*maxDuration + step) // time += 11s, 11 > 2*maxDuration
if b.IsInBackOffSince(id, lastUpdate) {
t.Errorf("now=%s lastUpdate=%s (%s) expected Backoff reset got %s b.lastUpdate=%s", tc.Now(), startTime, tc.Now().Sub(lastUpdate), b.Get(id))
t.Errorf("expected to not be in Backoff after reset (start=%s, now=%s, lastUpdate=%s), got %s", startTime, tc.Now(), lastUpdate, b.Get(id))
}
}

Expand Down