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.

Sign up for GitHub

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

Jump to bottom

Stop cleaning the plugin directory by default + Add an opt-in flag for cleaning the target plugin directory + Restore the --list command #239

Merged
merged 11 commits into from Dec 9, 2020
Merged

Stop cleaning the plugin directory by default + Add an opt-in flag for cleaning the target plugin directory + Restore the --list command #239

merged 11 commits into from Dec 9, 2020

Conversation

oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Dec 8, 2020 •
edited

After #94 , the plugin directory was always emptied before downloading the plugins. This patch makes the plugin dir cleaning an optional behavior.

Sample output on a second run:

structs has no dependencies
Version of script-security (1.39) required by workflow-support (3.4) is lower than the version required (1.46) by pipeline-utility-steps (2.6.1), upgrading required plugin version
Installed version (1.18) of structs is less than minimum required version of 1.20, bundled plugin will be upgraded
Installed version (1.39) of script-security is less than minimum required version of 1.46, bundled plugin will be upgraded
Installed version (3.3) of workflow-support is less than minimum required version of 3.4, bundled plugin will be upgraded
Installed version (2.20) of workflow-step-api is less than minimum required version of 2.22, bundled plugin will be upgraded
Installed version (2.36) of workflow-api is less than minimum required version of 2.40, bundled plugin will be upgraded
Installed version (2.39) of workflow-job is less than minimum required version of 2.40, bundled plugin will be upgraded
Done

@oleg-nenashev oleg-nenashev added the enhancement New feature or request label Dec 8, 2020
timja
timja reviewed Dec 8, 2020
View reviewed changes
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

I think this needs to be done to a temp directory, or at least change the code to remove broken plugin downloads, they are kept for debugging currently i think

scenario:

  1. broken plugin download, checksum mismatch
  2. re-run download, code will find plugin already exists and skip the download
  3. dodgy plugin is used

timja
timja approved these changes Dec 8, 2020
View reviewed changes
plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java Show resolved Hide resolved
plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java Outdated Show resolved Hide resolved
...-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java Outdated
@@ -191,4 +195,42 @@ public void allowsLatestTopLevelDependency() throws IOException {
pluginManager.findPluginsAndDependencies(requestedPlugins);
assertThatNoException();
}

//TODO: Enable as auto-test once it can run without massive traffic overhead
Copy link
Member

Choose a reason for hiding this comment

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

is this a problem because you used workflow-job? could you use a small simple plugin instead and it would be ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

Workflow-job also emulates dependency updates, so it is useful for deeper tests. Will create a simple fork

@oleg-nenashev oleg-nenashev added the bug Something isn't working label Dec 9, 2020
jetersen
jetersen approved these changes Dec 9, 2020
View reviewed changes
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

LGTM, some super minor nits

plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java Outdated Show resolved Hide resolved
...-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java Show resolved Hide resolved
@oleg-nenashev @jetersen
Apply suggestions from code review
cbcdbba 
Co-authored-by: Joseph Petersen <josephp90@gmail.com>
@oleg-nenashev
Copy link
Member Author

@timja @jetersen Is it fine to ship it? I also removed the --list flag blocker. Although this command is not particularly helpful, maybe it worth keeping it until the stuff is reworked in #237

@jetersen
Copy link
Member

jetersen commented Dec 9, 2020

:shipit:

@oleg-nenashev oleg-nenashev merged commit 06ca514 into jenkinsci:master Dec 9, 2020
@oleg-nenashev oleg-nenashev deleted the plugin-downloads-wipe branch December 9, 2020 11:42
@oleg-nenashev oleg-nenashev changed the title Stop cleaning the plugin directory by default, add an opt-in flag for doing so Stop cleaning the plugin directory by default + Add an opt-in flag for cleaning the target plugin directory + Restore the --list command Dec 9, 2020
@oleg-nenashev oleg-nenashev mentioned this pull request Jan 25, 2021
Closed
hashar added a commit to hashar/plugin-installation-manager-tool that referenced this pull request Feb 9, 2023
@hashar
Add --clean-download-directory to README.md
cf851cf 
The option has been added via jenkinsci#239 in 2020. Add it to the `README.md`
for those reading the documentation from the GitHub repo page.
@hashar hashar mentioned this pull request Feb 9, 2023
Merged
6 tasks
MarkEWaite pushed a commit that referenced this pull request Feb 9, 2023
@hashar
Add --clean-download-directory to README.md (#528)
610d5c6 
The option has been added via #239 in 2020. Add it to the `README.md`
for those reading the documentation from the GitHub repo page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelneale michaelneale michaelneale approved these changes

@jetersen jetersen jetersen approved these changes

@timja timja timja approved these changes

Assignees
No one assigned
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@oleg-nenashev @jetersen @michaelneale @timja