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

Remove YUI #10135

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Remove YUI #10135

wants to merge 8 commits into from

Conversation

timja
Copy link
Member

@timja timja commented Jan 8, 2025

See JENKINS-75100

Now that the disable by default of YUI has been released for ~1 month with no complaints its time to start thinking about removing YUI itself.

We're passed the baseline cut-off for the next LTS which was what @MarkEWaite requested that I wait for before removing YUI fully

What I've left:

  • I've removed CSS where I think its safe but I haven't removed all mentions of yui.
  • getYuiSuffix in Functions, there's a few plugins calling this which need cleaning up
  • l:yui I've changed it to do nothing but its used in a few unmaintained plugins, I could remove this, thoughts?
  • There's a few TODOs that say they could be cleaned up after yui was removed for the component, but hasn't been done yet

Testing done

Clicked around a number of pages and didn't see anything wrong.

Proposed changelog entries

  • Remove the Yahoo! User Interface library

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

@timja timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Jan 8, 2025
@timja timja requested a review from a team January 8, 2025 14:32
@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Jan 8, 2025
@MarkEWaite MarkEWaite added the removed This PR removes a feature or a public API label Jan 8, 2025
@MarkEWaite
Copy link
Contributor

Thanks very much @timja! There are 8 plugins that have just been deprecated in update center as further preparation for this pull request. Only 1 of those plugins has more than 500 installs and it has less than 1000 installs. The pull request that deprecates the plugins is:

I think that we need an entry in the LTS upgrade guide for this removal. We can pattern it after the text in the Prototype.js removal announcement.

@MarkEWaite MarkEWaite added the upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed label Jan 8, 2025

// Button styles

.yui-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

we might have the one or other plugin that use the construct

<span class="yui-button">
  <button ...>
</span>

Those might look broken when we remove all the yui styling here.

Copy link
Member

Choose a reason for hiding this comment

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

Not too many that matter, though. From my selection of checked-out open-source repositories:

atlassian-jira-software-cloud-plugin/src/main/resources/com/atlassian/jira/cloud/jenkins/config/JiraCloudPluginConfig/config.groovy
coverity-plugin/src/main/resources/jenkins/plugins/coverity/CIMStream/DescriptorImpl/defectFilters.jelly
electricflow-plugin/src/main/resources/org/jenkinsci/plugins/electricflow/ElectricFlowAssociateBuildToRelease/config.jelly
electricflow-plugin/src/main/resources/org/jenkinsci/plugins/electricflow/ElectricFlowDeployApplication/config.jelly
electricflow-plugin/src/main/resources/org/jenkinsci/plugins/electricflow/ElectricFlowPipelinePublisher/config.jelly
electricflow-plugin/src/main/resources/org/jenkinsci/plugins/electricflow/ElectricFlowRunProcedure/config.jelly
electricflow-plugin/src/main/resources/org/jenkinsci/plugins/electricflow/ElectricFlowTriggerRelease/config.jelly
hpe-application-automation-tools-plugin/src/main/resources/com/microfocus/application/automation/tools/model/ParallelRunnerEnvironmentModel/config.jelly
jira-ext-plugin/src/main/resources/org/jenkinsci/plugins/jiraext/view/AddLabelToField/config.jelly
jira-ext-plugin/src/main/resources/org/jenkinsci/plugins/jiraext/view/UpdateField/config.jelly
jira-integration-plugin/src/main/webapp/css/sync-with-jira.css
metrics-plugin/src/main/resources/jenkins/metrics/api/MetricsAccessKey/resource.css
qtest-plugin/src/main/resources/com/qasymphony/ci/plugin/action/PushingResultAction/config.jelly
qtest-plugin/src/main/resources/com/qasymphony/ci/plugin/action/SubmitJUnitStep/config.jelly
release-plugin/src/main/resources/hudson/plugins/release/dashboard/RecentReleasesPortlet/main.jelly
synopsys-coverity-plugin/src/main/resources/com/synopsys/integration/jenkins/coverity/extensions/buildstep/CoverityBuildStep/config.jelly
synopsys-coverity-plugin/src/main/resources/com/synopsys/integration/jenkins/coverity/extensions/pipeline/CheckForIssuesStep/config.jelly
synopsys-coverity-plugin/src/main/resources/com/synopsys/integration/jenkins/coverity/extensions/wrap/CoverityEnvironmentWrapper/config.jelly

Hopefully we can rather remove this rather than keeping it, but I could accept keeping it if we have to.

@basil basil added the needs-ath-build Needs to run through the full acceptance-test-harness suite label Jan 8, 2025
@batmat
Copy link
Member

batmat commented Jan 8, 2025

~1 month with no complaints

OTOH, this change was already released on weeklies only. Aren't these users a special profile? In a ideal world with no time limits/constrains, we would have shipped the change of the default value to an LTS release, isn't it?
And then if there there's no negative feedback for ~1 month, I would definitely agree with this stance.

WDYT?

@timja
Copy link
Member Author

timja commented Jan 8, 2025

~1 month with no complaints

OTOH, this change was already released on weeklies only. Aren't these users a special profile?

Generally we get feedback pretty quickly if something is broken in weeklies. I don't expect delaying much longer to add more value but not in a big rush.

Most of the code here hopefully is separate enough to not conflict

@basil
Copy link
Member

basil commented Jan 8, 2025

While working on the Jakarta EE 9 project, I noticed that a big wave of users upgraded to the latest weekly and reported issues, and then no issues showed up for a while until a second big wave of users upgraded to the latest LTS and reported issues. So I agree with Baptiste that there are a few profiles of users and that we don't necessarily get the full picture of user feedback until LTS ships.

I also agree with Tim that the risk is not too high with this removal. With regard to CSS, there might be a few visual regressions that can and should be fixed, but at least the functionality would still work. With regard to JavaScript, breakage would be more serious, but we have found relatively few plugins in the ecosystem that still have YUI JavaScript, and even fewer that are highly popular. My impression is that the same pattern was found in CloudBees proprietary plugins as well.

If we are still concerned about breakage, it might be possible to split the CSS and JavaScript removal into two separate phases/PRs, but even that seems likely overkill to me and I am mostly mentioning the idea for completeness.

Copy link

@A1exKH A1exKH left a comment

Choose a reason for hiding this comment

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

@timja LGTM.

@daniel-beck
Copy link
Member

would have shipped the change of the default value to an LTS release, isn't it?

The upcoming LTS line (looks like it'll be 2.492.x) will have YUI disabled by default, giving plugins 3 months to fix forward any newly discovered issues, while allowing affected installs to re-enable it. IMO it's unlikely it'll be so bad that we'd consider leaving YUI in core, and if it is, we can always revert this PR.

@timja timja mentioned this pull request Jan 9, 2025
6 tasks
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Code changes look reasonable. I also reviewed leftover yui mentions and nothing looks like it needs addressing now.

core/src/main/resources/lib/layout/yui.jelly Outdated Show resolved Hide resolved
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Once this is fully tested in ATH and PCT, this looks ready to me from an open-source Jenkins perspective. I will approve this PR once it is labelled as passing ATH and PCT.

I am not able to comment with great accuracy on whether CloudBees is quite ready for this in weekly releases, but my impression from a distance is that CloudBees is likely close to being ready for this. I will let others (@MarkEWaite? @scherler? someone else?) leave their thoughts about timing if necessary.

core/src/main/java/hudson/Functions.java Outdated Show resolved Hide resolved
core/src/main/java/hudson/Functions.java Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ath-build Needs to run through the full acceptance-test-harness suite removed This PR removes a feature or a public API rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed 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.

7 participants