[JENKINS-75378] Adding a CLI command listener #10382
Conversation
comment-ops-bot
bot
added
the
rfe
Wadeck
changed the title
| } else if (e instanceof AccessDeniedException) { | ||
| exitCode = 6; | ||
| printError(e.getMessage()); | ||
| } else if (e instanceof BadCredentialsException) { |
There was a problem hiding this comment.
Looks like BadCredentialsException is not longer possible inside CLICommand#main, note {set/get}TransportAuth2:
-
for HTTP or WebSocket the BadCredential is thrown/handled in the filters. When using command line
-auth user:passbasic auth : BasicHeaderAuthenticator / AbstractUserAuth. -
for SSH auth errors (not a BadCredentials) is also handled before executing the command (PublicKeyAuthenticatorImpl / UserAuthNamedFactory).
b1803a9 I can't find CliAuthenticationTest, may not apply anymore.
EDIT: this branch is only possible if a CLICommand implementation throws a BadCredentialsException,
| @@ -239,6 +238,9 @@ public int main(List<String> args, Locale locale, InputStream stdin, PrintStream | |||
| this.locale = locale; | |||
| CmdLineParser p = getCmdLineParser(); | |||
|
|
|||
| final String correlationId = UUID.randomUUID().toString(); | |||
There was a problem hiding this comment.
suggestion: The introduction of correlationId common to all events could be highlighted in the PR description.
There was a problem hiding this comment.
At this point correlationId is only used in BadCredentialsException handling. (where the original randomUUID comes form)
|
Looks good, thanks @apuig ! |
|
Hi @apuig, would this need to be added to the jenkins.io documentation, and if so is there any additional content/context beyond what is described in this and the JIRA tickets? |
| * Invoked after successful execution. | ||
| * | ||
| * @param context Information about the command being executed | ||
| * @param exitCode `run` returned exit code. |
There was a problem hiding this comment.
Implementations of CLICommand#run can return other values to indicate custom errors.
I try to avoid onSuccess because of this (but the javadoc comment still mention success :S), the idea is we call this if the command returned instead of throwing an exception.
I agree onCompleted vs onError its not clear enough, but can't find a better option (onException?)
There was a problem hiding this comment.
updated javadoc and using onException
| * Invoked on execution failure. | ||
| * | ||
| * @param context Information about the command being executed | ||
| * @param exitCode `run` returned exit code. |
There was a problem hiding this comment.
It seems unnecessary to pass this parameter to the listener. If anyone really cared it could be inferred by the same logic as in CLICommand but generally a listener is going to want to report any error at the Java level, not by using a Unix-like exit code value.
There was a problem hiding this comment.
ATM exit code is used to respect the current log levels. Notably unhanded exceptions and BadCredentials.
We can indeed check the same kind of Exceptions as in CLICommand#handleException, but exit codes handled by jenkins are properly documented on CLICommand#main, perhaps more maintainable ?
There was a problem hiding this comment.
exit code removed in the signature with the exception
clarify interface about onComplete not always success remove exitCode from onException remove fire methods args in CliContext rename CliListenerTest
See JENKINS-75378.
The default implementation mimics the current logs in CLICommand.
Testing done
Executed commands using cli.jar using different transports
Proposed changelog entries
Proposed changelog category
/label rfe
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered (see query).