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-59136] Enable labels/categories with deprecated to show a warning for plugins #4073

Merged
merged 27 commits into from Jul 15, 2020
Merged

[JENKINS-59136] Enable labels/categories with deprecated to show a warning for plugins #4073

merged 27 commits into from Jul 15, 2020

Conversation

jetersen
Copy link
Member

@jetersen jetersen commented Jun 17, 2019

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

  • Plugins marked as 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

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@daniel-beck @oleg-nenashev @stopalopa

Copy link
Member

@oleg-nenashev oleg-nenashev left a 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

@jetersen
Copy link
Member Author

jetersen commented Jun 17, 2019

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>
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the alternative? 😅

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<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.

Suggested change
<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?).

Copy link
Member Author

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 😢

Copy link
Member

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.

Copy link
Member Author

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?

Co-Authored-By: Daniel Beck <1831569+daniel-beck@users.noreply.github.com>
@zbynek
Copy link
Contributor

zbynek commented Jun 17, 2019

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

@jetersen
Copy link
Member Author

Open to ideas/solutions @zbynek 👍
Update center has SCM data, I wonder why it is not exposed in the plugin data 😕
I'd like to use it at least.
getInfo obtains the "wiki" ie. plugins site https://plugins.jenkins.io/cloudbees-folder

@daniel-beck
Copy link
Member

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

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:

  • Add more metadata to deprecations, rather than just what amounts to a flag, necessitating a completely different approach (more similar to security warnings)
  • Send the user to the plugin URL provided by the update center, if it exists (typically plugins.jenkins.io), instead of the URL provided by the plugin itself (typically a wiki page). Through https://github.com/jenkins-infra/update-center2#wiki-page-override we could then modify what would be included/referenced on http://plugins.jenkins.io/ and make the entire experience somewhat usable.

@daniel-beck
Copy link
Member

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 warnings.json-like better metadata in parallel), or dropping support for this variant of deprecation notices.

@jetersen
Copy link
Member Author

I am up for improving Jenkins infra to support this effort 👍
Skipping the basic implementation 😅

@jetersen
Copy link
Member Author

jetersen commented Jun 18, 2019

I think something similar to warnings.json would be feasible
And I would rather implement it the right way now than having to have double maintenance.

Perhaps sent something to the dev mailing list and perhaps we could have a meeting discussing what would be needed to implement this feature.
WDYT @daniel-beck

@daniel-beck
Copy link
Member

Works for me for initial ideas gathering. Would recommend going with a JEP for the writeup afterwards.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jun 19, 2019

Maybe it should be just "Warnings Engine" then?

@oleg-nenashev oleg-nenashev added the web-ui The PR includes WebUI changes which may need special expertise label Jun 27, 2019
@jetersen
Copy link
Member Author

@daniel-beck daniel-beck added the plugin-api-changes Changes the API of Jenkins available for use in plugins. label Oct 11, 2019
jetersen and others added 3 commits October 12, 2019 11:13
Co-Authored-By: Tim Jacomb <t.jacomb@kainos.com>
@daniel-beck
Copy link
Member

@Casz Any plans to update this PR based on the discussion above to take care of plugins that have been suspended?

@jetersen
Copy link
Member Author

Question is how to proceed, could you point me in the direction of warnings.json parser?

@daniel-beck
Copy link
Member

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

@jetersen
Copy link
Member Author

jetersen commented May 26, 2020

I think you can safely remove on-hold and needs-more-reviews due to jenkinsci/pipeline-model-definition-plugin#386 (comment)

@daniel-beck daniel-beck removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label May 26, 2020
@daniel-beck
Copy link
Member

CC @timja who set on-hold previously.

@daniel-beck daniel-beck added squash-merge-me Unclean or useless commit history, should be merged only with squash-merge and removed plugin-api-changes Changes the API of Jenkins available for use in plugins. labels Jun 9, 2020
@daniel-beck
Copy link
Member

Let's try to merge this before its anniversary 🎉 😄

@timja
Copy link
Member

timja commented Jun 9, 2020

Let's try to merge this before its anniversary 🎉 😄

Or we could ship it on its anniversary 😂

@jetersen
Copy link
Member Author

jetersen commented Jun 9, 2020

@daniel-beck much appreciated for driving it forward 😅 👏 👏 👏

@daniel-beck
Copy link
Member

@jenkinsci/code-reviewers PTAL

@daniel-beck
Copy link
Member

Belated happy birthday! 🎉 🎈

@timja timja added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Jul 12, 2020
@timja
Copy link
Member

timja commented Jul 12, 2020

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.

@timja timja merged commit 55a523e into jenkinsci:master Jul 15, 2020
@jetersen jetersen deleted the feature/add-deprecated-plugin-monitor branch July 15, 2020 21:39
@oleg-nenashev oleg-nenashev added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Jul 20, 2020
@daniel-beck daniel-beck mentioned this pull request Jul 27, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 squash-merge-me Unclean or useless commit history, should be merged only with squash-merge web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
10 participants