[JENKINS-54133] Pregenerating console notes sent to agents for some common cases #128
Conversation
| @@ -45,23 +46,51 @@ | |||
| </distributionManagement> | |||
|
|
|||
| <properties> | |||
| <jenkins.version>1.642.3</jenkins.version> | |||
| <jenkins.version>2.121.2</jenkins.version> | |||
There was a problem hiding this comment.
According to stats this is not a big deal, even if there are active trunk releases of this plugin for unrelated changes.
But, if baseline is an obstacle, the POM changes could be reverted. In that case the only src/main/ change would be to convert some uses of lambdas to inner classes, pretty simple; the src/test/ code could remain, but would no longer reproduce the problem unless run under plugin-compat-tester.
| for (AnsiColorMap.Color color : AnsiColorMap.Color.values()) { | ||
| pregenerateNote(new AnsiAttributeElement(AnsiAttributeElement.AnsiAttrType.FG, "span", "style=\"color: " + colorMap.getNormal(color.ordinal()) + ";\"")); | ||
| } | ||
| // TODO other cases, and other methods |
There was a problem hiding this comment.
The tedious part. Handling bright colors, swapColors, bold/italic/underline, etc. Basically every call to the AnsiAttributeElement constructor from AnsiHtmlOutputStream which could be predicted given an AnsiColorMap—probably all but the explicit use of RGB coördinates, which I am guessing are rarely used anyway.
|
|
||
| private void pregenerateNote(String html) { | ||
| if (!notes.containsKey(html)) { | ||
| JenkinsJVM.checkJenkinsJVM(); |
There was a problem hiding this comment.
See jenkinsci/ant-plugin#32 for a simpler patch that illustrates the idiom.
There was a problem hiding this comment.
Looks good to me, and it's easy enough to add more cases, although I don't know what would be most commonly used. I did a quick test locally and it seems like the addition of AnsiAttrType.BOLD would handle all of the ANSI sequence that Maven uses, so maybe it would be good to add that one as well.
There was a problem hiding this comment.
This looks good to me. See a note on test.
Please update https://github.com/jenkinsci/ansicolor-plugin/blob/master/CHANGELOG.md, too.
| WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p"); | ||
| p.setDefinition(new CpsFlowDefinition( | ||
| "node {\n" | ||
| "node('!master') {\n" |
There was a problem hiding this comment.
Feels like this should be a separate test.
There was a problem hiding this comment.
It seemed unnecessary as AnsiColorStepTest already tests the behavior on master.
|
@jglick We could also use another maintainer here, let us know if you're interested. |
Sorry, I could not commit to that. |
Probably. I was reluctant to get into cases for which there is no automated test coverage. It is possible the relevant parts of |
|
Merged, thanks. |
JENKINS-54133. Does not attempt to pregenerate notes for all cases, and in fact this seems to be impossible, though more common cases could certainly be added as needed.
As noted in JIRA, reimplementing the plugin as a
ConsoleAnnotatorFactorywould be a better solution in many respects, as well as fully solving this issue, but this would be a more ambitious refactoring than I am prepared to take on at the moment as it would involve inverting the control flow ofAnsiOutputStream. See the corresponding Blue Ocean jenkinsci/blueocean-plugin#1325 for a very general idea.