plugin-installation-manager-tool icon indicating copy to clipboard operation
plugin-installation-manager-tool copied to clipboard

-d flag removes contents of already existing directory, though does not say in README

Open minesskylineGTR opened this issue 4 years ago • 7 comments

Using the -d or --plugin-download-directory will remove the directory/contents specified with the flag, even if there are contents there. This is not reflected in the README:

--plugin-download-directory or -d: (optional) Path to the directory in which to install plugins, which can also be set via
 the PLUGIN_DIR environment variable. Directory will be created if it does not exist. If no directory is entered, directory
 will default to C:\ProgramData\Jenkins\Reference\Plugins if detected OS is Windows, or /usr/share/jenkins/ref/plugins 
otherwise.

I don't believe this should be the default behavior. There should be at least a warning letting the user know that it whatever is there will be removed or that location already exists. I just found this out the hard way as I pointed -d to my current plugins folder which caused the removal of all of my plugins, and now Jenkins wants to uninstall them on next reboot since they're missing.

Please feel free to correct me if I have something wrong.

minesskylineGTR avatar Jun 29 '20 21:06 minesskylineGTR

This was a quick fix to an issue we had with failed plugin downloads never being cleared up.

The proper fix would either be:

  • Download plugins to a tmp directory and move when successful
  • delete failed downloads only

timja avatar Jun 30 '20 06:06 timja

IMHO the directory should be deleted so that it contains only the plugins listed in the YAML file (and the resulting dependency tree).

Otherwise plugin-installation-manager-tool cannot be used in IasC to uninstall plugins when they are removed from config, it can only ever add new plugins, leaving unlisted ones installed forever.

Also I would DL them all to tmp, and only move them when the entire list is successful - i.e. the entire operation is atomically successful or atomically fails.

ricksbrown avatar Jun 30 '20 20:06 ricksbrown

That sounds quite good ^^

timja avatar Jun 30 '20 20:06 timja

IMHO the directory should be deleted so that it contains only the plugins listed in the YAML file (and the resulting dependency tree).

Otherwise plugin-installation-manager-tool cannot be used in IasC to uninstall plugins when they are removed from config, it can only ever add new plugins, leaving unlisted ones installed forever.

Also I would DL them all to tmp, and only move them when the entire list is successful - i.e. the entire operation is atomically successful or atomically fails.

My concern, as I mentioned in my original post, was that this behavior caused the deletion of the plugins that were already installed and working, which is not ideal. For someone who was not prepared (and because the README does not explain this behavior) -d should only work against empty directories. From there, like you mentioned, it can be removed once the plugins have finished being installed/copied over.

minesskylineGTR avatar Jun 30 '20 23:06 minesskylineGTR

@minesskylineGTR yeah sorry I jumped straight into it and forgot to acknowledge you are totally right about the readme 👍

Also it's lucky you didn't point it at your home dir 😨

Perhaps it should do a check before removing the existing directory to ensure it only contains *.jpi, *.hpi and perhaps unpacked plugin directories.

ricksbrown avatar Jul 01 '20 03:07 ricksbrown

I believe that delete should only take place when it is specifically requested (with a separate switch perhaps). My use case is that we provide a base Docker image with some plugins installed and users can define their own additional plugins. Second run of the tool wipes out all preinstalled plugins and this should not take place.

Moreover, since the tool always removes directory, it always redownloads everything, even those that were already downloaded and have not changed. This is unnecessary waste of bandwidth and time.

See also #173.

raspy avatar Sep 29 '20 14:09 raspy

You can probably work around it for now by changing the directory that users plugins go to and then moving them after download.

Not downloading plugins that already exist and are of the same version should be another feature request as it’s more complicated

timja avatar Sep 29 '20 18:09 timja