[JENKINS-41516] Add script console listener by daniel-beck · Pull Request #7056 · jenkinsci/jenkins

daniel-beck · 2022-09-02T15:38:52Z

Alternative to #6539 (but might still get reintegrated there) by @meiswjn, for now just to get a PR build/incremental deployment.

See JENKINS-41516.

Minor secondary TODOs:

Add implementation of extension point outside core (IMO kinda optional since DefaultScriptListener exists)

Proposed changelog entries

Developer: Add extension point to notify about in-process scripting events.

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, 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 directives (see documentation on jenkins.io).
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

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

There are at least 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 does not block the change
Changelog entries in the PR 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, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (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).

…com/meiswjn/jenkins into feature/add-script-console-listener

Co-authored-by: Wadeck Follonier <Wadeck@users.noreply.github.com>

…com/meiswjn/jenkins into feature/add-script-console-listener

timja

nice

meiswjn · 2023-10-02T13:24:05Z

One more reviewer! :)

NotMyFault

Tagging the reviewers that approved the previous PR #6539: NotMyFault @res0nance timja StefanSpieker

My humble apology, I must have missed this PR :(

meiswjn · 2023-10-02T13:30:37Z

No worries, thanks for the approval! Looking forward to the release 🚀

timja · 2023-10-02T13:52:05Z

/label ready-for-merge

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

Thanks!

NotMyFault · 2023-10-06T08:15:45Z

The tests added in this PR cause failures in the default branch for Windows agents: https://ci.jenkins.io/job/Core/job/jenkins/job/master/5460/testReport/jenkins.model/

Would you mind taking a look and think you can submit a fixup before next Tuesday? Otherwise, I'd revert the merge to restore the test functionality and not break the weekly release.

daniel-beck · 2023-10-06T09:06:57Z

@NotMyFault Any idea why CI passed for this PR?

daniel-beck · 2023-10-06T09:12:06Z

jenkins/Jenkinsfile

Line 109 in 778b8b6

def target = platform == 'windows' ? '30%' : '100%'

So we're running every test on Linux 3 times (11, 17, and 19), but less than a third of tests on Windows?

daniel-beck · 2023-10-06T09:13:38Z

How do I even confirm a fix PR will work, if this just skips execution of the test I'm fixing?

(Also, seems like a clear bug to not execute new tests.)

NotMyFault · 2023-10-06T09:24:51Z

jenkins/Jenkinsfile

Line 109 in 778b8b6

def target = platform == 'windows' ? '30%' : '100%'

So we're running every test on Linux 3 times (11, 17, and 19), but less than a third of tests on Windows?

But that's only used for the launchable part, isn't it

jenkins/Jenkinsfile

Lines 113 to 116 in 778b8b6

    
             sh "launchable verify && launchable subset --session ${session} --target ${target} --get-tests-from-previous-sessions --output-exclusion-rules maven >${excludesFile}" 
        
           } else { 
        
             excludesFile = "${tmpDir}\\excludes.txt" 
        
             bat "launchable verify && launchable subset --session ${session} --target ${target}% --get-tests-from-previous-sessions --output-exclusion-rules maven >${excludesFile}"

?

@NotMyFault Any idea why CI passed for this PR?

To be fair, it passed at the end of July, I could've updated this PR before integrating it into master.

daniel-beck · 2023-10-06T09:27:54Z

But that's only used for the launchable part, isn't it

Unsure what you mean. https://ci.jenkins.io/job/Core/job/jenkins/job/PR-7056/12/consoleFull did not run the new test on Windows, just three times on Linux. See https://ci.jenkins.io/job/Core/job/jenkins/job/PR-7056/12/execution/node/273/log/?consoleFull for the Windows output specifically.

timja · 2023-10-06T10:08:55Z

How do I even confirm a fix PR will work, if this just skips execution of the test I'm fixing?

(Also, seems like a clear bug to not execute new tests.)

cc @basil if you have any input on the above ^^

daniel-beck · 2023-10-06T10:33:40Z

How do I even confirm a fix PR will work, if this just skips execution of the test I'm fixing?

Given that it's being executed in an unrelated PR now, this comment is probably obsolete.

MarkEWaite · 2023-10-06T11:53:12Z

Thanks for reporting the issue.

Launchable was enabled months ago to reduce the cost of pull request evaluation. Launchable uses results from previous tests to define a subset of Windows tests that will execute in roughly the same amount of time as the Linux builds require. The full set of Windows tests are run on the master branch after the pull request is merged.

This is the first time since Launchable was enabled that we've had a test failure "escape" from a pull request merge and reach the master branch. I think that the cost savings from limiting Windows tests to approximately one hour (instead of the four hours required on the main branch) are worth this type of rare event.

I agree that the test selection algorithm used by Launchable should add all new tests to the list of tests that it runs. We'll need to report that to them, since others will likely want the same thing. When there is no test result data for a test, add it to the executed set of tests.

MarkEWaite · 2023-10-06T15:52:05Z

I agree that the test selection algorithm used by Launchable should add all new tests to the list of tests that it runs. We'll need to report that to them, since others will likely want the same thing. When there is no test result data for a test, add it to the executed set of tests.

I've reported the issue to Launchable support and copied Kohsuke on the report so that he's aware. Thanks again for detecting the issue!

jglick · 2023-10-10T11:43:59Z

+     * {@link java.io.Writer} that calls {@link #fireScriptOutput(String, Object, Object, String, hudson.model.User)} with the
+     * output it writes to the wrapped {@link java.io.Writer}, and otherwise just forwards {@link #flush()} and {@link #close()}.
+     */
+    class ListenerWriter extends Writer {


This is implicitly public. If you did you not mean for this to be part of the API, use @Restricted(NoExternalUse.class). Ditto other members of the interface.

Could go either way. Realistically there are few (possibly none) possible callers of #fire… methods outside core (that possibly could want this), so it might be better to start off @Restricted and open up if necessary.

jglick · 2023-10-10T11:44:47Z

+/**
+ * Basic default implementation of {@link jenkins.util.ScriptListener} that just logs.
+ *
+ * @since TODO


Should be omitted because it is not part of the API anyway.

I like it to quickly see when a feature was added. While I could get there via git blame, given the negligible cost, this is reasonable to have IMO.

basil · 2023-10-18T19:33:41Z

Causes JENKINS-72181.

meiswjn and others added 30 commits April 26, 2022 14:52

Set location for listener

e745089

Add ScriptListener

060fe1e

Remove debugging statements

89ac091

Merge branch 'master' into feature/add-script-console-listener

2aed668

Add listener for CLI

8d41ad1

Merge branch 'master' into feature/add-script-console-listener

5160902

Merge branch 'jenkinsci:master' into feature/add-script-console-listener

6b86d43

Add new listener for other sources

dc7728f

Simplify listener

c533407

Merge branch 'master' into feature/add-script-console-listener

8e9e618

Fix whitespace and comments

02ffdd1

Merge branch 'feature/add-script-console-listener' of https://github.…

0ceef7c

…com/meiswjn/jenkins into feature/add-script-console-listener

Merge branch 'master' into feature/add-script-console-listener

d30108d

Merge branch 'master' into feature/add-script-console-listener

a2ede02

Update core/src/main/java/hudson/cli/GroovyCommand.java

70b1334

Co-authored-by: Wadeck Follonier <Wadeck@users.noreply.github.com>

Add precise origins; Correctly log CLI input

eadd842

Merge branch 'jenkinsci:master' into feature/add-script-console-listener

cc4ce09

Merge branch 'feature/add-script-console-listener' of https://github.…

a4a541c

…com/meiswjn/jenkins into feature/add-script-console-listener

Add LoggingGroovySh as suggested by @daniel-beck

301dea2

Add ScriptListener Tests

f1e5b03

Merge branch 'master' into feature/add-script-console-listener

fcf5506

Merge branch 'master' into feature/add-script-console-listener

df5b49c

Add basic default listener that just logs

567e03b

WIP Script Listener

9ae14b4

Cleaner ScriptListener API

6b43b1e

Add license

91bc8a9

Cleaner Binding stringification

f79a9eb

Note TODO

db09ba6

Add Object feature parameters

d6c64a3

Thanks Checkstyle

c9d641f

daniel-beck added the needs-more-reviews Complex change, which would benefit from more eyes label Aug 29, 2023

timja approved these changes Sep 4, 2023

View reviewed changes

timja requested a review from a team September 4, 2023 14:25

StefanSpieker approved these changes Sep 5, 2023

View reviewed changes

NotMyFault approved these changes Oct 2, 2023

View reviewed changes

NotMyFault added developer Changes which impact plugin developers and removed needs-more-reviews Complex change, which would benefit from more eyes labels Oct 2, 2023

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 2, 2023

NotMyFault merged commit 270062a into jenkinsci:master Oct 5, 2023

NotMyFault changed the title ~~[JENKINS-41516] Add script console listener (alternative/extended proposal)~~ [JENKINS-41516] Add script console listener Oct 6, 2023

daniel-beck mentioned this pull request Oct 6, 2023

Fix tests that the PR build did not run on Windows #8574

Merged

jglick reviewed Oct 10, 2023

View reviewed changes

daniel-beck mentioned this pull request Oct 10, 2023

Restrict helper classes in ScriptListener for internal use #8588

Merged

daniel-beck mentioned this pull request Oct 18, 2023

[JENKINS-72181] Restore flushed output for 'groovy' CLI command #8622

Merged

This was referenced Mar 21, 2024

[JENKINS-68404] Add ScriptUsageListener to track Groovy scripts jenkinsci/audit-trail-plugin#72

Merged

[JENKINS-68404] Add script listener to track usage jenkinsci/script-security-plugin#416

Draft

Uh oh!

Conversation

daniel-beck commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

Uh oh!

timja left a comment

Choose a reason for hiding this comment

Uh oh!

meiswjn commented Oct 2, 2023

Uh oh!

NotMyFault left a comment

Choose a reason for hiding this comment

Uh oh!

meiswjn commented Oct 2, 2023

Uh oh!

timja commented Oct 2, 2023

Uh oh!

NotMyFault commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daniel-beck commented Oct 6, 2023

Uh oh!

daniel-beck commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daniel-beck commented Oct 6, 2023

Uh oh!

NotMyFault commented Oct 6, 2023

Uh oh!

daniel-beck commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timja commented Oct 6, 2023

Uh oh!

daniel-beck commented Oct 6, 2023

Uh oh!

MarkEWaite commented Oct 6, 2023

Uh oh!

MarkEWaite commented Oct 6, 2023

Uh oh!

jglick Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

daniel-beck Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

daniel-beck Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

jglick Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

daniel-beck Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

basil commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

daniel-beck commented Sep 2, 2022 •

edited

Loading

NotMyFault commented Oct 6, 2023 •

edited

Loading

daniel-beck commented Oct 6, 2023 •

edited

Loading

daniel-beck commented Oct 6, 2023 •

edited

Loading