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 1 commit
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
25 changes: 24 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", "URLCONNECTION_SSRF_FD"}, justification = "User provided values for running the program.")
Copy link
Contributor

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.

Suggested change
@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.")

Copy link
Author

Choose a reason for hiding this comment

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

Done @MarkEWaite

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,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<>();
Copy link
Contributor

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?

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

Copy link
Author

Choose a reason for hiding this comment

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

Done @MarkEWaite


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) {
pluginNames.add(child.getName());
Copy link
Contributor

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").

Suggested change
pluginNames.add(child.getName());
pluginNames.add(child.getName());

Copy link
Author

Choose a reason for hiding this comment

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

Done @MarkEWaite

}
}

for(Plugin plugin: requestedPlugins){
for(String names:pluginNames) {
if ( names.indexOf( plugin.getName() )!= -1) {
logVerbose("Plugin " + plugin.getName() + " is already present in the dir");
}
}
}

return requestedPlugins;

}

/**
Expand Down
1 change: 0 additions & 1 deletion 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 @@ -216,7 +216,6 @@ public void setupUpdateCenterCliTest() throws Exception {
assertThat(cfg.getJenkinsIncrementalsRepoMirror()).hasToString(incrementalsCli);
assertThat(cfg.getJenkinsPluginInfo()).hasToString(pluginInfoCli);
}

Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done @MarkEWaite

@Test
public void setupSecurityWarningsTest() throws CmdLineException {
parser.parseArgument("--view-all-security-warnings", "--view-security-warnings");
Expand Down
Loading