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

Added plugin information while using verbose #611

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added plugin information while using verbose #611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6956132
Added plugin information while using verbose
AayushSaini101 Nov 12, 2023
cad3fd8
Improve naming convention and added test cases
AayushSaini101 Nov 14, 2023
8aa5303
Resolve checkstyle issue
AayushSaini101 Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/CliOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,14 @@ private String getJenkinsWar() {
*
* @return list of plugins representing user-specified input
*/

Copy link
Contributor

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.

Suggested change

@SuppressFBWarnings(value = {"PATH_TRAVERSAL_IN"}, justification = "User provided values for running the program.")
private List<Plugin> getPlugins() {
PluginListParser pluginParser = new PluginListParser(verbose);
List<Plugin> requestedPlugins = new ArrayList<>(pluginParser.parsePluginsFromCliOption(plugins));

File pluginFile = getPluginFile();

Copy link
Contributor

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.

Suggested change

if (pluginFile != null) {
if (isFileExtension(pluginFile, "yaml", "yml")) {
requestedPlugins.addAll(pluginParser.parsePluginYamlFile(pluginFile));
Expand All @@ -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<>();
Copy link
Contributor

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.

Suggested change
List<String> pluginFileNames = new ArrayList<>();
List<String> pluginFileNames = new ArrayList<>();


File dr = new File(String.valueOf(pluginDir));
Copy link
Contributor

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.

Suggested change
File dr = new File(String.valueOf(pluginDir));


File[] directoryListing = dr.listFiles();
Copy link
Contributor

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

Suggested change
File[] directoryListing = dr.listFiles();
File[] directoryListing = pluginDir.listFiles();

if (directoryListing != null) {
for (File child : directoryListing) {
if (child.isFile()) {
pluginFileNames.add(child.getAbsoluteFile().getName());
}
}
}
for(Plugin plugin: requestedPlugins){

for(String names:pluginFileNames) {
if (names.contains(plugin.getName())) {
Copy link
Contributor

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

logVerbose("Plugin " + plugin.getName() + " is already present in the dir");
}
}
}

return requestedPlugins;

}

/**
Expand Down
21 changes: 21 additions & 0 deletions plugin-management-cli/src/test/java/io/jenkins/tools/pluginmanager/cli/CliOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public class CliOptionsTest {
@Rule
public TemporaryFolder temporaryFolder = new TemporaryFolder();


Copy link
Contributor

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.

Suggested change

@Before
public void createParser() {
options = new CliOptions();
Expand Down Expand Up @@ -383,4 +384,24 @@ private void assertConfigHasPlugins(Config cfg, List<Plugin> expectedPlugins) {
Plugin[] expectedPluginsAsArray = expectedPlugins.toArray(new Plugin[0]);
assertThat(cfg.getPlugins()).containsExactlyInAnyOrder(expectedPluginsAsArray);
}

@Test
public void verboseEnabledContainingPluginInformation() throws Exception{

String jenkinsWar = this.getClass().getResource("/jenkinstest.war").toString();

parser.parseArgument("--war", jenkinsWar);

String pluginDir = temporaryFolder.newFolder("plugins").toString();

parser.parseArgument("--war",jenkinsWar,"--plugin-download-directory",pluginDir,"--plugins","delivery-pipeline-plugin:1.3.2 deployit-plugin","--verbose");

String stdOut = tapSystemOutNormalized(() -> {
Config cfg = options.setup();
});

assertThat(stdOut).isEmpty();
Comment on lines +399 to +403
Copy link
Contributor

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.


}

}
2 changes: 2 additions & 0 deletions ...anagement-library/src/main/java/io/jenkins/tools/pluginmanager/util/PluginListParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ public List<Plugin> parsePluginTxtFile(File pluginTxtFile) {
return pluginsFromTxt;
}



Comment on lines +74 to +75
Copy link
Contributor

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.

Suggested change

/**
* Given a Jenkins yaml file with a plugins root element, will parse the yaml file and create a list of requested
* plugins
Expand Down
Loading