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

Add tombstone page for suspended plugins #669

Merged
merged 5 commits into from
May 11, 2021
Merged

Add tombstone page for suspended plugins #669

merged 5 commits into from
May 11, 2021

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented May 8, 2021

Related to issue #629

Summary of this pull request: simple tombstone page for plugins that were removed from distribution.
image

May be extended later by providing human-readable title of the plugin and maybe some other metadata (a link to GitHub repo in case someone wants to revive the plugin?), but that would require changes in other components.

CC @daniel-beck

@zbynek zbynek requested a review from a team as a code owner May 8, 2021 20:18
const updateData = await requestGET({url: updateUrl, reporter});
const suspendedPromises = [];
for (const deprecation of Object.keys(updateData.deprecations)) {
if (names.indexOf(deprecation) == -1) {
Copy link
Member

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes
I recommend names.includes(deprecation) instead of indexof. Also I think performance wise, a set is faster than a straight up array, but i doubt it matters for this.

parent: null,
children: [],
internal: {
type: 'JenkinsPlugin',
Copy link
Member

Choose a reason for hiding this comment

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

Lets make this a completely different type. It only needs name, id and url right? then you can just loop through the new type in gatsby-node.js

Just simplifies things and keeps them seperate

Copy link
Contributor

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

Thanks for taking care of this!

@@ -159,6 +161,26 @@ const fetchPluginData = async ({createNode, reporter}) => {
page = pluginsContainer.page + 1;
} while (!page || pluginsContainer.page < pluginsContainer.pages);
await Promise.all(promises);
const updateUrl = 'https://updates.jenkins.io/current/update-center.actual.json';
Copy link
Contributor

@daniel-beck daniel-beck May 8, 2021

Choose a reason for hiding this comment

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

Suggested change
const updateUrl = 'https://updates.jenkins.io/current/update-center.actual.json';
const updateUrl = 'https://updates.jenkins.io/update-center.actual.json';

Better to not rely on this implementation detail.

const updateUrl = 'https://updates.jenkins.io/current/update-center.actual.json';
const updateData = await requestGET({url: updateUrl, reporter});
const suspendedPromises = [];
for (const deprecation of Object.keys(updateData.deprecations)) {
Copy link
Contributor

@daniel-beck daniel-beck May 8, 2021

Choose a reason for hiding this comment

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

This does not actually list plugins whose distribution is suspended, but those marked deprecated -- this feature exists just so that plugins can be marked deprecated while being suspended, but having the customizable URL is a benefit that could result in plugins being marked deprecated through this feature even while still being distributed.

Some plugins have been removed without having an entry here (e.g. emotional-hudson). Other plugins may have an entry here without being suspended (unlikely, but as I wrote, possible as it has a benefit).

At a minimum, this needs to cross-check that no plugin found here is still actively distributed before just showing the tombstone page. Then we can just accept that some (few) plugins still don't show up.

The alternative is parsing https://github.com/jenkins-infra/update-center2/blob/master/resources/artifact-ignores.properties and relying on ugly internals of that tool, or extending it to list what's suspended in an additional output file. As a start, the current approach is pretty reasonable if we ensure no currently distributed plugin would have a tombstone page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other plugins may have an entry here without being suspended

there is a check for the false positives already. For false negatives we can manually check the artifact-ignores.properties and add some more entries to the deprecations if needed (if it's not worth showing as deprecated in Jenkins, it probably doesn't need a tombstone page either).

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, then this is good to go from my POV 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plugins with more than 100 installs (as of April 1) that don't have a deprecation entry:

7205 copy-to-slave
5930 build-flow-plugin
4691 tfs
3093 dynamicparameter
1629 perforce
1276 svn-tag
1047 kubernetes-pipeline-steps
761 scripttrigger
595 grails
559 kubernetes-pipeline-aggregator
558 kubernetes-pipeline-arquillian-steps
435 kubernetes-ci
367 zap-pipeline
333 persona
278 jshint-checkstyle
230 xtrigger
229 play-autotest-plugin
200 pipeline-classpath
188 puppet-enterprise-pipeline
184 simple-travis-runner
181 groovyaxis
162 gcm-notification
152 cvs-tag
148 fortify360
101 jslint-checkstyle

@oleg-nenashev
Copy link
Contributor

It sis definitely a great start! Just having a tombstone will be very helpful so that plugin links are preserved. We can iterate on that in subsequent pull requests

Copy link
Member

@halkeye halkeye left a comment

Choose a reason for hiding this comment

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

Looks good to me

src/templates/tombstone.jsx Outdated Show resolved Hide resolved
zbynek and others added 3 commits May 9, 2021 22:28
Co-authored-by: Daniel Beck <1831569+daniel-beck@users.noreply.github.com>
@zbynek zbynek merged commit 49a25c6 into master May 11, 2021
@zbynek zbynek deleted the tombstone branch May 11, 2021 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants