Changed the truncate summary to cut build logs from top #272
Conversation
| */ | ||
| private Optional<String> truncateSummary(final TruncatedString summaryToTruncate, final int maxSize) { | ||
| String content = summaryToTruncate.toString(); | ||
| if (!content.contains("<summary>")) { |
There was a problem hiding this comment.
I'm not too keen on all this implementation detail of the build log being in checks output.
Couldn't we do something smarter when we get the log here:
https://github.com/jenkinsci/checks-api-plugin/blob/master/src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java#L205-L208
Possibly we can offset it to only ask for the last bit of log, if we can't do that we could process it in that method to truncate to just what we need?
There was a problem hiding this comment.
@timja are you ok with hardcoding the max char size in the FlowExecutionAnalyzer?
There was a problem hiding this comment.
Yeah I think we'll need to, based on the GitHub size for now, no other providers have been published, we can refactor if anymore come along or find a way to pass the config in but for now I think it would be overengineering to do it another way.
There was a problem hiding this comment.
Thanks I wasn't clear on consumers so was nervous to modify the FlowExecutionAnalyzer, but agree that's much simpler. Will refactor and tag you again
There was a problem hiding this comment.
@timja could you review again when you get a moment?
| @@ -132,9 +134,12 @@ private Pair<String, String> processErrorOrWarningRow(final FlowGraphTable.Row r | |||
| nodeTextBuilder.append(String.format("**Error**: *%s*", displayName)); | |||
| nodeSummaryBuilder.append(String.format("```%n%s%n```%n", displayName)); | |||
| if (!suppressLogs) { | |||
| String log = getLog(flowNode); | |||
| // -2 for "\n\n" at the end of the summary | |||
There was a problem hiding this comment.
shouldn't it be approx -70 (or even -100 to be safe?)
echo '<details>%n<summary>Build log</summary>%n%n```%n%s%n```%n</details>' | wc
1 2 68
There was a problem hiding this comment.
I think you are looking at an older version of the code, I have included the log format in the max size. I added an additional 30 character buffer to be safe though
// -2 for "\n\n" at the end of the summary
String logTemplate = "<details>%n<summary>Build log</summary>%n%n```%n%s%n```%n</details>";
int maxMessageSize = MAX_MESSAGE_SIZE_TO_CHECKS_API - nodeSummaryBuilder.length() - logTemplate.length() - 2;
| assertThat(details.getOutput()).isPresent().get().satisfies(output -> { | ||
| assertThat(output.getSummary()).isPresent().get().satisfies(summary -> { | ||
| // Verify the log section exists and is truncated | ||
| assertThat(summary).contains("<details>"); |
There was a problem hiding this comment.
could you also check for </details> to make sure its not dropped?
|
@timja The |
yes a manual step in this repo that I missed, see here: https://github.com/jenkinsci/checks-api-plugin/releases/tag/v2.2.3 |
From these two tickets:
Previous attempt at this change:
#270
Desired behavior threads:
jenkinsci/github-checks-plugin#343 (comment)
jenkinsci/github-checks-plugin#423
We believe it is better to default truncating the checks build logs to be from the start rather than the end of the logs. This behavior is better because typically the most relevant logs, especially error logs, happen at the end of the output.
I reverted my previous PR because truncating text is not actually relevant to the change I want.
I maintained the existing behavior in cases where truncating the build logs was a non-viable option.
Submitter checklist