Skip to content
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

#185 Add RunListener.onFinalized listener. Inject START ActionNote #198

Merged
merged 14 commits into from
Aug 1, 2020

Conversation

tszmytka
Copy link
Collaborator

Fixes #185

During a log rendering, the business logic for ansicolor is enabled by ActionNotes serialized directly in the log file. If you open only the last x Bytes of the log it very likely won't contain any START ColorizedActions (there can be multiple ones) so the plugin will stay inactive.

If we inject an additional START ActionNote at x Bytes from the end of the log we will be able to trigger the functionality at just the right moment without affecting anything else. The problem is that you don't know the actual log size before the build has finished and when it does the OutputStream has obviously moved far beyond the injection point.

What this PR proposes is:

  • Attaching a RunListener.onFinalized event
  • Finding out which ColorizedAction should be active at the end - x Byte mark
  • Opening up the build log and injecting the appropriate ActionNote

The general idea already works. There are still a couple of todos and I also want to cover this with tests.
Constructive criticism would be welcome.

@dblock
Copy link
Member

dblock commented Jul 20, 2020

So you're now altering the log files? What could go wrong? :)

@tszmytka
Copy link
Collaborator Author

So you're now altering the log files? What could go wrong? :)

Well, "altering" seems so ominous.
What I am doing here is adding a ConsoleNote (in the exact same way as Jenkins does it) at an arbitrary position (end - x Bytes) in the log file. The problem is that in order to calculate the position the job needs to have finished (along with log appending).

Obviously the very worst scenario is a completely messed up log file and Jenkins not able to show it. I did think about that and I am doing the changes in a separate log copy and only substituting the original one in the last few lines (reverting the substitution in case of exceptions while doing it) - if anything goes wrong along the way the original log stays intact.

While thinking about alternatives what would essentially be needed in order to fix this bug is:

  1. Way to recognize that only the last x Bytes of the log are rendered vs. the whole log is rendered.
  2. Method to identify which ansiColor('xterm') Step (out of the possible multiple ones) is supposed to be "active" currently.

For Point no.1 we could maybe look into the actual log line and do something like:

if (line.startsWith("Started")) {
    // whole log is rendered
} else {
    // last `x` Bytes of the log are rendered
}

which might be problematic (what if some build tool also starts their logs with "Starting").
But for point no.2 we would still need to go through the whole log file to figure out which Step is currently "active".

Do you see any other alternatives?

@tszmytka tszmytka added the major-bug (release-drafter) label Jul 27, 2020
@tszmytka tszmytka changed the title [WIP] #185 Add RunListener.onFinalized listener. Inject START ActionNote #185 Add RunListener.onFinalized listener. Inject START ActionNote Aug 1, 2020
@tszmytka tszmytka marked this pull request as ready for review August 1, 2020 12:29
@tszmytka
Copy link
Collaborator Author

tszmytka commented Aug 1, 2020

Since manually updating the log file is not without risk of destroying the thing I took a bit cautious approach and managed to implement triggering the plugin in a shortlog without it.

This implementation still reads the whole log after the build but doesn't update it in any way. Instead it adds an additional ColorizedAction (containing the currently "active" ansicolor block) triggered by the first line rendered in shortlog. It works with global settings as well as pipeline-specified steps, multiple ones per pipeline included.

I consider the work done (functions and tests are there) but of course - I'm open to improvement suggestions.

@dblock dblock merged commit 71a3e73 into jenkinsci:master Aug 1, 2020
@dblock
Copy link
Member

dblock commented Aug 1, 2020

Nice job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-bug (release-drafter)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Escape chars printed raw in partial console output since 0.7.0
2 participants