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-27177] Add disable plugin command and tests #3648

Merged
merged 11 commits into from Nov 8, 2018

Conversation

@MRamonLeon
Copy link
Contributor

commented Sep 25, 2018

See JENKINS-27177.

Added a new CLI command disable-plugin to disable one or more plugins and optionally restart Jenkins.

Behavior:

  • The info about the plugins successfully disabled are shown in the output console.
  • If a plugin is already disabled, it show it in the output and exit normally (error code 0).
  • If a plugin in the list can't be disabled, the process continues up to the end of the list of plugins passed and the command exits with error code 16 and the message about this plugin is output in the error console.

Proposed changelog entries

  • JENKINS-27177: Add a new CLI command disable-plugin to disable one ore more installed plugins and optionally restart Jenkins.

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
  • 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

@reviewbybees @jvz

screenshot from 2018-09-25 14-54-24

If you call it with:
java -jar jenkins-cli.jar ... disable-plugin variant dependee mandatory-depender depender

It responds with:

The plugin variant was already disabled.
The plugin dependee has, at least, one dependant plugin (mandatory-depender), so it cannot be disabled.
Plugin mandatory-depender disabled.
Plugin depender disabled.

Return code: 16 (because one plugin couldn't be disabled)

@amuniz

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

I think this CLI will need a more complex implementation to work properly.

Let's say plugin C depends on B and B depends on A. If you pass C, B, A the command (as written now) will do its job, however A, B, C won't work.

@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

I think this CLI will need a more complex implementation to work properly.

I think so too. But I wanted to have a first implementation to discuss a more complex one, messages and behavior.

There are more topics to discuss, as:

  • Check the ability to disable a plugin via Java, and not trust in Javascript
  • How to deal with optional dependencies
@Wadeck

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

How to deal with optional dependencies

Perhaps @Vlatombe could help IIRC you did some improvement on the Wizard on that area?

@Wadeck

Wadeck approved these changes Sep 25, 2018

@@ -167,4 +171,8 @@ private void assertJenkinsInQuietMode() {
private void assertJenkinsNotInQuietMode() {
QuietDownCommandTest.assertJenkinsNotInQuietMode(j);
}

private void assumeNotWindows() {
Assume.assumeFalse(System.getProperty("os.name").startsWith("Windows"));

This comment has been minimized.

Copy link
@Wadeck

Wadeck Sep 25, 2018

Contributor

Functions.isWindows perhaps?

@Vlatombe

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

My latest PR was related to classloading of optional dependencies (#3370). Not really related with Wizard.

@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

I'll improve the procedure to allow disabling all plugins, no matter the order

@jvz

jvz approved these changes Sep 25, 2018

Copy link
Member

left a comment

LGTM. I like the @WithPlugin bit to simplify things!

@varyvol
Copy link
Contributor

left a comment

I agree this needs a more complex logic so that the order of the plugins doesn't influence the result.

Show resolved Hide resolved core/src/main/java/hudson/cli/DisablePluginCommand.java Outdated
Show resolved Hide resolved core/src/main/java/hudson/cli/DisablePluginCommand.java Outdated
Show resolved Hide resolved core/src/main/java/hudson/cli/DisablePluginCommand.java
private DisablingStatus disablePlugin(PluginManager manager, String shortName) throws IOException {
PluginWrapper plugin = manager.getPlugin(shortName);
if (plugin == null) {
throw new IllegalArgumentException(Messages.DisablePluginCommand_NoSuchPlugin(shortName)); // exit with 3

This comment has been minimized.

Copy link
@varyvol

varyvol Sep 26, 2018

Contributor

Why not continue with other plugins in this case?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Sep 26, 2018

Author Contributor

it's an option, I like it, but it's how the enable-plugin works. I didn't want to do in the other way.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 27, 2018

Member

+1. Better to fail so that the error is propagated

This comment has been minimized.

Copy link
@fcojfernandez

fcojfernandez Sep 27, 2018

Contributor

My question here: Let's suppose we want to disable A, B, C. A is disabled correctly, B does not exist so it throws the error and C then still remains enabled.
I like the idea of throwing the error, but I think an "all or nothing" approach would it better. We might check the existence of all plugins as first step before disabling any of them.

}

plugin.disable();
stdout.format(Messages.DisablePluginCommand_Plugin_Disabled(plugin.getShortName()));

This comment has been minimized.

Copy link
@varyvol

varyvol Sep 26, 2018

Contributor

I'm not sure about printing the messages here. I think it makes more sense having the disable logic here and then printing the messages anywhere else.

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Sep 26, 2018

Author Contributor

Could you explain it please?

This comment has been minimized.

Copy link
@varyvol

varyvol Sep 26, 2018

Contributor

To me it makes more sense than this command would return the result of disabling the plugin, and the place where it is called react to that result and prints the corresponding message.

This comment has been minimized.

Copy link
@fcojfernandez

fcojfernandez Sep 27, 2018

Contributor

Maybe you're proposing like a final summary? It could be good idea.

public void disablePluginWithMandatoryDepender() {
assertThat(disablePlugins("-restart", "dependee"), failedWith(16));
assertPluginEnabled("dependee");
assertJenkinsNotInQuietMode();

This comment has been minimized.

Copy link
@varyvol

varyvol Sep 26, 2018

Contributor

What does this command check?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Sep 26, 2018

Author Contributor

Wether Jenkins is restarting

@oleg-nenashev
Copy link
Member

left a comment

Looks good assuming that @Wadeck addressed the magic return codes issue in a follow-up

private DisablingStatus disablePlugin(PluginManager manager, String shortName) throws IOException {
PluginWrapper plugin = manager.getPlugin(shortName);
if (plugin == null) {
throw new IllegalArgumentException(Messages.DisablePluginCommand_NoSuchPlugin(shortName)); // exit with 3

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 27, 2018

Member

+1. Better to fail so that the error is propagated

@Wadeck

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2018

@Wadeck addressed the magic return codes issue in a follow-up

#3663

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

Based on comment history this still needs the ordering fix discussed above?

@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2018

You are right @daniel-beck, I'll push the improved version on monday, with different strategies to disable dependents plugins. Stay tuned ;-)

[JENKINS-27177] Add strategies to disable plugins. Move process to core
Move the process to core to be reusable, not only for the cli command.
Add optionalDependants list to PluginWrapper to process them easily.

Replace (depreacate) the disable method without checks by this one with
strategies.

Improve messages and tests
@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

I've implemented several ways to process the dependant plugins letting the administrator to use the one that best fits his/her needs.
screenshot from 2018-10-12 20-32-52

  • none: if a plugin has enabled-mandatory dependant plugins, it cannot be disabled.
  • mandatory: all enabled-mandatory dependant plugins are disabled too.
  • all: all enabled dependant plugins (mandatory or optional) are disabled too.

The process is moved to the core itself, allowing it to be used along the code, not only by the CLI command.

The former disable method (without any check) is deprecated in favor of the new one. The new method returns a PluginDisableResult with the result of the operation over each plugin (the one who we wanted to disable and its depedants plugins, depending on the strategy used). It gives you:

  • plugins processed
  • status of each plugin (disabled, already disabled, no such plugin, not disabled due to dependants)
  • descriptive message of each plugin

Added the optionalDependants list to the PluginWrapper to be able to process it efficiently.

Improved the PluginManager.resolveDependantPlugins method to avoid unnecessary iterations and added the disablePlugins method.

In the command itself, I've renamed the option cascade to strategy, added quiet option, moved the procedure itself to the PluginManager and PluginWrapper classes to be able to reuse it and I keeped printing the output and calculating the result code in the command class.

Also improved messages and test class: add more tests and avoid having to know the output messages in the test class.

For the release notes / changelog, we should mention:

  • Add the disable-plugin CLI command.
  • Developer: Add disablePlugins to PluginManager.
  • Developer: Deprecate disablePlugin in PluginWrapper.
  • Developer: Add a fine-grained disablePlugin method.
@Wadeck
Copy link
Contributor

left a comment

Thank you for the new strategies!

I put some comments about the coding style that are not blocking (nit ones) but other are from my PoV.

The logic seems fine 👍


// already know possibleDependant depends on plugin, no need to continue with the rest of
// dependencies. We continue with the next possibleDependant
break;

This comment has been minimized.

Copy link
@Wadeck

Wadeck Oct 15, 2018

Contributor

Please avoid using break in general (except in switch) as a good practice, to avoid attracting C developers ;)

(same for continue)

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 15, 2018

Author Contributor

I'm a one-single-point exit lover, and never ever use break or continue, but in this case I realized that this way was more understandable and maintainable. Even knowing that, I did a deep search about the topic and no clear winner come to me. So I ended up using it, to avoid extra control flags in bucles and ifs. Let's see what the rest of the guys think about it to take an action. WYT?

Show resolved Hide resolved core/src/main/java/hudson/PluginManager.java Outdated
Show resolved Hide resolved core/src/main/java/hudson/PluginManager.java Outdated
Show resolved Hide resolved core/src/main/java/hudson/PluginWrapper.java
Show resolved Hide resolved core/src/main/java/hudson/PluginWrapper.java
Show resolved Hide resolved core/src/main/java/hudson/cli/DisablePluginCommand.java Outdated
Show resolved Hide resolved core/src/main/java/hudson/cli/DisablePluginCommand.java Outdated
Show resolved Hide resolved core/src/main/resources/hudson/cli/Messages.properties Outdated
Show resolved Hide resolved test/src/test/java/hudson/cli/DisablePluginCommandTest.java
Show resolved Hide resolved core/src/main/java/hudson/PluginWrapper.java

MRamonLeon added some commits Oct 15, 2018

@Wadeck

Wadeck approved these changes Oct 15, 2018

Copy link
Contributor

left a comment

🐝


if (plugin == null) {
PluginWrapper.PluginDisableResult result = new PluginWrapper.PluginDisableResult(pluginName);
result.setMessage(Messages.PluginWrapper_NoSuchPlugin(pluginName));

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

It seems weird to create an object and then set some attributes when you already knew those when creating the object. Maybe creating a new constructor or a helper method?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

I created the constructor in this way because in the process (PluginWrapper#disablePlugin) you create the object with the plugin being processed, and when you process all its dependants plugins and you know the result, you add the status and message. Anyway, I'll create a new constructor for the rest of the cases.

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 18, 2018

Contributor

But you haven't used the new constructor here?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 18, 2018

Author Contributor

it slipped my mind

} else {
// As there is no cycles in the plugin dependencies, the recursion shouldn't be infinite. The
// strategy used is the same for its dependants plugins
result.addDependantDisableStatus(dependantPlugin.disable(strategy));

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

What if this dependant plugin cannot be disabled? Shouldn't that set "aDependantNotDisabled?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

Right. Even more, we should add this case as a new status for this plugin.

Show resolved Hide resolved core/src/main/java/hudson/PluginWrapper.java Outdated
// getDependants returns all the dependant plugins, mandatory or optional.
dependantsToCheck = this.getDependants();
break;
case NONE:

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

Why not putting NONE and MANDATORY together?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

I left it so to be clearer, but I'll change

}

// disable...
List<PluginWrapper.PluginDisableResult> results = jenkins.pluginManager.disablePlugins(strategyToUse, pluginNames);

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

Should we check for null or empty lists?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

added a @nonnull to the disablePlugins method

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 16, 2018

Contributor

You are still not checking if "pluginNames" is null or empty.

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 17, 2018

Author Contributor

Added the Nonnull also

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 18, 2018

Contributor

I am not talking about the annotations, but about checking here if pluginNames == null and then throwing an exception.

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 18, 2018

Author Contributor

The parameter is required (required=true in the @Argument annotation of the class) so it cannot be null. If the user set "" as one of the plugins, the command will result in NO_SUCH_PLUGIN for this specific plugin

*/
private void restartIfNecessary(List<PluginWrapper.PluginDisableResult> results) throws RestartNotSupportedException {
if (restart) {
for (PluginWrapper.PluginDisableResult oneResult : results) {

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

Maybe a stream() with anymatch()?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

The control of the RestartNotSupportedException exception inside the restartIfNecessary makes it harder IMO, suggestions are welcomed, anyway.

}

if (oneResult.getDependantsDisableStatus().size() > 0) {
for (PluginWrapper.PluginDisableResult oneDependantResult : oneResult.getDependantsDisableStatus()) {

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

Ditto streams.

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

Ditto

private boolean restartIfNecessary(PluginWrapper.PluginDisableResult oneResult) throws RestartNotSupportedException {
PluginWrapper.PluginDisableStatus status = oneResult.getStatus();
if (PluginWrapper.PluginDisableStatus.DISABLED.equals(status)) {
Jenkins.get().safeRestart();

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

Does this wait until the CLI command finishes?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

It queues a restart after all builds are completed. It does it in another thread.

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 16, 2018

Contributor

I know that, but does it way until the CLI command finishes?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 17, 2018

Author Contributor

It does Jenkins#safeRestart so no, it doesn't

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 18, 2018

Contributor

I'm not sure if this ideal then, but the "enable-plugin" command works the same, so let's leave it like that.

assertPluginDisabled("depender");
assertPluginDisabled("plugin-first");
assertPluginDisabled("mandatory-depender");
assertJenkinsInQuietMode(); // some plugins were disabled

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

I don't understand this comment.

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

changed, i wanted to say that Jenkins is restarting because at least one plugin was disabled.

import static org.hamcrest.Matchers.notNullValue;
import static org.junit.Assert.*;

public class DisablePluginCommandTest {

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 15, 2018

Contributor

I think a test demonstrating that the order in which the plugins are specified does not matter is important here.

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 16, 2018

Author Contributor

There are some tests that demonstrate it, for example: quietModeEmptyOutputSucceed, quietModeWithErrorNoSuch

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 16, 2018

Contributor

How are they proving that?

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 18, 2018

Author Contributor

no worries, I've created a specific test to demo it: canDisableDependentPluginWrongOrderStrategyAll

[JENKINS-27177] Add new status error_disabling and minor fixes
When there is some problem disabling a plugin (the disabling file cannot
be created) it should be indicated and also reflected in the dependee
plugin.
[JENKINS-27177] Minor fixes.
Change the way of an array copy and ensure non null args
newArgs[i] = arg;
i++;
}
System.arraycopy(arguments, 0, newArgs, 1, arguments.length);

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 17, 2018

Author Contributor

WYT @varyvol ? 😉

This comment has been minimized.

Copy link
@varyvol

varyvol Oct 18, 2018

Contributor

Better, but I think you removed too much (the assigment to newArgs[0]).

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Oct 18, 2018

Author Contributor

You're right

@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

@daniel-beck As long as the tests don't fail you can remove the needs-fix and work-in-progress labels.

@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

@daniel-beck, you can remove the needs-fix and work-in-progress labels.

@batmat

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

I plan to merge this today if nobody objects or does it before. Didn't review it myself, but given there are many approvals and all issues have seemingly been addressed, we can move forward.

@batmat

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

Slightly adjusted the proposed changelog. Merging.

@batmat batmat merged commit 131794b into jenkinsci:master Nov 8, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.