From d6e09505f5c484aed7cc66d69d932ad764513a5d Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 17:32:05 -0800 Subject: [PATCH 1/9] Disable logging by default The log output is not of interest to the ordinary users, so it should only be enabled when one of the logging configuration environment variables is set. --- configuration/configuration.go | 6 ++++-- configuration/configuration_test.go | 1 - configuration/defaults.go | 5 +++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/configuration/configuration.go b/configuration/configuration.go index 2e373a4a4..a9554d235 100644 --- a/configuration/configuration.go +++ b/configuration/configuration.go @@ -56,12 +56,15 @@ func Initialize(flags *pflag.FlagSet, projectPaths []string) error { } } + logrus.SetOutput(defaultLogOutput) + if logFormatString, ok := os.LookupEnv("ARDUINO_CHECK_LOG_FORMAT"); ok { logFormat, err := logFormatFromString(logFormatString) if err != nil { return fmt.Errorf("--log-format flag value %s not valid", logFormatString) } logrus.SetFormatter(logFormat) + logrus.SetOutput(os.Stderr) // Enable log output. } if logLevelString, ok := os.LookupEnv("ARDUINO_CHECK_LOG_LEVEL"); ok { @@ -70,8 +73,7 @@ func Initialize(flags *pflag.FlagSet, projectPaths []string) error { return fmt.Errorf("--log-level flag value %s not valid", logLevelString) } logrus.SetLevel(logLevel) - } else { - logrus.SetLevel(defaultLogLevel) + logrus.SetOutput(os.Stderr) // Enable log output. } superprojectTypeFilterString, _ := flags.GetString("project-type") diff --git a/configuration/configuration_test.go b/configuration/configuration_test.go index 9ea49f30b..b44e0b6f1 100644 --- a/configuration/configuration_test.go +++ b/configuration/configuration_test.go @@ -120,7 +120,6 @@ func TestInitializeLogFormat(t *testing.T) { func TestInitializeLogLevel(t *testing.T) { require.Nil(t, Initialize(test.ConfigurationFlags(), projectPaths)) - assert.Equal(t, defaultLogLevel, logrus.GetLevel(), "Default level") os.Setenv("ARDUINO_CHECK_LOG_LEVEL", "foo") assert.Error(t, Initialize(test.ConfigurationFlags(), projectPaths), "Invalid level") diff --git a/configuration/defaults.go b/configuration/defaults.go index a4cc1e3b2..7f2f3d5d2 100644 --- a/configuration/defaults.go +++ b/configuration/defaults.go @@ -18,9 +18,10 @@ package configuration // The default configuration settings. import ( + "io/ioutil" + "github.com/arduino/arduino-check/configuration/checkmode" "github.com/arduino/arduino-check/project/projecttype" - "github.com/sirupsen/logrus" ) // Default check modes for each superproject type. @@ -60,4 +61,4 @@ var defaultCheckModes = map[projecttype.Type]map[checkmode.Type]bool{ }, } -var defaultLogLevel = logrus.FatalLevel +var defaultLogOutput = ioutil.Discard // Default to no log output. From e4852b105e56c84dfc913d7e11e4dfc1f7a92cc6 Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 17:38:11 -0800 Subject: [PATCH 2/9] Make feedback.Error*() print to stderr --- result/feedback/feedback.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/result/feedback/feedback.go b/result/feedback/feedback.go index bf603aa14..17e1b87a8 100644 --- a/result/feedback/feedback.go +++ b/result/feedback/feedback.go @@ -18,6 +18,7 @@ package feedback import ( "fmt" + "os" "github.com/arduino/arduino-check/configuration" "github.com/arduino/arduino-check/result/outputformat" @@ -55,6 +56,6 @@ func Errorf(format string, v ...interface{}) { // Error behaves like fmt.Print but also logs the error. func Error(errorMessage string) { - fmt.Printf(errorMessage) + fmt.Fprint(os.Stderr, errorMessage) logrus.Error(fmt.Sprint(errorMessage)) } From 2145f352ca4756ea60943fcf7a3fbd9dc0071d25 Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 17:39:45 -0800 Subject: [PATCH 3/9] Make feedback.Error*() add a newline to the output The error must always be printed with a newline, otherwise the log will be appended to the output. This follows the convention set by the Arduino CLI feedback package. --- result/feedback/feedback.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/result/feedback/feedback.go b/result/feedback/feedback.go index 17e1b87a8..df033d52c 100644 --- a/result/feedback/feedback.go +++ b/result/feedback/feedback.go @@ -49,13 +49,13 @@ func Print(message string) { } } -// Errorf behaves like fmt.Printf but also logs the error. +// Errorf behaves like fmt.Printf but adds a newline and also logs the error. func Errorf(format string, v ...interface{}) { Error(fmt.Sprintf(format, v...)) } -// Error behaves like fmt.Print but also logs the error. +// Error behaves like fmt.Print but adds a newline and also logs the error. func Error(errorMessage string) { - fmt.Fprint(os.Stderr, errorMessage) + fmt.Fprintln(os.Stderr, errorMessage) logrus.Error(fmt.Sprint(errorMessage)) } From eb96078420f754a52bcf0812edf4ab29cfe7f351 Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 17:45:14 -0800 Subject: [PATCH 4/9] Change parameter of feedback package functions to variadic interface Even though not currently needed, this brings consistency with the standard fmt API. --- result/feedback/feedback.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/result/feedback/feedback.go b/result/feedback/feedback.go index df033d52c..770c5bcce 100644 --- a/result/feedback/feedback.go +++ b/result/feedback/feedback.go @@ -31,9 +31,9 @@ func VerbosePrintf(format string, v ...interface{}) { } // VerbosePrint behaves like Print but only prints when verbosity is enabled. -func VerbosePrint(message string) { +func VerbosePrint(v ...interface{}) { if configuration.Verbose() && (configuration.OutputFormat() == outputformat.Text) { - Printf(message) + Print(v...) } } @@ -43,9 +43,9 @@ func Printf(format string, v ...interface{}) { } // Print behaves like fmt.Print but only prints when output format is set to `text`. -func Print(message string) { +func Print(v ...interface{}) { if configuration.OutputFormat() == outputformat.Text { - fmt.Printf(message) + fmt.Print(v...) } } @@ -55,7 +55,7 @@ func Errorf(format string, v ...interface{}) { } // Error behaves like fmt.Print but adds a newline and also logs the error. -func Error(errorMessage string) { - fmt.Fprintln(os.Stderr, errorMessage) - logrus.Error(fmt.Sprint(errorMessage)) +func Error(v ...interface{}) { + fmt.Fprintln(os.Stderr, v...) + logrus.Error(fmt.Sprint(v...)) } From 3e6d2f69213ef4855858f23dcb1a8a69e0792914 Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 17:47:55 -0800 Subject: [PATCH 5/9] Add *ln functions to feedback package --- result/feedback/feedback.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/result/feedback/feedback.go b/result/feedback/feedback.go index 770c5bcce..da79d8152 100644 --- a/result/feedback/feedback.go +++ b/result/feedback/feedback.go @@ -25,6 +25,12 @@ import ( "github.com/sirupsen/logrus" ) +// VerbosePrintln behaves like Println but only prints when verbosity is enabled. +func VerbosePrintln(v ...interface{}) { + VerbosePrint(v...) + VerbosePrint("\n") +} + // VerbosePrintf behaves like Printf but only prints when verbosity is enabled. func VerbosePrintf(format string, v ...interface{}) { VerbosePrint(fmt.Sprintf(format, v...)) @@ -37,6 +43,12 @@ func VerbosePrint(v ...interface{}) { } } +// Println behaves like fmt.Println but only prints when output format is set to `text`. +func Println(v ...interface{}) { + Print(v...) + Print("\n") +} + // Printf behaves like fmt.Printf but only prints when output format is set to `text`. func Printf(format string, v ...interface{}) { Print(fmt.Sprintf(format, v...)) From f636b764fcf34dd6767b7f9adcd1886b2c2a87aa Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 18:03:27 -0800 Subject: [PATCH 6/9] Add trailing newlines at the point of output The previous approach of adding trailing newlines when the report text was generated made it difficult to keep track of how the output was formatted. --- check/check.go | 6 +++--- command/command.go | 2 +- result/result.go | 8 ++++---- result/result_test.go | 8 ++++---- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/check/check.go b/check/check.go index a659622d3..ef12c3f40 100644 --- a/check/check.go +++ b/check/check.go @@ -33,7 +33,7 @@ import ( // RunChecks runs all checks for the given project and outputs the results. func RunChecks(project project.Type) { - feedback.Printf("Checking %s in %s\n", project.ProjectType, project.Path) + feedback.Printf("\nChecking %s in %s\n", project.ProjectType, project.Path) checkdata.Initialize(project, configuration.SchemasPath()) @@ -55,7 +55,7 @@ func RunChecks(project project.Type) { checkResult, checkOutput := checkConfiguration.CheckFunction() reportText := result.Results.Record(project, checkConfiguration, checkResult, checkOutput) if (checkResult == checkresult.Fail) || configuration.Verbose() { - feedback.Print(reportText) + feedback.Println(reportText) } } @@ -63,7 +63,7 @@ func RunChecks(project project.Type) { result.Results.AddProjectSummary(project) // Print the project check results summary. - feedback.Print(result.Results.ProjectSummaryText(project)) + feedback.Printf("\n%s\n", result.Results.ProjectSummaryText(project)) } // shouldRun returns whether a given check should be run for the given project under the current tool configuration. diff --git a/command/command.go b/command/command.go index cb6826b9d..c6f14c192 100644 --- a/command/command.go +++ b/command/command.go @@ -75,7 +75,7 @@ func ArduinoCheck(rootCommand *cobra.Command, cliArguments []string) { if configuration.OutputFormat() == outputformat.Text { if len(projects) > 1 { // There are multiple projects, print the summary of check results for all projects. - fmt.Print(result.Results.SummaryText()) + fmt.Printf("\n%s\n", result.Results.SummaryText()) } } else { // Print the complete JSON formatted report. diff --git a/result/result.go b/result/result.go index 61ff2709b..8d901548f 100644 --- a/result/result.go +++ b/result/result.go @@ -99,7 +99,7 @@ func (results *Type) Record(checkedProject project.Type, checkConfiguration chec os.Exit(1) } - summaryText := fmt.Sprintf("Check %s result: %s\n", checkConfiguration.ID, checkResult) + summaryText := fmt.Sprintf("Check %s result: %s", checkConfiguration.ID, checkResult) checkMessage := "" if checkResult == checkresult.Fail { @@ -112,7 +112,7 @@ func (results *Type) Record(checkedProject project.Type, checkConfiguration chec // Add explanation of check result if present. if checkMessage != "" { - summaryText += fmt.Sprintf("%s: %s\n", checkLevel, checkMessage) + summaryText += fmt.Sprintf("\n%s: %s", checkLevel, checkMessage) } reportExists, projectReportIndex := results.getProjectReportIndex(checkedProject.Path) @@ -187,7 +187,7 @@ func (results Type) ProjectSummaryText(checkedProject project.Type) string { } projectSummaryReport := results.Projects[projectReportIndex].Summary - return fmt.Sprintf("\nFinished checking project. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v\n\n", projectSummaryReport.WarningCount, projectSummaryReport.ErrorCount, projectSummaryReport.Pass) + return fmt.Sprintf("Finished checking project. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v", projectSummaryReport.WarningCount, projectSummaryReport.ErrorCount, projectSummaryReport.Pass) } // AddSummary summarizes the check results for all projects and adds it to the report. @@ -212,7 +212,7 @@ func (results *Type) AddSummary() { // SummaryText returns a text summary of the cumulative check results. func (results Type) SummaryText() string { - return fmt.Sprintf("Finished checking projects. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v\n", results.Summary.WarningCount, results.Summary.ErrorCount, results.Summary.Pass) + return fmt.Sprintf("Finished checking projects. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v", results.Summary.WarningCount, results.Summary.ErrorCount, results.Summary.Pass) } // JSONReport returns a JSON formatted report of checks on all projects. diff --git a/result/result_test.go b/result/result_test.go index bab43d5ac..3892cac40 100644 --- a/result/result_test.go +++ b/result/result_test.go @@ -75,9 +75,9 @@ func TestRecord(t *testing.T) { checkConfiguration := checkconfigurations.Configurations()[0] checkOutput := "foo" summaryText := results.Record(checkedProject, checkConfiguration, checkresult.Fail, checkOutput) - assert.Equal(t, fmt.Sprintf("Check %s result: %s\n%s: %s\n", checkConfiguration.ID, checkresult.Fail, checklevel.Error, message(checkConfiguration.MessageTemplate, checkOutput)), summaryText) + assert.Equal(t, fmt.Sprintf("Check %s result: %s\n%s: %s", checkConfiguration.ID, checkresult.Fail, checklevel.Error, message(checkConfiguration.MessageTemplate, checkOutput)), summaryText) summaryText = results.Record(checkedProject, checkConfiguration, checkresult.NotRun, checkOutput) - assert.Equal(t, fmt.Sprintf("Check %s result: %s\n%s: %s\n", checkConfiguration.ID, checkresult.NotRun, checklevel.Notice, checkOutput), summaryText, "Non-fail result should not use message") + assert.Equal(t, fmt.Sprintf("Check %s result: %s\n%s: %s", checkConfiguration.ID, checkresult.NotRun, checklevel.Notice, checkOutput), summaryText, "Non-fail result should not use message") summaryText = results.Record(checkedProject, checkConfiguration, checkresult.Pass, "") assert.Equal(t, "", "", summaryText, "Non-failure result with no check function output should result in an empty summary") @@ -198,7 +198,7 @@ func TestAddProjectSummary(t *testing.T) { assert.Equal(t, testTable.expectedPass, results.Projects[0].Summary.Pass) assert.Equal(t, testTable.expectedWarningCount, results.Projects[0].Summary.WarningCount) assert.Equal(t, testTable.expectedErrorCount, results.Projects[0].Summary.ErrorCount) - assert.Equal(t, fmt.Sprintf("\nFinished checking project. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v\n\n", testTable.expectedWarningCount, testTable.expectedErrorCount, testTable.expectedPass), results.ProjectSummaryText(checkedProject)) + assert.Equal(t, fmt.Sprintf("Finished checking project. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v", testTable.expectedWarningCount, testTable.expectedErrorCount, testTable.expectedPass), results.ProjectSummaryText(checkedProject)) } } @@ -281,7 +281,7 @@ func TestAddSummary(t *testing.T) { assert.Equal(t, testTable.expectedPass, results.Passed()) assert.Equal(t, testTable.expectedWarningCount, results.Summary.WarningCount) assert.Equal(t, testTable.expectedErrorCount, results.Summary.ErrorCount) - assert.Equal(t, fmt.Sprintf("Finished checking projects. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v\n", testTable.expectedWarningCount, testTable.expectedErrorCount, testTable.expectedPass), results.SummaryText()) + assert.Equal(t, fmt.Sprintf("Finished checking projects. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v", testTable.expectedWarningCount, testTable.expectedErrorCount, testTable.expectedPass), results.SummaryText()) } } From 5421e9f5995f571ba075f75f55d733082a3eb549 Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 18:06:21 -0800 Subject: [PATCH 7/9] Move project summary report output out of check.RunChecks() It's better for this function to be exclusively for the running of checks. It is more sensible for the project related output to be handled at a higher level. --- check/check.go | 6 ------ command/command.go | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/check/check.go b/check/check.go index ef12c3f40..796e32172 100644 --- a/check/check.go +++ b/check/check.go @@ -58,12 +58,6 @@ func RunChecks(project project.Type) { feedback.Println(reportText) } } - - // Checks are finished for this project, so summarize its check results in the report. - result.Results.AddProjectSummary(project) - - // Print the project check results summary. - feedback.Printf("\n%s\n", result.Results.ProjectSummaryText(project)) } // shouldRun returns whether a given check should be run for the given project under the current tool configuration. diff --git a/command/command.go b/command/command.go index c6f14c192..13e453b10 100644 --- a/command/command.go +++ b/command/command.go @@ -67,6 +67,12 @@ func ArduinoCheck(rootCommand *cobra.Command, cliArguments []string) { for _, project := range projects { check.RunChecks(project) + + // Checks are finished for this project, so summarize its check results in the report. + result.Results.AddProjectSummary(project) + + // Print the project check results summary. + feedback.Printf("\n%s\n", result.Results.ProjectSummaryText(project)) } // All projects have been checked, so summarize their check results in the report. From d6701acf7438473f6be6591e951222ced1d3b8bb Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 18:08:36 -0800 Subject: [PATCH 8/9] Always panic on internal failures These uses of `feedback.Error()` in the event of internal failures was a departure from the convention established in the rest of the code of using `feedback.Error()` only for failures resulting from external issues, and panic() for those resulting from internal issues (in which case the traceback is useful). --- check/check.go | 4 +--- result/result.go | 5 +---- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/check/check.go b/check/check.go index 796e32172..c98a391ef 100644 --- a/check/check.go +++ b/check/check.go @@ -18,7 +18,6 @@ package check import ( "fmt" - "os" "github.com/arduino/arduino-check/check/checkconfigurations" "github.com/arduino/arduino-check/check/checkdata" @@ -40,8 +39,7 @@ func RunChecks(project project.Type) { for _, checkConfiguration := range checkconfigurations.Configurations() { runCheck, err := shouldRun(checkConfiguration, project) if err != nil { - feedback.Errorf("Error while determining whether to run check: %v", err) - os.Exit(1) + panic(err) } if !runCheck { diff --git a/result/result.go b/result/result.go index 8d901548f..c8b72b4fc 100644 --- a/result/result.go +++ b/result/result.go @@ -21,7 +21,6 @@ import ( "encoding/json" "fmt" "html/template" - "os" "github.com/arduino/arduino-check/check/checkconfigurations" "github.com/arduino/arduino-check/check/checklevel" @@ -29,7 +28,6 @@ import ( "github.com/arduino/arduino-check/configuration" "github.com/arduino/arduino-check/configuration/checkmode" "github.com/arduino/arduino-check/project" - "github.com/arduino/arduino-check/result/feedback" "github.com/arduino/go-paths-helper" ) @@ -95,8 +93,7 @@ func (results *Type) Initialize() { func (results *Type) Record(checkedProject project.Type, checkConfiguration checkconfigurations.Type, checkResult checkresult.Type, checkOutput string) string { checkLevel, err := checklevel.CheckLevel(checkConfiguration, checkResult) if err != nil { - feedback.Errorf("Error while determining check level: %v", err) - os.Exit(1) + panic(fmt.Errorf("Error while determining check level: %v", err)) } summaryText := fmt.Sprintf("Check %s result: %s", checkConfiguration.ID, checkResult) From 23d32fd1a16193bf00e2d08eb3ad4f6977e3f132 Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 10 Dec 2020 18:34:46 -0800 Subject: [PATCH 9/9] Move log output to after error check This log output is only meaningful if there wasn't an error. --- check/checkdata/schema/parsevalidationresult.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/check/checkdata/schema/parsevalidationresult.go b/check/checkdata/schema/parsevalidationresult.go index f358ef543..b1db7af51 100644 --- a/check/checkdata/schema/parsevalidationresult.go +++ b/check/checkdata/schema/parsevalidationresult.go @@ -189,10 +189,10 @@ func validationErrorSchemaPointerValueMatch( schemasPath *paths.Path, ) bool { marshalledSchemaPointerValue, err := json.Marshal(schemaPointerValue(schemaURL, schemaPointer, schemasPath)) - logrus.Tracef("Checking schema pointer value: %s match with regexp: %s", marshalledSchemaPointerValue, schemaPointerValueRegexp) if err != nil { panic(err) } + logrus.Tracef("Checking schema pointer value: %s match with regexp: %s", marshalledSchemaPointerValue, schemaPointerValueRegexp) return schemaPointerValueRegexp.Match(marshalledSchemaPointerValue) }