Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-73563] create a jenkins-button instead of a yui button in makeButton #9511

Merged
merged 6 commits into from
Aug 10, 2024

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Jul 30, 2024

See JENKINS-73563

makeButton is one of the last places in core were YUI is used. This method is called by a few plugins directly but also when plugins create inputs of type button or submit with class submit-button or yui-button. Frequently they then also attach an onclick handler.

This change will replace the input with a button with class jenkins-button on the fly.
Looking at the code of several plugins that rely on the behaviour it seems they will not be affected by this change (tested with shelve-plugin)

credentials plugin works with the returned YUI object and try to set the button to disabled or get the associated form, for this case a small wrapper is added that implements the 2 methods and credentials plugin still works without problems.
There is PR open for credentials that cleans up the code and removes yui usage
(Another usage affects the multi-slave-config plugin which is deprecated and not usable anyway due to prototype usage)

See https://docs.google.com/spreadsheets/d/1UjvtFmNmEdjMN5DUoFxJfBryA8q-E5_HwOzVKbVG9b0/edit?usp=sharing for a complete list of plugins that use makeButton directly or indirectly.

Need to test if ath needs to be adjusted

Testing done

Manual testing

Proposed changelog entries

  • makeButton creates jenkins-buttons on the fly instead of using YUI

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

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

Maintainer checklist

Preview Give feedback

makeButton is one of the last places in core were YUI is used.
This method is called by a few plugins directly but also when plugins
create inputs of type button with class `submit-button` or `yui-button`.
Frequently they then also attach an onclick handler.

This change will replace the input with a button with class
`jenkins-button` on the fly.
Looking at the code of several plugins that rely on the behaviour it
seems they will not be affected by this change.
@timja timja added needs-ath-build Needs to run through the full acceptance-test-harness suite web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Jul 31, 2024
@@ -913,47 +913,68 @@ function escapeHTML(html) {
* YUI Button widget.
*/
function makeButton(e, onclick) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs above could do with an update as it no longer is a YUI button

@timja timja requested a review from a team July 31, 2024 08:03
@timja timja added the needs-security-review Awaiting review by a security team member label Jul 31, 2024
@timja timja requested a review from a team July 31, 2024 11:09
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm, will approve once i've had time to manually test it

@timja timja requested a review from a team July 31, 2024 11:09
@mawinter69
Copy link
Contributor Author

ath is passing

@mawinter69 mawinter69 changed the title create a jenkins-button instead of a yui button in makeButton [JENKINS-73563] create a jenkins-button instead of a yui button in makeButton Aug 2, 2024
Co-authored-by: Kevin Guerroudj <91883215+Kevin-CB@users.noreply.github.com>
@Kevin-CB Kevin-CB added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Aug 7, 2024
Copy link
Contributor

@Kevin-CB Kevin-CB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change, LGTM security wise

@timja timja added ath-successful This PR has successfully passed the full acceptance-test-harness suite and removed needs-ath-build Needs to run through the full acceptance-test-harness suite labels Aug 7, 2024
@timja
Copy link
Member

timja commented Aug 7, 2024

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

@comment-ops-bot 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 Aug 7, 2024
@timja timja merged commit b349b6b into jenkinsci:master Aug 10, 2024
16 checks passed
@basil basil added the pct-fail The plugin-compatibility-test suite needs a forward-compatible change label Aug 12, 2024
@basil
Copy link
Member

basil commented Aug 12, 2024

Breaks PCT:

  • hudson.security.LDAPEmbeddedTest#validateUI
  • org.jenkinsci.plugins.docker.commons.credentials.DockerServerDomainSpecificationTest#configRoundTrip

I am reverting this commit so that the 2.472 weekly release can be delivered tomorrow with Jetty 12 (EE 8) and without PCT failures. This can be reintegrated when there are no PCT failures.

@MarkEWaite
Copy link
Contributor

I've temporarily added the skip-changelog label so that this commit won't be included in the Jenkins 2.472 changelog. When it is reintegrated, we'll include it in the changelog for the matching release.

@mawinter69
Copy link
Contributor Author

Seems that htmlunit doesn't support the forEach method of a DOMTokenList. Should be no big deal to rewrite the code to avoid the forEach and instead loop over the list manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite pct-fail The plugin-compatibility-test suite needs a forward-compatible change ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues skip-changelog Should not be shown in the changelog web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants