From ef079aa9535c5ed4e77adb90cc498df4ca802e21 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Tue, 28 Nov 2023 16:32:18 +0200 Subject: [PATCH 1/5] fixed race condition in RunCmdWithOutputParser --- io/cmd.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/io/cmd.go b/io/cmd.go index 5c54c85..5c451b4 100644 --- a/io/cmd.go +++ b/io/cmd.go @@ -94,9 +94,11 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO for _, regExp := range regExpStruct { matched := regExp.RegExp.Match([]byte(line)) if matched { - regExp.MatchedResults = regExp.RegExp.FindStringSubmatch(line) - regExp.Line = line - line, err = regExp.ExecFunc(regExp) + results := CmdOutputPattern{ + MatchedResults: regExp.RegExp.FindStringSubmatch(line), + Line: line, + } + line, err = regExp.ExecFunc(&results) if err != nil { errChan <- err } @@ -117,8 +119,11 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO for _, regExp := range regExpStruct { matched := regExp.RegExp.Match([]byte(line)) if matched { - regExp.MatchedResults = regExp.RegExp.FindStringSubmatch(line) - regExp.Line = line + results := CmdOutputPattern{ + MatchedResults: regExp.RegExp.FindStringSubmatch(line), + Line: line, + } + line, err = regExp.ExecFunc(&results) line, scannerError = regExp.ExecFunc(regExp) if scannerError != nil { errChan <- scannerError From e6a747bf147076dc3ddeaba1fc1f36c2f0d8a078 Mon Sep 17 00:00:00 2001 From: yahavi Date: Wed, 29 Nov 2023 14:14:55 +0200 Subject: [PATCH 2/5] fixed race condition in RunCmdWithOutputParser --- io/cmd.go | 130 ++++++++++++++++++++++++++++++++++++------------- io/cmd_test.go | 97 ++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 35 deletions(-) create mode 100644 io/cmd_test.go diff --git a/io/cmd.go b/io/cmd.go index 5c451b4..fbb373f 100644 --- a/io/cmd.go +++ b/io/cmd.go @@ -2,12 +2,14 @@ package io import ( "bufio" + "bytes" "errors" "fmt" "io" "os" "os/exec" "regexp" + "strings" "sync" ) @@ -89,57 +91,28 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO errChan := make(chan error) wg.Add(1) go func() { + defer wg.Done() for stdoutReader.Scan() { - line := stdoutReader.Text() - for _, regExp := range regExpStruct { - matched := regExp.RegExp.Match([]byte(line)) - if matched { - results := CmdOutputPattern{ - MatchedResults: regExp.RegExp.FindStringSubmatch(line), - Line: line, - } - line, err = regExp.ExecFunc(&results) - if err != nil { - errChan <- err - } - } - } + line, _ := processLine(regExpStruct, stdoutReader.Text(), errChan) if prompt { fmt.Fprintf(os.Stderr, line+"\n") } stdOut += line + "\n" } - wg.Done() }() wg.Add(1) go func() { + defer wg.Done() for stderrReader.Scan() { - line := stderrReader.Text() - var scannerError error - for _, regExp := range regExpStruct { - matched := regExp.RegExp.Match([]byte(line)) - if matched { - results := CmdOutputPattern{ - MatchedResults: regExp.RegExp.FindStringSubmatch(line), - Line: line, - } - line, err = regExp.ExecFunc(&results) - line, scannerError = regExp.ExecFunc(regExp) - if scannerError != nil { - errChan <- scannerError - break - } - } - } + line, hasError := processLine(regExpStruct, stderrReader.Text(), errChan) if prompt { fmt.Fprintf(os.Stderr, line+"\n") } errorOut += line + "\n" - if scannerError != nil { + if hasError { break } } - wg.Done() }() go func() { @@ -147,7 +120,10 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO close(errChan) }() - for err = range errChan { + for channelErr := range errChan { + err = errors.Join(err, channelErr) + } + if err != nil { return } @@ -163,6 +139,34 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO return } +// Run all of the input regExpStruct array on the input stdout or stderr line. +// If an error occurred, add it to the error channel. +// regExpStruct - Array of command output patterns to process the line +// line - string line from stdout or stderr +// errChan - if an error occurred, add it to this channel +func processLine(regExpStruct []*CmdOutputPattern, line string, errChan chan error) (processedLine string, hasError bool) { + var err error + processedLine = line + for _, regExp := range regExpStruct { + matched := regExp.RegExp.MatchString(processedLine) + if matched { + results := CmdOutputPattern{ + RegExp: regExp.RegExp, + MatchedResults: regExp.RegExp.FindStringSubmatch(processedLine), + Line: processedLine, + ExecFunc: regExp.ExecFunc, + } + processedLine, err = regExp.ExecFunc(&results) + if err != nil { + errChan <- err + hasError = true + break + } + } + } + return +} + // Create command stdout and stderr readers. // The returned readers are automatically closed after the running command exit and shouldn't be closed explicitly. // cmd - The command to execute @@ -197,3 +201,59 @@ type CmdOutputPattern struct { Line string ExecFunc func(pattern *CmdOutputPattern) (string, error) } + +type Command struct { + Executable string + CmdName string + CmdArgs []string + Dir string + StrWriter io.WriteCloser + ErrWriter io.WriteCloser +} + +func NewCommand(executable, cmdName string, cmdArgs []string) *Command { + return &Command{Executable: executable, CmdName: cmdName, CmdArgs: cmdArgs} +} + +func (config *Command) RunWithOutput() (data []byte, err error) { + cmd := config.GetCmd() + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err = cmd.Run() + if err != nil { + return nil, fmt.Errorf("failed running command: '%s %s' with error: %s - %s", + cmd.Dir, + strings.Join(cmd.Args, " "), + err.Error(), + stderr.String(), + ) + } + return stdout.Bytes(), nil +} + +func (config *Command) GetCmd() (cmd *exec.Cmd) { + var cmdStr []string + if config.CmdName != "" { + cmdStr = append(cmdStr, config.CmdName) + } + if config.CmdArgs != nil && len(config.CmdArgs) > 0 { + cmdStr = append(cmdStr, config.CmdArgs...) + } + cmd = exec.Command(config.Executable, cmdStr...) + cmd.Dir = config.Dir + return +} + +func (config *Command) GetEnv() map[string]string { + return map[string]string{} +} + +func (config *Command) GetStdWriter() io.WriteCloser { + return config.StrWriter +} + +func (config *Command) GetErrWriter() io.WriteCloser { + return config.ErrWriter +} diff --git a/io/cmd_test.go b/io/cmd_test.go new file mode 100644 index 0000000..960c946 --- /dev/null +++ b/io/cmd_test.go @@ -0,0 +1,97 @@ +package io + +import ( + "errors" + "regexp" + "testing" + + "github.com/stretchr/testify/assert" +) + +var matchAllRegexp = regexp.MustCompile(".*") +var errParsing = errors.New("parsing error") + +func TestRunCmdWithOutputParser(t *testing.T) { + config := NewCommand("git", "", []string{"status"}) + stdout, stderr, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{ + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, nil }, + }) + assert.NoError(t, err) + assert.True(t, exitOk) + assert.Contains(t, stdout, "On branch") + assert.Empty(t, stderr) +} + +func TestRunCmdWithOutputParserError(t *testing.T) { + config := NewCommand("git", "", []string{"status"}) + _, _, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{ + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, errParsing }, + }) + assert.ErrorContains(t, err, "parsing error\nparsing error") + assert.False(t, exitOk) +} + +var processLineCases = []struct { + cmdOutputPatterns []*CmdOutputPattern + line string + expectedOutput string + expectError bool +}{ + {[]*CmdOutputPattern{}, "", "", false}, + + {[]*CmdOutputPattern{{ + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, nil }, + }}, "hello", "hello", false}, + + {[]*CmdOutputPattern{{ + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line[1:], nil }, + }}, "hello", "ello", false}, + + {[]*CmdOutputPattern{ + { + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "l", nil }, + }, + { + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "o", nil }, + }, + }, "hel", "hello", false}, + + {[]*CmdOutputPattern{ + { + RegExp: regexp.MustCompile("doesn't match"), + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "aaaaaa", nil }, + }, + { + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "o", nil }, + }, + }, "hell", "hello", false}, + + {[]*CmdOutputPattern{{ + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return "", errParsing }, + }}, "hello", "", true}, +} + +func TestProcessLine(t *testing.T) { + for _, testCase := range processLineCases { + t.Run("", func(t *testing.T) { + errChan := make(chan error, 1) + defer close(errChan) + processedLine, hasErrors := processLine(testCase.cmdOutputPatterns, testCase.line, errChan) + if testCase.expectError { + assert.True(t, hasErrors) + assert.ErrorIs(t, errParsing, <-errChan) + } else { + assert.False(t, hasErrors) + assert.Equal(t, testCase.expectedOutput, processedLine) + } + }) + } +} From 96ea18210e2d4573b98d261066e55abbf87b9321 Mon Sep 17 00:00:00 2001 From: yahavi Date: Wed, 29 Nov 2023 14:17:29 +0200 Subject: [PATCH 3/5] Update Go to 1.20 --- .github/workflows/analysis.yml | 4 ++-- .github/workflows/frogbot-fix-go.yml | 2 +- .github/workflows/frogbot-scan-pr-go.yml | 2 +- .github/workflows/test.yml | 2 +- go.mod | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/analysis.yml b/.github/workflows/analysis.yml index c398717..e847680 100644 --- a/.github/workflows/analysis.yml +++ b/.github/workflows/analysis.yml @@ -9,7 +9,7 @@ jobs: - name: Install Go uses: actions/setup-go@v3 with: - go-version: 1.19.x + go-version: 1.20.x - name: Static Code Analysis uses: dominikh/staticcheck-action@v1 with: @@ -23,7 +23,7 @@ jobs: - name: Install Go uses: actions/setup-go@v3 with: - go-version: 1.19.x + go-version: 1.20.x - name: Install gosec run: curl -sfL https://raw.githubusercontent.com/securego/gosec/master/install.sh | sh -s -- -b $(go env GOPATH)/bin - name: Run gosec diff --git a/.github/workflows/frogbot-fix-go.yml b/.github/workflows/frogbot-fix-go.yml index 2fcdf7f..84c550e 100644 --- a/.github/workflows/frogbot-fix-go.yml +++ b/.github/workflows/frogbot-fix-go.yml @@ -18,7 +18,7 @@ jobs: - name: Setup Go uses: actions/setup-go@v3 with: - go-version: 1.19.x + go-version: 1.20.x - uses: jfrog/frogbot@v2 env: diff --git a/.github/workflows/frogbot-scan-pr-go.yml b/.github/workflows/frogbot-scan-pr-go.yml index 0a19766..87f618e 100644 --- a/.github/workflows/frogbot-scan-pr-go.yml +++ b/.github/workflows/frogbot-scan-pr-go.yml @@ -19,7 +19,7 @@ jobs: - name: Setup Go uses: actions/setup-go@v3 with: - go-version: 1.19.x + go-version: 1.20.x - uses: jfrog/frogbot@v2 env: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5ab119a..8377cfe 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -17,7 +17,7 @@ jobs: - name: Install Go uses: actions/setup-go@v3 with: - go-version: 1.19.x + go-version: 1.20.x - name: Go Cache uses: actions/cache@v3 diff --git a/go.mod b/go.mod index bd3e1b0..95b050f 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/jfrog/gofrog -go 1.19 +go 1.20 require ( github.com/mholt/archiver/v3 v3.5.1 From 4dab18c54b36c38be2a0ff2a1bda4f4f1f631e14 Mon Sep 17 00:00:00 2001 From: yahavi Date: Wed, 29 Nov 2023 14:22:20 +0200 Subject: [PATCH 4/5] Fix test --- io/cmd.go | 15 +++++++++------ io/cmd_test.go | 8 ++++---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/io/cmd.go b/io/cmd.go index fbb373f..65cf5c8 100644 --- a/io/cmd.go +++ b/io/cmd.go @@ -89,6 +89,7 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO return } errChan := make(chan error) + stdoutBuilder := strings.Builder{} wg.Add(1) go func() { defer wg.Done() @@ -97,9 +98,11 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO if prompt { fmt.Fprintf(os.Stderr, line+"\n") } - stdOut += line + "\n" + stdoutBuilder.WriteString(line) + stdoutBuilder.WriteRune('\n') } }() + stderrBuilder := strings.Builder{} wg.Add(1) go func() { defer wg.Done() @@ -108,7 +111,8 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO if prompt { fmt.Fprintf(os.Stderr, line+"\n") } - errorOut += line + "\n" + stderrBuilder.WriteString(line) + stderrBuilder.WriteRune('\n') if hasError { break } @@ -120,12 +124,11 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO close(errChan) }() - for channelErr := range errChan { - err = errors.Join(err, channelErr) - } - if err != nil { + for err = range errChan { return } + stdOut = stdoutBuilder.String() + errorOut = stderrBuilder.String() err = cmd.Wait() if err != nil { diff --git a/io/cmd_test.go b/io/cmd_test.go index 960c946..13cf5d0 100644 --- a/io/cmd_test.go +++ b/io/cmd_test.go @@ -12,24 +12,24 @@ var matchAllRegexp = regexp.MustCompile(".*") var errParsing = errors.New("parsing error") func TestRunCmdWithOutputParser(t *testing.T) { - config := NewCommand("git", "", []string{"status"}) + config := NewCommand("go", "", []string{"version"}) stdout, stderr, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{ RegExp: matchAllRegexp, ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, nil }, }) assert.NoError(t, err) assert.True(t, exitOk) - assert.Contains(t, stdout, "On branch") + assert.Contains(t, stdout, "go version") assert.Empty(t, stderr) } func TestRunCmdWithOutputParserError(t *testing.T) { - config := NewCommand("git", "", []string{"status"}) + config := NewCommand("go", "", []string{"version"}) _, _, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{ RegExp: matchAllRegexp, ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, errParsing }, }) - assert.ErrorContains(t, err, "parsing error\nparsing error") + assert.ErrorContains(t, err, "parsing error") assert.False(t, exitOk) } From b52bef08c3c06ecaf509179ab9987849c1f60a06 Mon Sep 17 00:00:00 2001 From: yahavi Date: Thu, 30 Nov 2023 11:13:26 +0200 Subject: [PATCH 5/5] CR --- io/cmd.go | 28 ++++++++++++++-------------- io/cmd_test.go | 24 +++++++++++++++--------- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/io/cmd.go b/io/cmd.go index 65cf5c8..0f6cc3c 100644 --- a/io/cmd.go +++ b/io/cmd.go @@ -151,20 +151,20 @@ func processLine(regExpStruct []*CmdOutputPattern, line string, errChan chan err var err error processedLine = line for _, regExp := range regExpStruct { - matched := regExp.RegExp.MatchString(processedLine) - if matched { - results := CmdOutputPattern{ - RegExp: regExp.RegExp, - MatchedResults: regExp.RegExp.FindStringSubmatch(processedLine), - Line: processedLine, - ExecFunc: regExp.ExecFunc, - } - processedLine, err = regExp.ExecFunc(&results) - if err != nil { - errChan <- err - hasError = true - break - } + if !regExp.RegExp.MatchString(processedLine) { + continue + } + results := CmdOutputPattern{ + RegExp: regExp.RegExp, + MatchedResults: regExp.RegExp.FindStringSubmatch(processedLine), + Line: processedLine, + ExecFunc: regExp.ExecFunc, + } + processedLine, err = regExp.ExecFunc(&results) + if err != nil { + errChan <- err + hasError = true + break } } return diff --git a/io/cmd_test.go b/io/cmd_test.go index 13cf5d0..0511fea 100644 --- a/io/cmd_test.go +++ b/io/cmd_test.go @@ -13,11 +13,16 @@ var errParsing = errors.New("parsing error") func TestRunCmdWithOutputParser(t *testing.T) { config := NewCommand("go", "", []string{"version"}) + parserCalled := false stdout, stderr, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{ - RegExp: matchAllRegexp, - ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, nil }, + RegExp: matchAllRegexp, + ExecFunc: func(pattern *CmdOutputPattern) (string, error) { + parserCalled = true + return pattern.Line, nil + }, }) assert.NoError(t, err) + assert.True(t, parserCalled) assert.True(t, exitOk) assert.Contains(t, stdout, "go version") assert.Empty(t, stderr) @@ -34,24 +39,25 @@ func TestRunCmdWithOutputParserError(t *testing.T) { } var processLineCases = []struct { + name string cmdOutputPatterns []*CmdOutputPattern line string expectedOutput string expectError bool }{ - {[]*CmdOutputPattern{}, "", "", false}, + {"Empty", []*CmdOutputPattern{}, "", "", false}, - {[]*CmdOutputPattern{{ + {"Simple", []*CmdOutputPattern{{ RegExp: matchAllRegexp, ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, nil }, }}, "hello", "hello", false}, - {[]*CmdOutputPattern{{ + {"Append character once", []*CmdOutputPattern{{ RegExp: matchAllRegexp, ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line[1:], nil }, }}, "hello", "ello", false}, - {[]*CmdOutputPattern{ + {"Append character twice", []*CmdOutputPattern{ { RegExp: matchAllRegexp, ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "l", nil }, @@ -62,7 +68,7 @@ var processLineCases = []struct { }, }, "hel", "hello", false}, - {[]*CmdOutputPattern{ + {"Doesn't match", []*CmdOutputPattern{ { RegExp: regexp.MustCompile("doesn't match"), ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "aaaaaa", nil }, @@ -73,7 +79,7 @@ var processLineCases = []struct { }, }, "hell", "hello", false}, - {[]*CmdOutputPattern{{ + {"Parsing error", []*CmdOutputPattern{{ RegExp: matchAllRegexp, ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return "", errParsing }, }}, "hello", "", true}, @@ -81,7 +87,7 @@ var processLineCases = []struct { func TestProcessLine(t *testing.T) { for _, testCase := range processLineCases { - t.Run("", func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { errChan := make(chan error, 1) defer close(errChan) processedLine, hasErrors := processLine(testCase.cmdOutputPatterns, testCase.line, errChan)