jenkins
jenkins copied to clipboard
[JENKINS-41516] Add script console listener (alternative/extended proposal)
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
- Fill-in the
- [ ] 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 aProposed 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).
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 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.
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 👍
Tagging the reviewers that approved the previous PR #6539: @NotMyFault @res0nance @timja @StefanSpieker
@daniel-beck perhaps you can tag the reviewer group? :)
One more reviewer! :)
No worries, thanks for the approval! Looking forward to the release 🚀
/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!
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 Any idea why CI passed for this PR?
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?
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.)
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.
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.
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 ^^
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.
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.
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!
Causes JENKINS-72181.