jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

[JENKINS-41516] Add script console listener (alternative/extended proposal)

Open daniel-beck opened this issue 1 year ago • 1 comments

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

See JENKINS-41516.

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).

daniel-beck avatar Sep 02 '22 15:09 daniel-beck

Hi Daniel, Thanks for your investment into this topic! Do you want to implement this PR or https://github.com/jenkinsci/jenkins/pull/6539? For me, both is okay. I am just interested in having the feature available soon :D

meiswjn avatar Sep 14 '22 07:09 meiswjn

@meiswjn FYI I've finished this PR. I have no preference about proceeding with this PR, or integrating the changes here into your PR. Let me know if you want to do the latter.

daniel-beck avatar Jul 28 '23 10:07 daniel-beck

Hey @daniel-beck, thank you! I have no preference either, I am just here for the features 😄 I think it is easiest to keep this one 👍

meiswjn avatar Jul 28 '23 11:07 meiswjn

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

meiswjn avatar Jul 28 '23 11:07 meiswjn

@daniel-beck perhaps you can tag the reviewer group? :)

meiswjn avatar Aug 29 '23 14:08 meiswjn

One more reviewer! :)

meiswjn avatar Oct 02 '23 13:10 meiswjn

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

meiswjn avatar Oct 02 '23 13:10 meiswjn

/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!

timja avatar Oct 02 '23 13:10 timja

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.

NotMyFault avatar Oct 06 '23 08:10 NotMyFault

@NotMyFault Any idea why CI passed for this PR?

daniel-beck avatar Oct 06 '23 09:10 daniel-beck

https://github.com/jenkinsci/jenkins/blob/778b8b62de4e6f42da87cd78dbe8d731a6888cf6/Jenkinsfile#L109

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 avatar Oct 06 '23 09:10 daniel-beck

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.)

daniel-beck avatar Oct 06 '23 09:10 daniel-beck

https://github.com/jenkinsci/jenkins/blob/778b8b62de4e6f42da87cd78dbe8d731a6888cf6/Jenkinsfile#L109

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 https://github.com/jenkinsci/jenkins/blob/778b8b62de4e6f42da87cd78dbe8d731a6888cf6/Jenkinsfile#L113-L116 ?

@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.

NotMyFault avatar Oct 06 '23 09:10 NotMyFault

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.

daniel-beck avatar Oct 06 '23 09:10 daniel-beck

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 ^^

timja avatar Oct 06 '23 10:10 timja

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.

daniel-beck avatar Oct 06 '23 10:10 daniel-beck

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 avatar Oct 06 '23 11:10 MarkEWaite

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!

MarkEWaite avatar Oct 06 '23 15:10 MarkEWaite

Causes JENKINS-72181.

basil avatar Oct 18 '23 19:10 basil