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

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

Open
minesskylineGTR opened this issue Jun 29, 2020 · 7 comments

Comments

@minesskylineGTR
Copy link

minesskylineGTR commented Jun 29, 2020

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.

@timja
Copy link
Member

timja commented Jun 30, 2020

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

@ricksbrown
Copy link
Contributor

ricksbrown commented Jun 30, 2020

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.

@timja
Copy link
Member

timja commented Jun 30, 2020

That sounds quite good ^^

@minesskylineGTR
Copy link
Author

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.

@ricksbrown
Copy link
Contributor

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

@raspy
Copy link
Member

raspy commented Sep 29, 2020

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.

@timja
Copy link
Member

timja commented Sep 29, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants