Add support for retrying a build after a node restart #3271
Add support for retrying a build after a node restart #3271uhafner merged 7 commits intojenkinsci:mainfrom
Conversation
…ady used by another action: io.jenkins.plugins.analysis.core.model.ResultAction for SpotBugs
|
Please review the changes @uhafner |
☀️ Quality MonitorTests Coverage for New Code 〰️ Line Coverage: 100.00% — perfect 🎉 Coverage for Whole Project 〰️ Line Coverage: 80.78% — 1176 missed lines Style Bugs Vulnerabilities🛡️ OWASP Dependency Check: 214 vulnerabilities — error: 18, high: 54, normal: 136, low: 6 Software Metrics 🌀 Cyclomatic Complexity: 2603 (total) 🚦 Quality GatesOverall Status: ❌ FAILURE✅ Passed Gates
❌ Failed Gates
Created by Quality Monitor v4.10.0 (#be1b382). More details are shown in the GitHub Checks Result. |
uhafner
left a comment
There was a problem hiding this comment.
Can you attach the action with run.addOrReplaceAction
| if (other.isPresent()) { | ||
| throw new IllegalStateException( | ||
| "ID %s is already used by another action: %s%n".formatted(getId(), other.get())); | ||
| report.logInfo("Removing existing result action with ID '%s' (might be caused by a restart)", getId()); |
There was a problem hiding this comment.
| report.logInfo("Removing existing result action with ID '%s' (might be caused by a restart)", getId()); | |
| report.logError("Removing existing result action with ID '%s' (duplicate ID: restart or configuration error?)", getId()); |
There was a problem hiding this comment.
Can you remove this method? It makes no sense anymore. Create the selector inline where it is needed.
See line 127.
| List<AnalysisResult> results = getAnalysisResults(build); | ||
|
|
||
| assertThat(results).hasSize(1); | ||
| var result = results.get(0); |
There was a problem hiding this comment.
Can you check the content of the warnings, so we can see that the second result is used? (Or remove one warning from the second file, then check for 5 issues.)
| /** | ||
| * Tests retry after restart scenario (JENKINS-72920). When the same tool is run twice within a build | ||
| * (which can happen during retry after infrastructure failure), the second run should replace the first | ||
| * with a warning message instead of throwing an exception. | ||
| */ | ||
| @Test | ||
| @org.junitpioneer.jupiter.Issue("JENKINS-72920") | ||
| void shouldAllowRetryAfterRestart() { | ||
| var project = createFreestyleJob("checkstyle.xml"); | ||
| var tool = createTool(new CheckStyle(), "**/checkstyle-issues.txt"); | ||
| enableWarnings(project, recorder -> recorder.setAggregatingResults(false), tool, tool); | ||
|
|
||
| Run<?, ?> build = buildWithResult(project, Result.SUCCESS); | ||
|
|
||
| List<AnalysisResult> results = getAnalysisResults(build); | ||
| assertThat(results).hasSize(1); | ||
|
|
||
| var result = results.get(0); | ||
| assertThat(result).hasId(CHECKSTYLE); | ||
| assertThat(result).hasTotalSize(6); | ||
| assertThat(getConsoleLog(result)).contains("Removing existing result action with ID 'checkstyle' (might be caused by a restart)"); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the same test as shouldThrowExceptionIfSameToolIsConfiguredTwice. Remove it.
| * be replaced with a warning (to support retry after restart scenarios). | ||
| */ | ||
| @Test | ||
| void shouldThrowExceptionIfSameToolIsConfiguredTwice() { |
There was a problem hiding this comment.
| void shouldThrowExceptionIfSameToolIsConfiguredTwice() { | |
| void shouldReplaceExistingAction() { |
…e tests for CheckStyle tool behavior
|
Can you please review the changes? @uhafner |
When Jenkins retries a build after a restart there might be already a published action available. In such a case we should not fail the build, we should only log a warning.
Fixes #3138
Testing done
Submitter checklist