From 58bbb522d1d91c00dbfee9909ac8e27b68bcbcd8 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Sat, 2 Mar 2024 14:40:24 -0300 Subject: [PATCH] fix: use logger only for github messages Signed-off-by: Felipe Zipitria --- cmd/flag_types.go | 2 +- cmd/regex_compare.go | 9 ++- cmd/regex_format.go | 17 +----- logger/logger.go | 35 ++++++------ logger/logger_test.go | 124 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 148 insertions(+), 39 deletions(-) create mode 100644 logger/logger_test.go diff --git a/cmd/flag_types.go b/cmd/flag_types.go index af1a00e..2d26e13 100644 --- a/cmd/flag_types.go +++ b/cmd/flag_types.go @@ -41,7 +41,7 @@ func (o *outputType) String() string { func (o *outputType) Set(value string) error { switch value { case string(gitHub): - logger = loggerConfig.SetGithubOutput() + logger = loggerConfig.SetGithubOutput(os.Stdout) fallthrough case string(text): rootValues.output = outputType(value) diff --git a/cmd/regex_compare.go b/cmd/regex_compare.go index 12c7bef..133f480 100644 --- a/cmd/regex_compare.go +++ b/cmd/regex_compare.go @@ -132,8 +132,8 @@ func performCompare(processAll bool, ctx *processors.Context) error { if err != nil && len(chainOffsetString) > 0 { return errors.New("failed to match chain offset. Value must not be larger than 255") } - regex := runAssemble(filePath, ctx) - err = processRegexForCompare(id, uint8(chainOffset), regex, ctx) + rx := runAssemble(filePath, ctx) + err = processRegexForCompare(id, uint8(chainOffset), rx, ctx) if err != nil && errors.Is(err, &ComparisonError{}) { failed = true return nil @@ -148,9 +148,8 @@ func performCompare(processAll bool, ctx *processors.Context) error { if err != nil { logger.Fatal().Err(err).Msg("Failed to compare expressions") } - if failed && rootValues.output == gitHub { - fmt.Println("::error::All rules need to be up to date.", - "Please run `crs-toolchain regex update --all`") + if failed { + logger.Error().Msg("All rules need to be up to date. Please run `crs-toolchain regex update --all`") return &ComparisonError{} } } else { diff --git a/cmd/regex_format.go b/cmd/regex_format.go index 50528df..d3f9c92 100644 --- a/cmd/regex_format.go +++ b/cmd/regex_format.go @@ -24,7 +24,6 @@ import ( const ( regexAssemblyStandardHeader = "##! Please refer to the documentation at\n##! https://coreruleset.org/docs/development/regex_assembly/.\n" - showCharsAround = 20 ) // formatCmd represents the generate command @@ -149,10 +148,7 @@ func processAll(ctxt *processors.Context, checkOnly bool) error { return err } if failed { - if rootValues.output == gitHub { - fmt.Println("::error::All assembly files need to be properly formatted.", - "Please run `crs-toolchain regex format --all`") - } + logger.Error().Msg("All assembly files need to be properly formatted. Please run `crs-toolchain regex format --all`") return &UnformattedFileError{} } return nil @@ -160,7 +156,6 @@ func processAll(ctxt *processors.Context, checkOnly bool) error { func processFile(filePath string, ctxt *processors.Context, checkOnly bool) error { var processFileError error - message := "" filename := path.Base(filePath) logger.Info().Msgf("Processing %s", filename) file, err := os.Open(filePath) @@ -213,8 +208,7 @@ func processFile(filePath string, ctxt *processors.Context, checkOnly bool) erro } equalContent := bytes.Equal(currentContents, newContents) if !equalContent || foundUppercase { - message = formatMessage(fmt.Sprintf("File %s not properly formatted", filePath)) - fmt.Println(message) + logger.Error().Msgf("File %s not properly formatted", filePath) processFileError = &UnformattedFileError{filePath: filePath} } } else { @@ -281,13 +275,6 @@ func processLine(line []byte, indent int) ([]byte, int, error) { return trimmedLine, nextIndent, nil } -func formatMessage(message string) string { - if rootValues.output == gitHub { - message = fmt.Sprintf("::warning ::%s\n", message) - } - return message -} - func formatEndOfFile(lines []string) []string { eof := len(lines) - 1 if eof < 0 { diff --git a/logger/logger.go b/logger/logger.go index 66a2a19..a8eab7f 100644 --- a/logger/logger.go +++ b/logger/logger.go @@ -5,6 +5,7 @@ package logger import ( "fmt" + "io" "os" "github.com/rs/zerolog" @@ -13,31 +14,23 @@ import ( const DefaultLogLevel zerolog.Level = zerolog.InfoLevel -var consoleOutput = zerolog.ConsoleWriter{Out: os.Stderr, TimeFormat: "03:04:05"} - func init() { - log.Logger = log.Output(consoleOutput).With().Caller().Logger() + log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr, TimeFormat: "03:04:05"}).With().Caller().Logger() zerolog.SetGlobalLevel(DefaultLogLevel) } -// SetGithubOutput changes the standard logging format to be compatible with GitHub's. -// See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#example-creating-an-annotation-for-an-error -// Levels on github are: -// - debug, notice, error, warning -// Another possibility is to add the following strings between the level and the message: -// file={name},line={line},endLine={endLine},title={title} -func SetGithubOutput() zerolog.Logger { - // the following formatlevel loosely translates from posix levels to github levels - consoleOutput.FormatLevel = func(i interface{}) string { +func SetGithubOutput(w io.Writer) zerolog.Logger { + ghOutput := zerolog.ConsoleWriter{Out: w, TimeFormat: "03:04:05"} + ghOutput.FormatLevel = func(i interface{}) string { var l string if ll, ok := i.(string); ok { switch ll { case zerolog.LevelTraceValue, zerolog.LevelDebugValue: l = "debug" case zerolog.LevelInfoValue: - l = "notice " + l = "notice" case zerolog.LevelWarnValue: - l = "warn " + l = "warn" case zerolog.LevelErrorValue, zerolog.LevelFatalValue, zerolog.LevelPanicValue: l = "error " default: @@ -50,9 +43,15 @@ func SetGithubOutput() zerolog.Logger { } return fmt.Sprintf("::%s", l) } - consoleOutput.FormatMessage = func(i interface{}) string { - return fmt.Sprintf("::%s", i) + ghOutput.FormatMessage = func(i interface{}) string { + return fmt.Sprintf("::%s\n", i) + } + ghOutput.PartsExclude = []string{zerolog.TimestampFieldName, zerolog.CallerFieldName} + ghOutput.PartsOrder = []string{ + zerolog.LevelFieldName, + zerolog.MessageFieldName, } - consoleOutput.PartsExclude = []string{zerolog.TimestampFieldName} - return zerolog.New(consoleOutput).With().Logger() + ghOutput.NoColor = true + + return log.Output(ghOutput).With().Caller().Logger() } diff --git a/logger/logger_test.go b/logger/logger_test.go new file mode 100644 index 0000000..a815c40 --- /dev/null +++ b/logger/logger_test.go @@ -0,0 +1,124 @@ +// Copyright 2024 OWASP ModSecurity Core Rule Set Project +// SPDX-License-Identifier: Apache-2.0 + +package logger + +import ( + "bytes" + "testing" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/suite" +) + +type loggerTestSuite struct { + suite.Suite + out *bytes.Buffer + logger zerolog.Logger +} + +func TestRunLoggerTestSuite(t *testing.T) { + suite.Run(t, new(loggerTestSuite)) +} + +var testJsonBase = []struct { + name string + text string + logType zerolog.Level + want string +}{ + { + name: "JsonBaseOutput", + text: "hello", + logType: zerolog.InfoLevel, + want: "message\":\"hello\"", + }, +} + +var testConsoleBase = []struct { + name string + text string + logType zerolog.Level + want string +}{ + { + name: "BaseConsoleOutput", + text: "hello", + logType: zerolog.InfoLevel, + want: "INF hello component=parser-test", + }, +} + +var testGithub = []struct { + name string + text string + logType zerolog.Level + want string +}{ + { + name: "TestGithubInfoOutput", + text: "this is an info message", + logType: zerolog.InfoLevel, + want: "::notice ::this is an info message", + }, + { + name: "TestGithubWarningOutput", + text: "this is a warning message", + logType: zerolog.WarnLevel, + want: "::warn ::this is a warning message", + }, + { + name: "TestGithubTraceOutput", + text: "this is a trace message that will show as debug", + logType: zerolog.TraceLevel, + want: "::debug ::this is a trace message that will show as debug", + }, + { + name: "TestGithubDebugOutput", + text: "this is a debug message", + logType: zerolog.DebugLevel, + want: "::debug ::this is a debug message", + }, +} + +func (s *loggerTestSuite) SetupTest() { + // reset logger + s.out = &bytes.Buffer{} + s.logger = zerolog.New(s.out).With().Str("component", "parser-test").Logger() + zerolog.SetGlobalLevel(zerolog.TraceLevel) +} + +func (s *loggerTestSuite) TestJsonOutput() { + for _, t := range testJsonBase { + s.Run(t.name, func() { + s.logger.WithLevel(t.logType).Msg(t.text) + s.Contains(s.out.String(), t.want) + s.out.Reset() + }) + } +} + +func (s *loggerTestSuite) TestConsoleOutput() { + s.logger = s.logger.Output(zerolog.ConsoleWriter{Out: s.out, NoColor: true, TimeFormat: "03:04:05"}) + for _, t := range testConsoleBase { + s.Run(t.name, func() { + s.logger.WithLevel(t.logType).Msg(t.text) + s.Contains(s.out.String(), t.want) + s.out.Reset() + }) + } +} + +//s.log = log.Output(zerolog.ConsoleWriter{Out: os.Stderr, TimeFormat: "03:04:05"}).With().Caller().Logger() + +func (s *loggerTestSuite) TestSetGithubOutput() { + // send logs to buffer + logger := SetGithubOutput(s.out) + for _, t := range testGithub { + s.Run(t.name, func() { + logger.WithLevel(t.logType).Msg(t.text) + s.Contains(s.out.String(), t.want) + s.out.Reset() + }) + } +}