Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

Commit 4d8b891

Browse files
jwendellistio-testing
jwendell
authored and
istio-testing
committed
Use RunE for cobra commands (#640)
This gives us more control of the output. In cases where we supply stdOut and stdErr (test cases), if we abort the execution upon an error, the caller does not have the chance to collect the output and print to user. By using `RunE` instead of `Run` we don't need to abort the execution and therefore break the workflow. By returning an error the caller can properly handle the error.
1 parent cc3b955 commit 4d8b891

9 files changed

+90
-94
lines changed

‎cmd/mesh/manifest-apply.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64,29 +64,29 @@ func manifestApplyCmd(rootArgs *rootArgs, maArgs *manifestApplyArgs) *cobra.Comm
6464
Short: "Generates and applies an Istio install manifest.",
6565
Long: "The apply subcommand generates an Istio install manifest and applies it to a cluster.",
6666
Args: cobra.ExactArgs(0),
67-
Run: func(cmd *cobra.Command, args []string) {
68-
// Passing cmd.OutOrStdXXX() allows capturing command output for e2e tests.
69-
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.OutOrStderr())
67+
RunE: func(cmd *cobra.Command, args []string) error {
68+
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.ErrOrStderr())
7069
// Warn users if they use `manifest apply` without any config args.
7170
if maArgs.inFilename == "" && len(maArgs.set) == 0 && !maArgs.skipConfirmation {
7271
if !confirm("This will install the default Istio profile into the cluster. Proceed? (y/N)", cmd.OutOrStdout()) {
7372
cmd.Print("Cancelled.\n")
7473
os.Exit(1)
7574
}
7675
}
77-
manifestApply(rootArgs, maArgs, l)
76+
return manifestApply(rootArgs, maArgs, l)
7877
}}
7978
}
8079

81-
func manifestApply(args *rootArgs, maArgs *manifestApplyArgs, l *logger) {
80+
func manifestApply(args *rootArgs, maArgs *manifestApplyArgs, l *logger) error {
8281
if err := configLogs(args.logToStdErr); err != nil {
83-
_, _ = fmt.Fprintf(os.Stderr, "Could not configure logs: %s", err)
84-
os.Exit(1)
82+
return fmt.Errorf("could not configure logs: %s", err)
8583
}
8684
if err := genApplyManifests(maArgs.set, maArgs.inFilename, maArgs.force, args.dryRun, args.verbose,
8785
maArgs.kubeConfigPath, maArgs.context, maArgs.readinessTimeout, l); err != nil {
88-
l.logAndFatalf("Failed to generate and apply manifests, error: %v", err)
86+
return fmt.Errorf("failed to generate and apply manifests, error: %v", err)
8987
}
88+
89+
return nil
9090
}
9191

9292
func confirm(msg string, writer io.Writer) bool {

‎cmd/mesh/manifest-diff.go

Lines changed: 19 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,13 @@ package mesh
1717
import (
1818
"fmt"
1919
"io/ioutil"
20-
"os"
2120
"path/filepath"
2221

2322
"istio.io/operator/pkg/compare"
2423

2524
"github.com/spf13/cobra"
2625

2726
"istio.io/operator/pkg/util"
28-
"istio.io/pkg/log"
2927
)
3028

3129
// YAMLSuffix is the suffix of a YAML file.
@@ -76,45 +74,44 @@ func manifestDiffCmd(rootArgs *rootArgs, diffArgs *manifestDiffArgs) *cobra.Comm
7674
}
7775
return nil
7876
},
79-
Run: func(cmd *cobra.Command, args []string) {
80-
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.OutOrStderr())
77+
RunE: func(cmd *cobra.Command, args []string) error {
8178
if diffArgs.compareDir {
82-
compareManifestsFromDirs(rootArgs, args[0], args[1], diffArgs.renameResources,
79+
return compareManifestsFromDirs(rootArgs, args[0], args[1], diffArgs.renameResources,
8380
diffArgs.selectResources, diffArgs.ignoreResources)
84-
} else {
85-
compareManifestsFromFiles(rootArgs, args, diffArgs.renameResources,
86-
diffArgs.selectResources, diffArgs.ignoreResources, l)
8781
}
82+
83+
return compareManifestsFromFiles(rootArgs, args, diffArgs.renameResources,
84+
diffArgs.selectResources, diffArgs.ignoreResources)
8885
}}
8986
return cmd
9087
}
9188

9289
//compareManifestsFromFiles compares two manifest files
9390
func compareManifestsFromFiles(rootArgs *rootArgs, args []string,
94-
renameResources, selectResources, ignoreResources string, l *logger) {
91+
renameResources, selectResources, ignoreResources string) error {
9592
initLogsOrExit(rootArgs)
9693

9794
a, err := ioutil.ReadFile(args[0])
9895
if err != nil {
99-
l.logAndFatal(fmt.Sprintf("Could not read %q: %v\n", args[0], err.Error()))
96+
return fmt.Errorf("could not read %q: %v", args[0], err)
10097
}
10198
b, err := ioutil.ReadFile(args[1])
10299
if err != nil {
103-
l.logAndFatal(fmt.Sprintf("Could not read %q: %v\n", args[1], err.Error()))
100+
return fmt.Errorf("could not read %q: %v", args[1], err)
104101
}
105102

106103
diff, err := compare.ManifestDiffWithRenameSelectIgnore(string(a), string(b), renameResources, selectResources,
107104
ignoreResources, rootArgs.verbose)
108105
if err != nil {
109-
log.Error(err.Error())
110-
os.Exit(1)
106+
return err
111107
}
112108
if diff == "" {
113109
fmt.Println("Manifests are identical")
114110
} else {
115-
fmt.Printf("Differences of manifests are:\n%s", diff)
116-
os.Exit(1)
111+
return fmt.Errorf("differences of manifests are:\n%s", diff)
117112
}
113+
114+
return nil
118115
}
119116

120117
func yamlFileFilter(path string) bool {
@@ -123,30 +120,28 @@ func yamlFileFilter(path string) bool {
123120

124121
//compareManifestsFromDirs compares manifests from two directories
125122
func compareManifestsFromDirs(rootArgs *rootArgs, dirName1, dirName2,
126-
renameResources, selectResources, ignoreResources string) {
123+
renameResources, selectResources, ignoreResources string) error {
127124
initLogsOrExit(rootArgs)
128125

129126
mf1, err := util.ReadFilesWithFilter(dirName1, yamlFileFilter)
130127
if err != nil {
131-
log.Error(err.Error())
132-
os.Exit(1)
128+
return err
133129
}
134130
mf2, err := util.ReadFilesWithFilter(dirName2, yamlFileFilter)
135131
if err != nil {
136-
log.Error(err.Error())
137-
os.Exit(1)
132+
return err
138133
}
139134

140135
diff, err := compare.ManifestDiffWithRenameSelectIgnore(mf1, mf2, renameResources, selectResources,
141136
ignoreResources, rootArgs.verbose)
142137
if err != nil {
143-
log.Error(err.Error())
144-
os.Exit(1)
138+
return err
145139
}
146140
if diff == "" {
147141
fmt.Println("Manifests are identical")
148142
} else {
149-
fmt.Printf("Differences of manifests are:\n%s", diff)
150-
os.Exit(1)
143+
return fmt.Errorf("differences of manifests are:\n%s", diff)
151144
}
145+
146+
return nil
152147
}

‎cmd/mesh/manifest-generate.go

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,26 +55,25 @@ func manifestGenerateCmd(rootArgs *rootArgs, mgArgs *manifestGenerateArgs) *cobr
5555
}
5656
return nil
5757
},
58-
Run: func(cmd *cobra.Command, args []string) {
59-
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.OutOrStderr())
60-
manifestGenerate(rootArgs, mgArgs, l)
58+
RunE: func(cmd *cobra.Command, args []string) error {
59+
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.ErrOrStderr())
60+
return manifestGenerate(rootArgs, mgArgs, l)
6161
}}
6262

6363
}
6464

65-
func manifestGenerate(args *rootArgs, mgArgs *manifestGenerateArgs, l *logger) {
65+
func manifestGenerate(args *rootArgs, mgArgs *manifestGenerateArgs, l *logger) error {
6666
if err := configLogs(args.logToStdErr); err != nil {
67-
_, _ = fmt.Fprintf(os.Stderr, "Could not configure logs: %s", err)
68-
os.Exit(1)
67+
return fmt.Errorf("could not configure logs: %s", err)
6968
}
7069

7170
overlayFromSet, err := makeTreeFromSetList(mgArgs.set, mgArgs.force, l)
7271
if err != nil {
73-
l.logAndFatal(err.Error())
72+
return err
7473
}
7574
manifests, err := genManifests(mgArgs.inFilename, overlayFromSet, mgArgs.force, l)
7675
if err != nil {
77-
l.logAndFatal(err.Error())
76+
return err
7877
}
7978

8079
if mgArgs.outFilename == "" {
@@ -83,12 +82,14 @@ func manifestGenerate(args *rootArgs, mgArgs *manifestGenerateArgs, l *logger) {
8382
}
8483
} else {
8584
if err := os.MkdirAll(mgArgs.outFilename, os.ModePerm); err != nil {
86-
l.logAndFatal(err.Error())
85+
return err
8786
}
8887
if err := manifest.RenderToDir(manifests, mgArgs.outFilename, args.dryRun); err != nil {
89-
l.logAndFatal(err.Error())
88+
return err
9089
}
9190
}
91+
92+
return nil
9293
}
9394

9495
func orderedManifests(mm name.ManifestMap) []string {

‎cmd/mesh/manifest-migrate.go

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,14 @@ func manifestMigrateCmd(rootArgs *rootArgs, mmArgs *manifestMigrateArgs) *cobra.
5555
}
5656
return nil
5757
},
58-
Run: func(cmd *cobra.Command, args []string) {
59-
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.OutOrStderr())
58+
RunE: func(cmd *cobra.Command, args []string) error {
59+
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.ErrOrStderr())
60+
6061
if len(args) == 0 {
61-
migrateFromClusterConfig(rootArgs, mmArgs, l)
62-
} else {
63-
migrateFromFiles(rootArgs, args, l)
62+
return migrateFromClusterConfig(rootArgs, mmArgs, l)
6463
}
64+
65+
return migrateFromFiles(rootArgs, args, l)
6566
}}
6667
}
6768

@@ -70,47 +71,49 @@ func valueFileFilter(path string) bool {
7071
}
7172

7273
// migrateFromFiles handles migration for local values.yaml files
73-
func migrateFromFiles(rootArgs *rootArgs, args []string, l *logger) {
74+
func migrateFromFiles(rootArgs *rootArgs, args []string, l *logger) error {
7475
initLogsOrExit(rootArgs)
7576
value, err := util.ReadFilesWithFilter(args[0], valueFileFilter)
7677
if err != nil {
77-
l.logAndFatal(err.Error())
78+
return err
7879
}
7980
if value == "" {
8081
l.logAndPrint("no valid value.yaml file specified")
81-
return
82+
return nil
8283
}
83-
translateFunc([]byte(value), l)
84+
return translateFunc([]byte(value), l)
8485
}
8586

8687
// translateFunc translates the input values and output the result
87-
func translateFunc(values []byte, l *logger) {
88+
func translateFunc(values []byte, l *logger) error {
8889
ts, err := translate.NewReverseTranslator(binversion.OperatorBinaryVersion.MinorVersion)
8990
if err != nil {
90-
l.logAndFatal("error creating values.yaml translator: ", err.Error())
91+
return fmt.Errorf("error creating values.yaml translator: %s", err)
9192
}
9293

9394
isCPSpec, err := ts.TranslateFromValueToSpec(values)
9495
if err != nil {
95-
l.logAndFatal("error translating values.yaml: ", err.Error())
96+
return fmt.Errorf("error translating values.yaml: %s", err)
9697
}
9798
isCP := &v1alpha2.IstioControlPlane{Spec: isCPSpec, Kind: "IstioControlPlane", ApiVersion: "install.istio.io/v1alpha2"}
9899

99100
ms := jsonpb.Marshaler{}
100101
gotString, err := ms.MarshalToString(isCP)
101102
if err != nil {
102-
l.logAndFatal("error marshaling translated IstioControlPlane: ", err.Error())
103+
return fmt.Errorf("error marshaling translated IstioControlPlane: %s", err)
103104
}
104105

105106
isCPYaml, _ := yaml.JSONToYAML([]byte(gotString))
106107
if err != nil {
107-
l.logAndFatal("error converting JSON: ", gotString, "\n", err.Error())
108+
return fmt.Errorf("error converting JSON: %s\n%s", gotString, err)
108109
}
110+
109111
l.print(string(isCPYaml) + "\n")
112+
return nil
110113
}
111114

112115
// migrateFromClusterConfig handles migration for in cluster config.
113-
func migrateFromClusterConfig(rootArgs *rootArgs, mmArgs *manifestMigrateArgs, l *logger) {
116+
func migrateFromClusterConfig(rootArgs *rootArgs, mmArgs *manifestMigrateArgs, l *logger) error {
114117
initLogsOrExit(rootArgs)
115118

116119
l.logAndPrint("translating in cluster specs\n")
@@ -119,7 +122,7 @@ func migrateFromClusterConfig(rootArgs *rootArgs, mmArgs *manifestMigrateArgs, l
119122
output, stderr, err := c.GetConfig("", "", "istio-sidecar-injector",
120123
mmArgs.namespace, "jsonpath='{.data.values}'")
121124
if err != nil {
122-
l.logAndFatal(err.Error())
125+
return err
123126
}
124127
if stderr != "" {
125128
l.logAndPrint("error: ", stderr, "\n")
@@ -130,11 +133,12 @@ func migrateFromClusterConfig(rootArgs *rootArgs, mmArgs *manifestMigrateArgs, l
130133
}
131134
err = json.Unmarshal([]byte(output), &value)
132135
if err != nil {
133-
l.logAndFatal("error unmarshaling JSON to untyped map ", err.Error())
136+
return fmt.Errorf("error unmarshaling JSON to untyped map %s", err)
134137
}
135138
res, err := yaml.Marshal(value)
136139
if err != nil {
137-
l.logAndFatal("error marshaling untyped map to YAML: ", err.Error())
140+
return fmt.Errorf("error marshaling untyped map to YAML: %s", err)
138141
}
139-
translateFunc(res, l)
142+
143+
return translateFunc(res, l)
140144
}

‎cmd/mesh/manifest-versions.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,18 +55,18 @@ func manifestVersionsCmd(rootArgs *rootArgs, versionsArgs *manifestVersionsArgs)
5555
}
5656
return nil
5757
},
58-
Run: func(cmd *cobra.Command, args []string) {
59-
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.OutOrStderr())
60-
manifestVersions(rootArgs, versionsArgs, l)
58+
RunE: func(cmd *cobra.Command, args []string) error {
59+
l := newLogger(rootArgs.logToStdErr, cmd.OutOrStdout(), cmd.ErrOrStderr())
60+
return manifestVersions(rootArgs, versionsArgs, l)
6161
}}
6262
}
6363

64-
func manifestVersions(args *rootArgs, mvArgs *manifestVersionsArgs, l *logger) {
64+
func manifestVersions(args *rootArgs, mvArgs *manifestVersionsArgs, l *logger) error {
6565
initLogsOrExit(args)
6666

6767
myVersionMap, err := getVersionCompatibleMap(mvArgs.versionsURI, binversion.OperatorBinaryGoVersion, l)
6868
if err != nil {
69-
l.logAndFatalf("Failed to retrieve version map, error: %v", err)
69+
return fmt.Errorf("failed to retrieve version map, error: %v", err)
7070
}
7171

7272
fmt.Print("\nOperator version is ", binversion.OperatorBinaryGoVersion.String(), ".\n\n")
@@ -79,6 +79,8 @@ func manifestVersions(args *rootArgs, mvArgs *manifestVersionsArgs, l *logger) {
7979
fmt.Printf(" %s\n", v.String())
8080
}
8181
fmt.Println()
82+
83+
return nil
8284
}
8385

8486
func getVersionCompatibleMap(versionsURI string, binVersion *goversion.Version,

0 commit comments

Comments
 (0)