[cli] Add exit code for processing errors #4991
Conversation
Generated by 🚫 Danger |
[skip ci]
There was a problem hiding this comment.
Thanks for the PR
I think we should also consider CPD's --skip-lexical-errors with this PR. It seems to me we should deprecate --skip-lexical-errors in favor of the new flag.
Currently CPD lexes all files and once it is done, if there was an error it recovered from, it just gives up if --skip-lexical-errors is not set. I think this is not good behavior, as we already recovered from all errors at this point. In this case, I think we should continue execution and just make sure the exit code reflects whether there were errors or not (also respecting the flags --fail-on-violation and --fail-on-error). This would align PMD and CPD closer.
Another thing is I think we should stop using the phrase "processing error" in our doc and especially in the new switch --fail-on-processing-error. What a "processing error" is is vague and although we use it sometimes in our doc we don't explain it anywhere. It is likely our users have no idea how a "processing error" is different from an "error".
FWIW what we call today a "processing error" is just an error we recovered from gracefully, and which didn't prevent us from outputting a report. There is no difference in nature from other errors, just in how we handled it. That's why I think this phrase it weird and cryptic. There are two kinds of errors in PMD/CPD:
- Errors which were unexpected and we did not recover from. These cause exit code 1, and when they happen, then the report is either non-existent (we failed before reporting) or broken (we failed during reporting).
- Errors from which we recovered gracefully ("processing errors"). They did not prevent the creation of a valid report, and these errors are actually part of the rendered report. Under your proposal these would cause exit code 5.
I think it would be better if we dropped the phrase "processing error" and just referred to these as "errors", or "recovered errors" if that's not clear from context.
So I would like to have the new flag named --[no-]fail-on-error without the processing.
|
Thanks for the review! I honestly ignored CPD's flag I'll try to update this PR the next couple of days... |
| <td>failonerror</td> | ||
| <td>Whether or not to fail the build if any errors occur while processing the files</td> | ||
| <td>failOnError</td> | ||
| <td>Whether or not to fail the build if any processing errors occur while analyzing files.</td> |
There was a problem hiding this comment.
| <td>Whether or not to fail the build if any processing errors occur while analyzing files.</td> | |
| <td>Whether or not to fail the build if any recoverable errors occur while analyzing files.</td> |
| * @return failOnViolation | ||
| * | ||
| * @see #isFailOnError() | ||
| * @since 7.2.0 |
There was a problem hiding this comment.
| * @since 7.2.0 | |
| * @since 6.0.0 |
| * @return failOnError | ||
| * | ||
| * @see #isFailOnViolation() | ||
| * @since 6.0.0 |
There was a problem hiding this comment.
| * @since 6.0.0 | |
| * @since 7.2.0 |
| By default, the build will fail. CPD will still create a report with all detected duplications, but the report might | ||
| be incomplete. | ||
| * The parameter `skipLexicalError` in CPDTask is deprecated. Use the new parameter `failOnError` instead. | ||
|
|
||
| #### Deprecated API |
There was a problem hiding this comment.
TODO:
- CPDConfiguration.isSkipLexicalErrors
- CPDConfiguration.setSkipLexicalErrors
- CPDTask.setSkipLexicalErrors
Describe the PR
This adds a new exit code 5 when processing errors occurred (both in PMD and CPD).
This helps in avoiding that such errors are unnoticed.
I think, it's better to fail in such cases than to ignore processing errors.
Not sure, whether an extra exit code 5 really beneficial or needed or we should reuse exit code 4 (which is violations only). The problem I see with exit code 5, you only know that one or more processing errors occurred, but you don't know whether there were any violations. We could of course add another exit code 6 (Only processing errors but no violations). I don't know, whether this is really needed.
The main goal I had is: I want to fail the build in case of processing errors. The result of a failing build is usually the same (regardless of the failure cause): I need to look at the build, logs, changes, to figure out the problem and solve it. The exit code gives only a little bit more information.
Interestingly, this feature was already present in the ant task. It is requested for gradle (gradle/gradle#28672). And the feature is also available in maven-pmd-plugin, but disabled by default. There is a request to fail the build by default in the presence of processing errors (https://issues.apache.org/jira/browse/MPMD-383).
Related issues
Fixes [cli] Consider processing errors in exit status #2827
Related in broader context: [core] Error handling in PMD 7 #3761
Ready?
./mvnw clean verifypasses (checked automatically by github actions)