[JENKINS-75378] Adding a CLI command listener by apuig · Pull Request #10382 · jenkinsci/jenkins

apuig · 2025-03-06T16:42:02Z

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

Add a new ExtensionPoint to receive events from CLI executions.
Move logs from CLICommand to a new default listener.
Command implementations using CLIMethod now log under the same conditions as CLICommand.

Proposed changelog category

/label rfe

Proposed upgrade guidelines

N/A

Submitter checklist

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

core/src/main/java/hudson/cli/CLICommand.java

core/src/main/java/hudson/cli/listeners/CliListener.java

test/src/test/java/hudson/cli/CliListenerTest.java

core/src/main/java/hudson/cli/CLICommand.java

core/src/main/java/hudson/cli/listeners/CliListener.java

core/src/main/java/hudson/cli/listeners/DefaultCliListener.java

test/src/test/java/hudson/cli/CliListenerTest.java

Add CliListener to CLIMethods

aneveux

Looks very good to me, thanks a lot! ❤️

apuig · 2025-03-11T08:40:02Z

core/src/main/java/hudson/cli/CLICommand.java

+        } else if (e instanceof AccessDeniedException) {
+            exitCode = 6;
+            printError(e.getMessage());
+        } else if (e instanceof BadCredentialsException) {


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:pass basic 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,

core/src/main/java/hudson/cli/listeners/CliListener.java

jeromepochat · 2025-03-11T09:15:30Z

core/src/main/java/hudson/cli/CLICommand.java

        this.locale = locale;
        CmdLineParser p = getCmdLineParser();

+        final String correlationId = UUID.randomUUID().toString();


suggestion: The introduction of correlationId common to all events could be highlighted in the PR description.

At this point correlationId is only used in BadCredentialsException handling. (where the original randomUUID comes form)

core/src/main/java/hudson/cli/listeners/CliContext.java

jeromepochat · 2025-03-11T12:30:16Z

Looks good, thanks @apuig !

clarify interface about onComplete not always success remove exitCode from onException remove fire methods args in CliContext rename CliListenerTest

apuig · 2025-03-14T16:33:57Z

please note: the failing test

hudson.cli.DisconnectNodeCommandTest.disconnectNodeManyShouldSucceed

reference CLI, but master also experience problems with this test class

aneveux · 2025-03-17T15:11:58Z

Hello @jenkinsci/core-pr-reviewers 👋

It looks like since @apuig is not a member (yet) of the jenkinsci organization, he cannot ask you for reviews on this PR, and AFAICT, I cannot add a team as reviewers on his PR either.
Could you please have a look at this Pull Request and let us know if there's anything we can do in order to move it towards a merge?

Thanks a lot ❤️

core/src/main/java/hudson/cli/CLICommand.java

krisstern

Makes sense to me

krisstern · 2025-03-19T14:19:15Z

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

alecharp

Looks good to me.

core/src/main/java/hudson/cli/listeners/CliListener.java

core/src/main/java/hudson/cli/CLICommand.java

daniel-beck · 2025-03-19T17:40:50Z

core/src/main/java/hudson/cli/listeners/CliListener.java

+     * @param context Information about the command being executed
+     * @param t Any error during the execution of the command.
+     * */
+    default void onException(@NonNull CliContext context, @NonNull Throwable t) {}


Misleading name if it takes a Throwable? #onError maybe?

I renamed onError to onException to better emphasize that onComplete also handles cases where run returns an error code.
#10382 (comment)
I'm open to suggestions, but I prefer to avoid using onError.

JUnit has failures (unsatisfied assertions) and errors (unexpected exceptions thrown), perhaps we could use one term to refer to "did not succeed" and use the other for "this threw up"?

core/src/main/java/hudson/cli/listeners/CliContext.java

test/src/test/java/hudson/cli/DefaultCliListenerTest.java

core/src/main/java/hudson/cli/listeners/CliContext.java

daniel-beck

Looks great 👍 Thanks!

Would be nice to have the missing test, and perhaps checking the onException name, but not blocking. I'll leave it to @apuig whether to address those.

apuig · 2025-03-21T14:09:44Z

Thanks for the review @daniel-beck . With your hint, I was able to write the missing test.
Regarding naming, we're going with onThrowable, for consistency with both the parameter type and when it's called.

The current failing test PasswordParameterDefinitionTest.defaultValueAlwaysAvailable is also present in master, so I belive it's unrelated to this PR.

Would it be possible to have another review to put back the ready-for-merge lable?

daniel-beck · 2025-03-21T16:04:58Z

I'm going to start the countdown here, but welcome further feedback on the renamed method before we're cementing it forever 😉

A1exKH

LGTM.

jglick · 2025-03-26T16:09:24Z

core/src/main/java/jenkins/cli/listeners/CLIListener.java

+     * Invoked after command execution.
+     *
+     * @param context Information about the command being executed.
+     * @param exitCode Exit code returned by the implementation of {@link CLICommand#run()}.


~~How would this not always be zero? I am presuming you either get onCompleted, with zero, or onThrowable?~~

Never mind, a command could return some nonzero code without throwing an exception.

yeah, that was the tricky part on the naming, indeed onCompleted will handle controlled exit codes from implementations

The Javadoc could probably be clarified to emphasize that either onCompleted or onThrowable is called.

[JENKINS-75378] cli command listener

b41633f

comment-ops-bot bot added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Mar 6, 2025