-
Notifications
You must be signed in to change notification settings - Fork 200
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
Added plugin information while using verbose #611
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request! Some items for clarification and the spotbugs suppression needs to be fixed.
Can you add a test that confirms the new output is being generated?
@@ -269,11 +269,14 @@ private String getJenkinsWar() { | |||
* | |||
* @return list of plugins representing user-specified input | |||
*/ | |||
|
|||
@SuppressFBWarnings(value = {"PATH_TRAVERSAL_IN", "URLCONNECTION_SSRF_FD"}, justification = "User provided values for running the program.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to suppress a warning that does not exist in this context.
@SuppressFBWarnings(value = {"PATH_TRAVERSAL_IN", "URLCONNECTION_SSRF_FD"}, justification = "User provided values for running the program.") | |
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "User provided values for running the program.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @MarkEWaite
File[] directoryListing = dr.listFiles(); | ||
if (directoryListing != null) { | ||
for (File child : directoryListing) { | ||
pluginNames.add(child.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will add many unnecessary items to the list when run in a Jenkins plugins folder that is used by a running Jenkins controller. I think it would be better to only add the child if it is a plain file (not a directory) and does not have one of the suffixes (".bak", ".pinned", or ".version_from_image").
pluginNames.add(child.getName()); | |
pluginNames.add(child.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @MarkEWaite
@@ -283,7 +286,27 @@ private List<Plugin> getPlugins() { | |||
throw new PluginInputException("Unknown file type, file must have .yaml/.yml or .txt extension"); | |||
} | |||
} | |||
return requestedPlugins; | |||
List<String> pluginNames = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it is generating a list of plugin file names rather than plugin names. Would it be better to rename the variable pluginFileNames
for clarity?
List<String> pluginNames = new ArrayList<>(); | |
List<String> pluginNames = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @MarkEWaite
@@ -216,7 +216,6 @@ public void setupUpdateCenterCliTest() throws Exception { | |||
assertThat(cfg.getJenkinsIncrementalsRepoMirror()).hasToString(incrementalsCli); | |||
assertThat(cfg.getJenkinsPluginInfo()).hasToString(pluginInfoCli); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that you not remove the empty line between tests. I makes it more difficult for me to read. In this case, it seems that is the only change in this file in this pull request, so it should be removed from the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @MarkEWaite
Thanks @MarkEWaite I have one doubt, How can i compare the logs of --verbose for the test cases. I need to test whether the log containing particular statement or not that concludes the plugin is already present or not |
I would search the existing tests in the repository to see if any of them are already testing the output. If such a test already exists, then I'd copy its source code into a new test and adapt the new test to check the new behavior.
|
Thanks @MarkEWaite |
return requestedPlugins; | ||
List<String> pluginFileNames = new ArrayList<>(); | ||
|
||
File dr = new File(String.valueOf(pluginDir)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a very complicated technique to get a File (dr
) from an existing File (pluginDir
). I think it would be better to use pluginDir
directly rather than convert pluginDir
to a string then use that string to create a new File.
Note that this suggestion won't compile. You should rework the change to use pluginDir instead of dr
.
File dr = new File(String.valueOf(pluginDir)); |
private List<Plugin> getPlugins() { | ||
PluginListParser pluginParser = new PluginListParser(verbose); | ||
List<Plugin> requestedPlugins = new ArrayList<>(pluginParser.parsePluginsFromCliOption(plugins)); | ||
|
||
File pluginFile = getPluginFile(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the benefit of adding this empty line. It makes more work for reviewers. I think it should be removed.
@@ -283,7 +286,29 @@ private List<Plugin> getPlugins() { | |||
throw new PluginInputException("Unknown file type, file must have .yaml/.yml or .txt extension"); | |||
} | |||
} | |||
return requestedPlugins; | |||
List<String> pluginFileNames = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use formatting that is consistent with the rest of the file. The preceding line uses 8 leading spaces yet you chose to use 9 leading spaces. That makes the code review and future changes more difficult because people reading the code wonder why the formatting changed in the middle of the file.
List<String> pluginFileNames = new ArrayList<>(); | |
List<String> pluginFileNames = new ArrayList<>(); |
|
||
File dr = new File(String.valueOf(pluginDir)); | ||
|
||
File[] directoryListing = dr.listFiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to use dr
when we already have pluginDir
File[] directoryListing = dr.listFiles(); | |
File[] directoryListing = pluginDir.listFiles(); |
@@ -269,11 +269,14 @@ private String getJenkinsWar() { | |||
* | |||
* @return list of plugins representing user-specified input | |||
*/ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other suppress entry in this file does not insert a blank line before the suppression. I think it would be best to not insert this blank line. Let's keep the formatting consistent when we can.
for(Plugin plugin: requestedPlugins){ | ||
|
||
for(String names:pluginFileNames) { | ||
if (names.contains(plugin.getName())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this will give an incorrect verbose log entry when an existing plugin name is a substring of another plugin name. For example, if the plugin jquery3-api
is already installed and the plugin jquery
is requested, then I think that this will report that jquery
is already present in the directory, even though it is not already present.
I don't have an immediate suggestion of how to resolve that issue, but here is a set of steps that I use to duplicate the issue:
mvn clean -DskipTests verify
rm -rf ../plugins
mkdir ../plugins
echo jquery > ../plugins.txt
java -jar plugin-management-cli/target/jenkins-plugin-manager-*.jar --verbose --war ~/bugs/jenkins-2.431.war --plugin-download-directory ../plugins/ --plugin-file ../plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin > ../stdout 2> ../stderr
rm -rf ../plugins/jquery.jpi
java -jar plugin-management-cli/target/jenkins-plugin-manager-*.jar --verbose --war ~/bugs/jenkins-2.431.war --plugin-download-directory ../plugins/ --plugin-file ../plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin > ../stdout 2> ../stderr
grep jquery ../stderr
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't add empty lines in unrelated files. It makes the review more difficult and does not help future reviewer.
@@ -41,6 +41,7 @@ public class CliOptionsTest { | |||
@Rule | |||
public TemporaryFolder temporaryFolder = new TemporaryFolder(); | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the benefit of this empty line.
String stdOut = tapSystemOutNormalized(() -> { | ||
Config cfg = options.setup(); | ||
}); | ||
|
||
assertThat(stdOut).isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to assert the contents of stderr (where the verbose log is written) rather than the contents of stdout.
See the earlier examples in the file for the technique to check the contents of stderr.
Fixes #610
Submitter checklist