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

[FIXED JENKINS-21485] AdministrativeMonitor for plugins that have failed #2234

Closed
wants to merge 2 commits into from

Conversation

@fbelzunc
Copy link
Contributor

fbelzunc commented Apr 6, 2016

This PR substitute #2098 in case we keep @Vlatombe's PR #2172.

Notice that I needed to do:

public final static PluginWrapperAdministrativeMonitor NOTICE = new PluginWrapperAdministrativeMonitor();

because it looks like the Extensions are not leaded at this point.

CC @jglick @daniel-beck @Vlatombe

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Apr 6, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@daniel-beck daniel-beck closed this Apr 6, 2016
@daniel-beck daniel-beck reopened this Apr 6, 2016
@@ -567,13 +571,16 @@ public boolean hasLicensesXml() {
PluginWrapper dependency = parent.getPlugin(d.shortName);
if (dependency == null) {
missingDependencies.add(d.toString());
NOTICE.addErrorMessage(Messages.PluginWrapper_admonitor_MissingDependency(shortName, dependency.getLongName()));

This comment has been minimized.

Copy link
@Vlatombe

Vlatombe Apr 7, 2016

Member

NPE here.

PluginWrapper.admonitor.OutdatedCoreVersion=Plugin {0} requires Jenkins {1} or later
PluginWrapper.admonitor.MissingDependency=Plugin {0} doesn\u2019t have installed the required dependency {1}.
PluginWrapper.admonitor.DisabledDependency=Plugin {0} has disabled the required dependency {1}

This comment has been minimized.

Copy link
@Vlatombe

Vlatombe Apr 7, 2016

Member

Plugin ssh-credentials has disabled the required dependency Credentials Plugin

I would rephrase as Plugin {0} depends on the disabled {1}.

@@ -555,6 +558,7 @@ public boolean hasLicensesXml() {
LOGGER.warning(shortName + " doesn't declare required core version.");
} else {
if (Jenkins.getVersion().isOlderThan(new VersionNumber(requiredCoreVersion))) {
NOTICE.addErrorMessage(Messages.PluginWrapper_admonitor_OutdatedCoreVersion(shortName, requiredCoreVersion));

This comment has been minimized.

Copy link
@Vlatombe

Vlatombe Apr 7, 2016

Member

Use getLongName() instead of shortName for consistency? Also applicable for the next messages.


PluginWrapper.admonitor.OutdatedCoreVersion=Plugin {0} requires Jenkins {1} or later
PluginWrapper.admonitor.MissingDependency=Plugin {0} doesn\u2019t have installed the required dependency {1}.

This comment has been minimized.

Copy link
@Vlatombe

Vlatombe Apr 7, 2016

Member

Plugin ssh-slaves doesn’t have installed the required dependency ssh-credentials.

I suggest Plugin {0} requires the missing plugin {1}. instead.

@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Apr 8, 2016

Should we restore #2172 in this PR as well since it got reverted in 2.0?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 3, 2016

Not ready to merge. @fbelzunc Are you working on it?

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented May 4, 2016

@Vlatombe @fbelzunc I'd love to see #2172 and this revived soon, so we make sure plugin loading is as solid and errors as noticeable and understandable as we can make it (JENKINS-34073).

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Aug 2, 2016

Superseded by #2487.

@daniel-beck daniel-beck closed this Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.