-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Step output writer #855
Step output writer #855
Conversation
stepoutput/stepoutputwriter.go
Outdated
func (w Writer) Flush() (int, error) { | ||
if w.secretWriter != nil { | ||
n, err := w.secretWriter.Flush() | ||
if err != nil { | ||
return n, err | ||
} | ||
|
||
} | ||
|
||
if w.logLevelWriter != nil { | ||
return w.logLevelWriter.Flush() | ||
} | ||
|
||
return 0, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is more of a thinking out loud than an actual comment.
Flush
is interesting because as far as I can see (I might be wrong here) the io.Writer
does not have a flush mechanism. Based on some stackoverflow answers the writer does write everything remaining when it is closed.
But in our case Flush
is called here:
Line 318 in 04c83d8
if _, ferr := flusher.Flush(); ferr != nil { |
When a step execution is done. But we are not reusing our own writers because we create a new one for every step:
Line 415 in f7bd078
writer := stepoutput.NewWriter(stepSecrets, opts) |
We could reuse the existing io.Closer
interface and implement our own closing stuff (which is here in the Flush method) and be a true writer. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, I updated all the step output writers to implement io.WriteCloser here: da50e4e
@@ -0,0 +1,69 @@ | |||
package stepoutput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new package was a great idea.
writers []errorFindingWriter | ||
// ErrorFinder parses the data coming via the `Write` method and keeps the latest "red" block (that matches \x1b[31;1m control sequence) | ||
// and hands over tha data to the wrapped `io.Writer` instance. | ||
type ErrorFinder struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does look better with less abstractions. ⭐
if e.collecting { | ||
if endIndex := getEndColorIndex(haystack); len(endIndex) > 0 { | ||
if endIndex[0] != 0 { | ||
e.errorMessages = append(e.errorMessages, redRegexp.ReplaceAllString(haystack[0:endIndex[0]], "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only bit that has changed compared to the original code block, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought it was better to collect all the errors and let the consumer decide which one to use. Also, later we will be able to utilize all the errors.
} else { | ||
inReader = os.Stdin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting catch. Was it a bug or feature? 😄
I have checked all of the codepaths (tried to look up all of them) and it does not seem like we ever wanted to use it. Which does not make this unnecessary but just made me wonder how you caught it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tiny improvement. When a plugin gets executed with EnvmanRun stdInPayload
might be defined, in this case from now on there is -1 assignment.
tools/tools.go
Outdated
errorParser, isErrorParser := outWriter.(ErrorParser) | ||
if err != nil { | ||
var exitErr *exec.ExitError | ||
if errors.As(err, &exitErr) { | ||
exitCode := exitErr.ExitCode() | ||
|
||
if isErrorParser { | ||
errorMessages := errorParser.ErrorMessages() | ||
if len(errorMessages) > 0 { | ||
lastErrorMessage := errorMessages[len(errorMessages)-1] | ||
return exitCode, errors.New(lastErrorMessage) | ||
} | ||
} | ||
|
||
return exitCode, err | ||
} else { | ||
return 1, fmt.Errorf("executing command failed: %w", err) | ||
} | ||
} | ||
|
||
return timeoutcmd.ExitStatus(err), errorFinderWriter.WrapError(err) | ||
return 0, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These embedded if
s made this block harder to read. I have tried to convert it into a more flat structure and would like to hear you opinion about it:
if err == nil {
return 0, nil
}
var exitErr *exec.ExitError
if errors.As(err, &exitErr) == false {
return 1, fmt.Errorf("executing command failed: %w", err)
}
exitCode := exitErr.ExitCode()
errorParser, isErrorParser := outWriter.(ErrorParser)
if isErrorParser {
errorMessages := errorParser.ErrorMessages()
if len(errorMessages) > 0 {
lastErrorMessage := errorMessages[len(errorMessages)-1]
return exitCode, errors.New(lastErrorMessage)
}
}
return exitCode, err
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 8f921fc
log/logwriter/writer.go
Outdated
logger: logger, | ||
} | ||
} | ||
|
||
func (w LogWriter) Write(p []byte) (n int, err error) { | ||
// TODO: handle if currentChunk is too big | ||
func (w *LogLevelWriter) Write(p []byte) (n int, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a meaty function with a lot of cases in it. I wonder if it could simplified in any way.
One idea I have is that if its enough to look for a single colour and its reset code. Our steps log stuff line by line and every line has a closing reset code. In the old implementation we did not care of the embedded colours because those do not contribute to the log level and will be rendered by the website.
I will try to write some rudimentary pseudo code:
func (w *LogLevelWriter) Write(p []byte) (n int, err error) {
chunk := string(p)
colorCodes := findEmbeddedColors(chunk)
if len(colorCodes) > 0 {
w.colorStack.add(colorCodes)
}
resetColorCount := numbeOfResetColors(chunk)
currentColor := ""
for i := 0; i < resetColorCount; i++ {
currentColor = w.colorStack.pop()
}
w.currentChunk = w.currentChunk + chunk
if len(w.colorStack) == 0 {
w.logger.LogMessage(w.currentChunk, convertCurrentColorToLevel(currentColor))
w.currentChunk = ""
return len(p), nil
}
return len(p), nil
}
I am not yet sure that this logic is correct but what it tries to do is that it collects how many colors are active. Because we are only looking for the outermost one it simply pops the saved colors of the stack as we get the reset codes. Once the stack is empty we log the message based on the last color code the stack had.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR took a slightly different direction: the change in this file was the primary functional change to fix the log-level parsing issue.
We decided to do some preliminary work before fixing the original issue.
The remaining file changes are still valid technical improvements, so the function change was reverted and this PR is about technical improvements now.
Checklist
README.md
is updated with the changes (if needed)Version
Requires a PATCH version update
Context
This PR introduces a new component (
stepoutput.Writer
), which is responsible for integrating the different writers processing the Step output logs.Changes
a new
stepoutput
packageThis package was introduced to make the step output writer chain a higher-level code and be able to unit tests the whole writer chain.
errorfinder
packageThe package was overcomplicated, so it got simplified to make it understandable.
Initially it exposed two structs:
errorFinder
anddefaultErrorFindingWriter
.errorFinder
was just a tiny layer on top of thedefaultErrorFindingWriter
, which exposed functionality for creating a new defaultErrorFindingWriter and basically for reading the errors from the underlying defaultErrorFindingWriter.The new version exposes the
ErrorFinder
struct, which is primarily an io.Writer, which parses error messages from the output.Investigation details
Decisions