-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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-59136] Enable labels/categories with deprecated
to show a warning for plugins
#4073
[JENKINS-59136] Enable labels/categories with deprecated
to show a warning for plugins
#4073
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a great concept we could add. One note is that "The following plugins can be uninstalled as they have been marked as deprecated" suggests that the plugin is no longer used/needed. It might be not a case for such deprecation event (e.g. Perforce plugin is deprecated but there is no full replacement). would just add a notice that the plugins as deprecated without any further suggestions for now. In such case users can go to plugin documentation and see the recommended actions there
The deprecation text could read something like this: "The following plugins have been marked deprecated, please check recommendation on plugins.site or GitHub." and then each entry will link to those? |
<dl> | ||
<dt>${%DeprecatedPlugins}</dt> | ||
<j:forEach var="p" items="${it.deprecatedPlugins}"> | ||
<dd><j:out value="${p.longName} v${p.version}"/></dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May introduce an XSS vulnerability through plugin display names if the value is not filtered. Do not use j:out
on untrusted input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the alternative? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<dd><j:out value="${p.longName} v${p.version}"/></dd> | |
<dd>${p.longName} v${p.version}</dd> |
would be equivalent except for being not unsafe.
<dd><j:out value="${p.longName} v${p.version}"/></dd> | |
<dd><a href="${p.url}" target="_blank">${p.longName} ${p.version}</dd> |
would also link to the plugin docs (and remove the v
before the version, something I don't think we're using in general?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.url can be empty 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then check for that and don't link if it's bad.
Also don't link if not http/https.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you consider a bad link?
core/src/main/resources/hudson/PluginManager/PluginDeprecationMonitor/message.jelly
Outdated
Show resolved
Hide resolved
core/src/main/resources/hudson/PluginManager/PluginDeprecationMonitor/message.properties
Outdated
Show resolved
Hide resolved
Would it be possible to extend the solution so that each plugin can point to the documentation of its replacement? For plugins that host the documentation outside of Jenkins wiki it may be impossible to add migration information to the documentation of the deprecated plugin. (Note that there are some plans to support moving plugin docs out of the wiki, see https://issues.jenkins-ci.org/browse/WEBSITE-406 ). |
Open to ideas/solutions @zbynek 👍 |
Depends on how we want to tackle this. In general, plugins whose URL points to some page we have no control over can potentially have a problem here. I see the following options:
|
This solution does not work for plugins that are no longer being distributed. In the case of security warnings, I was very deliberate to separate out the data to allow suspension of plugin distribution while still showing security warnings (and I think that has been pretty successful). This case is similar: We may want to mark a plugin as deprecated and simultaneously suspend distribution, e.g. when it's integrating with a service that has been shut down. If you check comments on https://github.com/jenkins-infra/update-center2/blob/master/src/main/resources/artifact-ignores.properties you will see many examples. Not strongly advocating for a complete overhaul both here and in the infra, with completely new metadata — but still something to consider. While it would be feasible to start with this and improve over time, we might end up having to choose between duplicate efforts in infra (maintaining label and |
I am up for improving Jenkins infra to support this effort 👍 |
core/src/main/resources/hudson/PluginManager/PluginDeprecationMonitor/message.properties
Outdated
Show resolved
Hide resolved
I think something similar to Perhaps sent something to the dev mailing list and perhaps we could have a meeting discussing what would be needed to implement this feature. |
Works for me for initial ideas gathering. Would recommend going with a JEP for the writeup afterwards. |
Maybe it should be just "Warnings Engine" then? |
@Casz Any plans to update this PR based on the discussion above to take care of plugins that have been suspended? |
Question is how to proceed, could you point me in the direction of warnings.json parser? |
@Casz Technically it's really straightforward -- https://github.com/jenkins-infra/update-center2/blob/ff6e8947b8a65301b2e9c6e00e35b8d90d8bdf85/src/main/java/org/jvnet/hudson/update_center/Main.java#L324-L339 is all there is. We could probably make the server side of this nicer by making the labels file the canonical data source for deprecation, but as we can adjust that at any time, no need to go beyond MVP right now. Client/core side, #2680 should have everything. Notable additions over this PR, feature-wise, are the ability to dismiss specific warnings without dismissing the admin monitor indefinitely, as well as the integration on the plugin manager UI (which is still rather basic, I think there's no transitive/dependency warning support). |
I think you can safely remove |
CC @timja who set on-hold previously. |
Let's try to merge this before its anniversary 🎉 😄 |
Or we could ship it on its anniversary 😂 |
@daniel-beck much appreciated for driving it forward 😅 👏 👏 👏 |
@jenkinsci/code-reviewers PTAL |
Belated happy birthday! 🎉 🎈 |
Looks like we have enough reviews and others have had plenty of time. We'll merge this after 24 hours if there's no objections. Thanks @jetersen for the initial implementation and @daniel-beck for the re-work and productionising of it. This will be a useful feature to help cleanup unneeded plugins that have been EOLed. |
See JENKINS-59136. Related to/downstream of jenkins-infra/update-center2#344. Triggered by this discussion in GItter
This admin monitor does not yet have a built-in dismiss button, and doesn't allow hiding individual plugins, like the security warnings monitor does. If there's a need we can add this later.
Testing notes
docker run --rm -ti -p 8080:8080 -e ID=4073 jenkins/core-pr-tester
.Some plugins get their deprecation URLs from https://github.com/jenkins-infra/update-center2/blob/master/resources/deprecations.properties, other deprecations are driven by regular labels, and those plugins use the normal plugin URL (to the plugin site).
Proposed changelog entries
deprecated
in update site metadata will now have a banner about it, and if any are installed, a notification is shown to admins.Submitter checklist
* Use the
Internal:
prefix if the change has no user-visible impact (API, test frameworks, etc.)Desired reviewers
@daniel-beck @oleg-nenashev @stopalopa