From 206108a48ca7aeef2402cdbdd2ae16d486d3a8fe Mon Sep 17 00:00:00 2001 From: Guang Ya Liu Date: Thu, 23 May 2019 10:54:27 +0800 Subject: [PATCH] Adding go tools scripts - part 1 (#573) * Added hack scripts for katib. * Run ./hack/update-gofmt.sh. --- cmd/manager/v1alpha1/main_test.go | 4 +- cmd/manager/v1alpha2/main_test.go | 80 +++++------ hack/.golint_failures | 0 hack/update-gofmt.sh | 21 +++ hack/verify-gofmt.sh | 31 ++++ hack/verify-golint.sh | 132 ++++++++++++++++++ pkg/common/v1alpha2/common.go | 6 +- pkg/common/v1alpha2/katib_manager_util.go | 14 +- .../v1alpha1/studyjob/studyjob_controller.go | 2 +- pkg/db/v1alpha1/interface_test.go | 2 +- pkg/db/v1alpha2/interface_test.go | 32 ++--- .../v1alpha1/hyperband_service_test.go | 2 +- test/e2e/v1alpha1/test-client.go | 4 +- 13 files changed, 257 insertions(+), 73 deletions(-) create mode 100644 hack/.golint_failures create mode 100755 hack/update-gofmt.sh create mode 100755 hack/verify-gofmt.sh create mode 100755 hack/verify-golint.sh diff --git a/cmd/manager/v1alpha1/main_test.go b/cmd/manager/v1alpha1/main_test.go index 91ab7e07348..468d882ca61 100644 --- a/cmd/manager/v1alpha1/main_test.go +++ b/cmd/manager/v1alpha1/main_test.go @@ -45,13 +45,13 @@ func TestGetStudies(t *testing.T) { s := &server{} dbIf = mockDB sc := []*api.StudyConfig{ - &api.StudyConfig{ + { Name: "test1", Owner: "admin", OptimizationType: 1, ObjectiveValueName: "obj_name1", }, - &api.StudyConfig{ + { Name: "test2", Owner: "admin", OptimizationType: 1, diff --git a/cmd/manager/v1alpha2/main_test.go b/cmd/manager/v1alpha2/main_test.go index c32ce5b9422..a017ef6f9c5 100644 --- a/cmd/manager/v1alpha2/main_test.go +++ b/cmd/manager/v1alpha2/main_test.go @@ -118,7 +118,7 @@ func TestGetExperimentList(t *testing.T) { req := &api_pb.GetExperimentListRequest{} testExpList := []*api_pb.ExperimentSummary{ - &api_pb.ExperimentSummary{ + { ExperimentName: "test1", Status: &api_pb.ExperimentStatus{ Condition: api_pb.ExperimentStatus_CREATED, @@ -126,7 +126,7 @@ func TestGetExperimentList(t *testing.T) { CompletionTime: "", }, }, - &api_pb.ExperimentSummary{ + { ExperimentName: "test2", Status: &api_pb.ExperimentStatus{ Condition: api_pb.ExperimentStatus_SUCCEEDED, @@ -177,11 +177,11 @@ func TestUpdateAlgorithmExtraSettings(t *testing.T) { req := &api_pb.UpdateAlgorithmExtraSettingsRequest{ ExperimentName: "test1", ExtraAlgorithmSettings: []*api_pb.AlgorithmSetting{ - &api_pb.AlgorithmSetting{ + { Name: "set1", Value: "10", }, - &api_pb.AlgorithmSetting{ + { Name: "set2", Value: "0.5", }, @@ -205,11 +205,11 @@ func TestGetAlgorithmExtraSettings(t *testing.T) { ExperimentName: "test1", } extraAlgoSets := []*api_pb.AlgorithmSetting{ - &api_pb.AlgorithmSetting{ + { Name: "set1", Value: "10", }, - &api_pb.AlgorithmSetting{ + { Name: "set2", Value: "0.5", }, @@ -239,11 +239,11 @@ func TestRegisterTrial(t *testing.T) { RunSpec: "", ParameterAssignments: &api_pb.TrialSpec_ParameterAssignments{ Assignments: []*api_pb.ParameterAssignment{ - &api_pb.ParameterAssignment{ + { Name: "param1", Value: "10", }, - &api_pb.ParameterAssignment{ + { Name: "param2", Value: "0.1", }, @@ -256,19 +256,19 @@ func TestRegisterTrial(t *testing.T) { CompletionTime: "", Observation: &api_pb.Observation{ Metrics: []*api_pb.Metric{ - &api_pb.Metric{ + { Name: "f1_score", Value: "88.95", }, - &api_pb.Metric{ + { Name: "loss", Value: "0.5", }, - &api_pb.Metric{ + { Name: "precision", Value: "88.7", }, - &api_pb.Metric{ + { Name: "recall", Value: "89.2", }, @@ -313,18 +313,18 @@ func TestGetTrialList(t *testing.T) { Filter: "trial", } trialList := []*api_pb.Trial{ - &api_pb.Trial{ + { Name: "test1-trial1", Spec: &api_pb.TrialSpec{ ExperimentName: "test1", RunSpec: "", ParameterAssignments: &api_pb.TrialSpec_ParameterAssignments{ Assignments: []*api_pb.ParameterAssignment{ - &api_pb.ParameterAssignment{ + { Name: "param1", Value: "10", }, - &api_pb.ParameterAssignment{ + { Name: "param2", Value: "0.1", }, @@ -337,19 +337,19 @@ func TestGetTrialList(t *testing.T) { CompletionTime: "", Observation: &api_pb.Observation{ Metrics: []*api_pb.Metric{ - &api_pb.Metric{ + { Name: "f1_score", Value: "88.95", }, - &api_pb.Metric{ + { Name: "loss", Value: "0.5", }, - &api_pb.Metric{ + { Name: "precision", Value: "88.7", }, - &api_pb.Metric{ + { Name: "recall", Value: "89.2", }, @@ -357,18 +357,18 @@ func TestGetTrialList(t *testing.T) { }, }, }, - &api_pb.Trial{ + { Name: "test1-trial2", Spec: &api_pb.TrialSpec{ ExperimentName: "test1", RunSpec: "", ParameterAssignments: &api_pb.TrialSpec_ParameterAssignments{ Assignments: []*api_pb.ParameterAssignment{ - &api_pb.ParameterAssignment{ + { Name: "param1", Value: "20", }, - &api_pb.ParameterAssignment{ + { Name: "param2", Value: "0.5", }, @@ -413,11 +413,11 @@ func TestGetTrial(t *testing.T) { RunSpec: "", ParameterAssignments: &api_pb.TrialSpec_ParameterAssignments{ Assignments: []*api_pb.ParameterAssignment{ - &api_pb.ParameterAssignment{ + { Name: "param1", Value: "10", }, - &api_pb.ParameterAssignment{ + { Name: "param2", Value: "0.1", }, @@ -430,19 +430,19 @@ func TestGetTrial(t *testing.T) { CompletionTime: "", Observation: &api_pb.Observation{ Metrics: []*api_pb.Metric{ - &api_pb.Metric{ + { Name: "f1_score", Value: "88.95", }, - &api_pb.Metric{ + { Name: "loss", Value: "0.5", }, - &api_pb.Metric{ + { Name: "precision", Value: "88.7", }, - &api_pb.Metric{ + { Name: "recall", Value: "89.2", }, @@ -475,19 +475,19 @@ func TestUpdateTrialStatus(t *testing.T) { CompletionTime: "2019-02-03T05:05:06+09:00", Observation: &api_pb.Observation{ Metrics: []*api_pb.Metric{ - &api_pb.Metric{ + { Name: "f1_score", Value: "88.95", }, - &api_pb.Metric{ + { Name: "loss", Value: "0.5", }, - &api_pb.Metric{ + { Name: "precision", Value: "88.7", }, - &api_pb.Metric{ + { Name: "recall", Value: "89.2", }, @@ -513,28 +513,28 @@ func TestReportObservationLog(t *testing.T) { TrialName: "test1-trial1", ObservationLog: &api_pb.ObservationLog{ MetricLogs: []*api_pb.MetricLog{ - &api_pb.MetricLog{ + { TimeStamp: "2019-02-03T04:05:06+09:00", Metric: &api_pb.Metric{ Name: "f1_score", Value: "88.95", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2019-02-03T04:05:06+09:00", Metric: &api_pb.Metric{ Name: "loss", Value: "0.5", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2019-02-03T04:05:06+09:00", Metric: &api_pb.Metric{ Name: "precision", Value: "88.7", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2019-02-03T04:05:06+09:00", Metric: &api_pb.Metric{ Name: "recall", @@ -566,28 +566,28 @@ func TestGetObservationLog(t *testing.T) { obs := &api_pb.ObservationLog{ MetricLogs: []*api_pb.MetricLog{ - &api_pb.MetricLog{ + { TimeStamp: "2019-02-03T04:05:06+09:00", Metric: &api_pb.Metric{ Name: "f1_score", Value: "88.95", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2019-02-03T04:05:06+09:00", Metric: &api_pb.Metric{ Name: "loss", Value: "0.5", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2019-02-03T04:05:06+09:00", Metric: &api_pb.Metric{ Name: "precision", Value: "88.7", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2019-02-03T04:05:06+09:00", Metric: &api_pb.Metric{ Name: "recall", diff --git a/hack/.golint_failures b/hack/.golint_failures new file mode 100644 index 00000000000..e69de29bb2d diff --git a/hack/update-gofmt.sh b/hack/update-gofmt.sh new file mode 100755 index 00000000000..74d3bde1486 --- /dev/null +++ b/hack/update-gofmt.sh @@ -0,0 +1,21 @@ +#!/bin/bash + +# Copyright 2019 The Kubeflow 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 + +find . -name "*.go" | grep -v "\/vendor\/" | xargs gofmt -s -w diff --git a/hack/verify-gofmt.sh b/hack/verify-gofmt.sh new file mode 100755 index 00000000000..7079ba16ea0 --- /dev/null +++ b/hack/verify-gofmt.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +# Copyright 2019 The Kubeflow 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 + +if ! which gofmt > /dev/null; then + echo "Can not find gofmt" + exit 1 +fi + +diff=$(find . -name "*.go" | grep -v "\/vendor\/" | xargs gofmt -s -d 2>&1) +if [[ -n "${diff}" ]]; then + echo "${diff}" + echo + echo "Please run hack/update-gofmt.sh" + exit 1 +fi diff --git a/hack/verify-golint.sh b/hack/verify-golint.sh new file mode 100755 index 00000000000..1ed52480c1b --- /dev/null +++ b/hack/verify-golint.sh @@ -0,0 +1,132 @@ +#!/bin/bash + +# Copyright 2019 The Kubeflow 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 pipefail + +KUBE_ROOT=$(dirname "${BASH_SOURCE}")/.. +#source "${KUBE_ROOT}/hack/lib/init.sh" + +#kube::golang::verify_go_version + +if ! which golint > /dev/null; then + echo 'Can not find golint, install with:' + echo 'go get -u github.com/golang/lint/golint' + exit 1 +fi + +cd "${KUBE_ROOT}" + +array_contains () { + local seeking=$1; shift # shift will iterate through the array + local in=1 # in holds the exit status for the function + for element; do + if [[ "$element" == "$seeking" ]]; then + in=0 # set in to 0 since we found it + break + fi + done + return $in +} + +# Check that the file is in alphabetical order +failure_file="${KUBE_ROOT}/hack/.golint_failures" +if ! diff -u "${failure_file}" <(LC_ALL=C sort "${failure_file}"); then + { + echo + echo "hack/.golint_failures is not in alphabetical order. Please sort it:" + echo + echo " LC_ALL=C sort -o hack/.golint_failures hack/.golint_failures" + echo + } >&2 + false +fi + +export IFS=$'\n' +# NOTE: when "go list -e ./..." is run within GOPATH, it turns the k8s.io/kubernetes +# as the prefix, however if we run it outside it returns the full path of the file +# with a leading underscore. We'll need to support both scenarios for all_packages. +all_packages=( + $(go list -e ./... | egrep -v "/(third_party|vendor|staging/src/k8s.io/client-go/pkg|generated|clientset_generated)" | sed -e 's|^k8s.io/kubernetes/||' -e "s|^_${KUBE_ROOT}/\?||") +) +failing_packages=( + $(cat $failure_file) +) +unset IFS +errors=() +not_failing=() +for p in "${all_packages[@]}"; do + # Run golint on package/*.go file explicitly to validate all go files + # and not just the ones for the current platform. + # Packages with a corresponding foo_test package will make golint fail + # with a useless error. Just ignore that, see golang/lint#68. + failedLint=$(golint "$p"/*.go 2>/dev/null) + array_contains "$p" "${failing_packages[@]}" && in_failing=$? || in_failing=$? + if [[ -n "${failedLint}" ]] && [[ "${in_failing}" -ne "0" ]]; then + errors+=( "${failedLint}" ) + fi + if [[ -z "${failedLint}" ]] && [[ "${in_failing}" -eq "0" ]]; then + not_failing+=( $p ) + fi +done + +# Check that all failing_packages actually still exist +gone=() +for p in "${failing_packages[@]}"; do + array_contains "$p" "${all_packages[@]}" || gone+=( "$p" ) +done + +# Check to be sure all the packages that should pass lint are. +if [ ${#errors[@]} -eq 0 ]; then + echo 'Congratulations! All Go source files have been linted.' +else + { + echo "Errors from golint:" + for err in "${errors[@]}"; do + echo "$err" + done + echo + echo 'Please review the above warnings. You can test via "golint" and commit the result.' + echo 'If the above warnings do not make sense, you can exempt this package from golint' + echo 'checking by adding it to hack/.golint_failures (if your reviewer is okay with it).' + echo + } >&2 + false +fi + +if [[ ${#not_failing[@]} -gt 0 ]]; then + { + echo "Some packages in hack/.golint_failures are passing golint. Please remove them." + echo + for p in "${not_failing[@]}"; do + echo " $p" + done + echo + } >&2 + false +fi + +if [[ ${#gone[@]} -gt 0 ]]; then + { + echo "Some packages in hack/.golint_failures do not exist anymore. Please remove them." + echo + for p in "${gone[@]}"; do + echo " $p" + done + echo + } >&2 + false +fi diff --git a/pkg/common/v1alpha2/common.go b/pkg/common/v1alpha2/common.go index aeab31601db..761c516866c 100644 --- a/pkg/common/v1alpha2/common.go +++ b/pkg/common/v1alpha2/common.go @@ -9,17 +9,17 @@ import ( func GetSupportedJobList() []schema.GroupVersionKind { supportedJobList := []schema.GroupVersionKind{ - schema.GroupVersionKind{ + { Group: "batch", Version: "v1", Kind: "Job", }, - schema.GroupVersionKind{ + { Group: "kubeflow.org", Version: "v1beta2", Kind: "TFJob", }, - schema.GroupVersionKind{ + { Group: "kubeflow.org", Version: "v1beta2", Kind: "PyTorchJob", diff --git a/pkg/common/v1alpha2/katib_manager_util.go b/pkg/common/v1alpha2/katib_manager_util.go index 107a73f570e..c6c409536ba 100644 --- a/pkg/common/v1alpha2/katib_manager_util.go +++ b/pkg/common/v1alpha2/katib_manager_util.go @@ -138,13 +138,13 @@ func GetSuggestions(request *api_pb.GetSuggestionsRequest) (*api_pb.GetSuggestio func PreCheckRegisterExperiment(request *api_pb.RegisterExperimentRequest) (*api_pb.PreCheckRegisterExperimentReply, error) { ctx := context.Background() - kcc, err := getKatibManagerClientAndConn() - if err != nil { - return nil, err - } - defer closeKatibManagerConnection(kcc) - kc := kcc.KatibManagerClient - return kc.PreCheckRegisterExperiment(ctx, request) + kcc, err := getKatibManagerClientAndConn() + if err != nil { + return nil, err + } + defer closeKatibManagerConnection(kcc) + kc := kcc.KatibManagerClient + return kc.PreCheckRegisterExperiment(ctx, request) } func GetObservationLog(request *api_pb.GetObservationLogRequest) (*api_pb.GetObservationLogReply, error) { diff --git a/pkg/controller/v1alpha1/studyjob/studyjob_controller.go b/pkg/controller/v1alpha1/studyjob/studyjob_controller.go index 9433a267e28..eb7b3159841 100644 --- a/pkg/controller/v1alpha1/studyjob/studyjob_controller.go +++ b/pkg/controller/v1alpha1/studyjob/studyjob_controller.go @@ -594,7 +594,7 @@ func (r *ReconcileStudyJobController) getAndRunSuggestion(instance *katibv1alpha katibv1alpha1.TrialSet{ TrialID: t.TrialId, WorkerList: []katibv1alpha1.WorkerCondition{ - katibv1alpha1.WorkerCondition{ + { WorkerID: wid, Kind: wkind.Kind, Condition: katibv1alpha1.ConditionCreated, diff --git a/pkg/db/v1alpha1/interface_test.go b/pkg/db/v1alpha1/interface_test.go index cdd7eab6cc0..c298c8c3012 100644 --- a/pkg/db/v1alpha1/interface_test.go +++ b/pkg/db/v1alpha1/interface_test.go @@ -202,7 +202,7 @@ func TestCreateStudyIdGeneration(t *testing.T) { } t.Logf("id gen %d %s %v\n", i, id, err) } - for id, _ := range encountered { + for id := range encountered { err := mysqlInterface.DeleteStudy(id) if err != nil { t.Errorf("DeleteStudy error %v", err) diff --git a/pkg/db/v1alpha2/interface_test.go b/pkg/db/v1alpha2/interface_test.go index f214fae9794..d06b1cc2007 100644 --- a/pkg/db/v1alpha2/interface_test.go +++ b/pkg/db/v1alpha2/interface_test.go @@ -223,11 +223,11 @@ func TestUpdateAlgorithmExtraSettings(t *testing.T) { //settin1 is already stored and setting2 is not exist in DB. exp_name := "test1" exAlgoSet := []*api_pb.AlgorithmSetting{ - &api_pb.AlgorithmSetting{ + { Name: "setting1", Value: "100", }, - &api_pb.AlgorithmSetting{ + { Name: "setting2", Value: "0.2", }, @@ -268,11 +268,11 @@ func TestRegisterTrial(t *testing.T) { }, ParameterAssignments: &api_pb.TrialSpec_ParameterAssignments{ Assignments: []*api_pb.ParameterAssignment{ - &api_pb.ParameterAssignment{ + { Name: "param1", Value: "0.9", }, - &api_pb.ParameterAssignment{ + { Name: "param2", Value: "10", }, @@ -284,19 +284,19 @@ func TestRegisterTrial(t *testing.T) { Condition: api_pb.TrialStatus_RUNNING, Observation: &api_pb.Observation{ Metrics: []*api_pb.Metric{ - &api_pb.Metric{ + { Name: "f1_score", Value: "88.95", }, - &api_pb.Metric{ + { Name: "loss", Value: "0.5", }, - &api_pb.Metric{ + { Name: "precision", Value: "88.7", }, - &api_pb.Metric{ + { Name: "recall", Value: "89.2", }, @@ -424,19 +424,19 @@ func TestUpdateTrialStatus(t *testing.T) { CompletionTime: "2016-12-31T20:02:06.123456Z", Observation: &api_pb.Observation{ Metrics: []*api_pb.Metric{ - &api_pb.Metric{ + { Name: "f1_score", Value: "88.95", }, - &api_pb.Metric{ + { Name: "loss", Value: "0.5", }, - &api_pb.Metric{ + { Name: "precision", Value: "88.7", }, - &api_pb.Metric{ + { Name: "recall", Value: "89.2", }, @@ -452,28 +452,28 @@ func TestUpdateTrialStatus(t *testing.T) { func TestRegisterObservationLog(t *testing.T) { obsLog := &api_pb.ObservationLog{ MetricLogs: []*api_pb.MetricLog{ - &api_pb.MetricLog{ + { TimeStamp: "2016-12-31T20:02:05.123456Z", Metric: &api_pb.Metric{ Name: "f1_score", Value: "88.95", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2016-12-31T20:02:05.123456Z", Metric: &api_pb.Metric{ Name: "loss", Value: "0.5", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2016-12-31T20:02:05.123456Z", Metric: &api_pb.Metric{ Name: "precision", Value: "88.7", }, }, - &api_pb.MetricLog{ + { TimeStamp: "2016-12-31T20:02:05.123456Z", Metric: &api_pb.Metric{ Name: "recall", diff --git a/pkg/suggestion/v1alpha1/hyperband_service_test.go b/pkg/suggestion/v1alpha1/hyperband_service_test.go index 1b0b0ec8776..13da64073a6 100644 --- a/pkg/suggestion/v1alpha1/hyperband_service_test.go +++ b/pkg/suggestion/v1alpha1/hyperband_service_test.go @@ -503,7 +503,7 @@ func TestEvalWorkers(t *testing.T) { _, rtn_bracket := h.evalWorkers(context.Background(), mockAPI, "studyId", &p) - exp_bracket := []Evals{Evals{"trial3", 19}, Evals{"trial2", 11}, Evals{"trial1", 3}} + exp_bracket := []Evals{{"trial3", 19}, {"trial2", 11}, {"trial1", 3}} for i, ebkt := range exp_bracket { if ebkt != rtn_bracket[i] { diff --git a/test/e2e/v1alpha1/test-client.go b/test/e2e/v1alpha1/test-client.go index 10bdbb5f35b..4b9c5024965 100644 --- a/test/e2e/v1alpha1/test-client.go +++ b/test/e2e/v1alpha1/test-client.go @@ -80,8 +80,8 @@ func main() { mlSet := getMetricsReply.MetricsLogSets //add dummy metricsValueTime - for i, _ := range mlSet { - for j, _ := range mlSet[i].MetricsLogs { + for i := range mlSet { + for j := range mlSet[i].MetricsLogs { mlSet[i].MetricsLogs[j].Values = append(mlSet[i].MetricsLogs[j].Values, &api.MetricsValueTime{Time: "2018-01-01T12:00:00.999999999Z", Value: "1.0"}) mlSet[i].MetricsLogs[j].Values = append(mlSet[i].MetricsLogs[j].Values, &api.MetricsValueTime{Time: "2019-02-02T13:30:00.999999999Z", Value: "2.0"}) }