From c163b996a4fcc16ac2c7fc4a6d80cf9c49902f4f Mon Sep 17 00:00:00 2001 From: Brandon Stoll Date: Thu, 9 Jul 2026 18:36:26 +0000 Subject: [PATCH] refactor(cmd): fix error handling and linter issues in CLI and loaders (5/5) This is part 5/5 of an overall cleanup effort to fix linter issues and format files across the repository. In this step: - Handle errors returned by viper.BindPFlags in command root and topology tests. - Standardize error message capitalization in CLI and loader modules. - Rename test variables for clarity (DNE -> Missing). - Remove new-from-rev from .golangci.yml to enforce full repository linting (check all) going forward. --- .github/linters/.golangci.yml | 50 ++++++++++++++++++++++++++++++++--- cmd/deploy/deploy.go | 4 +-- cmd/root.go | 4 ++- cmd/topology/topology_test.go | 37 +++++++++++++------------- deploy/deploy.go | 7 +++-- load/deploy.go | 10 +++---- 6 files changed, 80 insertions(+), 32 deletions(-) diff --git a/.github/linters/.golangci.yml b/.github/linters/.golangci.yml index 5e10a88a4..f0e73e113 100644 --- a/.github/linters/.golangci.yml +++ b/.github/linters/.golangci.yml @@ -1,4 +1,48 @@ +--- version: "2" - -issues: - new-from-rev: origin/main +linters: + enable: + - gocritic + - gosec + - revive + - unconvert + - unparam + - wastedassign + - whitespace + settings: + errcheck: + check-blank: true + gocritic: + disabled-checks: + - singleCaseSwitch + - appendAssign + revive: + severity: warning + exclusions: + generated: lax + presets: + - comments + - common-false-positives + - legacy + - std-error-handling + rules: + - linters: + - dupl + - goconst + - gosec + path: _test\.go + paths: + - third_party$ + - builtin$ + - examples$ +formatters: + enable: + - gofmt + - goimports + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ + - \.pb\.go diff --git a/cmd/deploy/deploy.go b/cmd/deploy/deploy.go index f55ad53fe..389205697 100644 --- a/cmd/deploy/deploy.go +++ b/cmd/deploy/deploy.go @@ -84,10 +84,10 @@ func newDeployment(cfgPath string, testing bool) (*deploy.Deployment, error) { return nil, err } if cfg.Cluster == nil { - return nil, fmt.Errorf("Cluster not specified") + return nil, fmt.Errorf("cluster not specified") } if cfg.Ingress == nil { - return nil, fmt.Errorf("Ingress not specified") + return nil, fmt.Errorf("ingress not specified") } if cfg.CNI == nil { return nil, fmt.Errorf("CNI not specified") diff --git a/cmd/root.go b/cmd/root.go index af6a54f9e..884e4bc1a 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -54,7 +54,9 @@ environment.`, return fmt.Errorf("error reading config: %w", err) } } - viper.BindPFlags(cmd.Flags()) + if err := viper.BindPFlags(cmd.Flags()); err != nil { + return err + } viper.SetDefault("report_usage", false) return nil } diff --git a/cmd/topology/topology_test.go b/cmd/topology/topology_test.go index 7aa1a1924..bc3f3bb7a 100644 --- a/cmd/topology/topology_test.go +++ b/cmd/topology/topology_test.go @@ -150,7 +150,7 @@ func TestReset(t *testing.T) { } fConfigRelative, closer := writeTopology(t, tWithConfigRelative) defer closer() - tWithConfigDNE := &tpb.Topology{ + tWithConfigMissing := &tpb.Topology{ Nodes: []*tpb.Node{{ Name: "resettable1", Vendor: tpb.Vendor(1001), @@ -164,7 +164,7 @@ func TestReset(t *testing.T) { Vendor: tpb.Vendor(1001), Config: &tpb.Config{ ConfigData: &tpb.Config_File{ - File: "dne", + File: "missing", }, }, }, { @@ -172,7 +172,7 @@ func TestReset(t *testing.T) { Vendor: tpb.Vendor(1002), }}, } - fConfigDNE, closer := writeTopology(t, tWithConfigDNE) + fConfigMissing, closer := writeTopology(t, tWithConfigMissing) defer closer() node.Vendor(tpb.Vendor(1001), NewR) node.Vendor(tpb.Vendor(1002), NewNR) @@ -206,15 +206,15 @@ func TestReset(t *testing.T) { desc: "valid topology push with relative file location", args: []string{"reset", fConfigRelative.Name(), "--skip", "--push"}, }, { - desc: "valid topology push with config DNE", - args: []string{"reset", fConfigDNE.Name(), "--skip", "--push"}, + desc: "valid topology push with config missing", + args: []string{"reset", fConfigMissing.Name(), "--skip", "--push"}, wantErr: "no such file or directory", }, { - desc: "valid topology push with config DNE single device", - args: []string{"reset", fConfigDNE.Name(), "--skip", "--push", "resettable1"}, + desc: "valid topology push with config missing single device", + args: []string{"reset", fConfigMissing.Name(), "--skip", "--push", "resettable1"}, }, { - desc: "valid topology push with config DNE single device invalid", - args: []string{"reset", fConfigDNE.Name(), "--skip", "--push", "dne"}, + desc: "valid topology push with config missing single device invalid", + args: []string{"reset", fConfigMissing.Name(), "--skip", "--push", "missing"}, wantErr: "not found", }} @@ -234,8 +234,7 @@ func TestReset(t *testing.T) { }() rCmd.PersistentFlags().String("kubecfg", "", "") rCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { - viper.BindPFlags(cmd.Flags()) - return nil + return viper.BindPFlags(cmd.Flags()) } buf := bytes.NewBuffer([]byte{}) rCmd.SetOut(buf) @@ -370,7 +369,7 @@ func TestGenerateRing(t *testing.T) { wantErr: "links must be positive", }, { desc: "file not found", - args: []string{"generate", "ring", "dne.textproto", "2", "8"}, + args: []string{"generate", "ring", "missing.textproto", "2", "8"}, wantErr: "no such file", }, { desc: "empty topology", @@ -571,10 +570,10 @@ func TestPush(t *testing.T) { defer os.Remove(confFile.Name()) tWithConfig := &tpb.Topology{ Nodes: []*tpb.Node{{ - Name: "configable", + Name: "configurable", Vendor: tpb.Vendor(1003), }, { - Name: "notconfigable", + Name: "notconfigurable", Vendor: tpb.Vendor(1004), }}, } @@ -594,22 +593,22 @@ func TestPush(t *testing.T) { }, { desc: "missing args", wantErr: "invalid args", - args: []string{"push", fConfig.Name(), "configable"}, + args: []string{"push", fConfig.Name(), "configurable"}, }, { desc: "no file", - args: []string{"push", fConfig.Name(), "configable", "filedne"}, + args: []string{"push", fConfig.Name(), "configurable", "filemissing"}, wantErr: "no such file", }, { desc: "valid file invalid device", args: []string{"push", fConfig.Name(), "foo", confFile.Name()}, wantErr: `node "foo" not found`, }, { - desc: "valid file notconfigable device", - args: []string{"push", fConfig.Name(), "notconfigable", confFile.Name()}, + desc: "valid file notconfigurable device", + args: []string{"push", fConfig.Name(), "notconfigurable", confFile.Name()}, wantErr: "does not implement ConfigPusher", }, { desc: "valid file", - args: []string{"push", fConfig.Name(), "configable", confFile.Name()}, + args: []string{"push", fConfig.Name(), "configurable", confFile.Name()}, }} rCmd := New() diff --git a/deploy/deploy.go b/deploy/deploy.go index 47ae5cc1a..1523e519e 100644 --- a/deploy/deploy.go +++ b/deploy/deploy.go @@ -110,7 +110,10 @@ type Deployment struct { } func (d *Deployment) String() string { - b, _ := json.MarshalIndent(d, "", "\t") + b, err := json.MarshalIndent(d, "", "\t") + if err != nil { + return fmt.Sprintf("Deployment: %+v (marshal error: %v)", *d, err) + } return string(b) } @@ -219,7 +222,7 @@ func (d *Deployment) Deploy(ctx context.Context, kubecfg string) (rerr error) { ctx, cancel := context.WithCancel(ctx) - // Watch the containter status of the pods so we can fail if a container fails to start running. + // Watch the container status of the pods so we can fail if a container fails to start running. if w, err := pods.NewWatcher(ctx, kClient, cancel); err != nil { log.Warningf("Failed to start pod watcher: %v", err) } else { diff --git a/load/deploy.go b/load/deploy.go index 684e363db..aaf427001 100644 --- a/load/deploy.go +++ b/load/deploy.go @@ -13,7 +13,7 @@ import ( var yamlNodeType = reflect.TypeOf(yaml.Node{}) -// open is overriden in tests. +// open is overridden in tests. var open = os.Open // A Spec represents a structure that yaml can be decoded into. The type is the @@ -43,7 +43,7 @@ func Register(kind string, spec *Spec) { // A Config represents a KNE deployment configuration. type Config struct { Path string // Path of the configuration file - Dir string // Absolute path of the diretory Path is in + Dir string // Absolute path of the directory Path is in Config interface{} // The configuration structure Deployment interface{} // Filled by Config.Decode @@ -179,7 +179,7 @@ func (c *Config) decode(v reflect.Value, path []string, tag reflect.StructTag) ( } case "spec": if sf.Type != yamlNodeType { - return fmt.Errorf("%s is not of type %v\n", strings.Join(append(path, sf.Name), "."), yamlNodeType) + return fmt.Errorf("%s is not of type %v", strings.Join(append(path, sf.Name), "."), yamlNodeType) } node := sv.Interface().(yaml.Node) spec = &node @@ -193,9 +193,9 @@ func (c *Config) decode(v reflect.Value, path []string, tag reflect.StructTag) ( switch { case kind == "" && spec == nil: case kind == "": - return fmt.Errorf("spec field without kind: %s\n", strings.Join(path, ".")) + return fmt.Errorf("spec field without kind: %s", strings.Join(path, ".")) case spec == nil: - return fmt.Errorf("kind field without spec: %s\n", strings.Join(path, ".")) + return fmt.Errorf("kind field without spec: %s", strings.Join(path, ".")) default: // kind and spec have been supplied.