Add option to set Basic credentials #292
Conversation
This closes jenkinsci#245
|
can you resolve the conflicts, should be easy, edit: thanks |
| @@ -80,6 +80,9 @@ | |||
| <directory>src/main/resources-filtered</directory> | |||
| <filtering>true</filtering> | |||
| </resource> | |||
| <resource> | |||
| <directory>src/main/resources</directory> | |||
There was a problem hiding this comment.
Messages.properties doesn't need to be filtered. Can move it to src/main/resources-filtered but this has the overhead of unnecessary filtering...
plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/config/Credentials.java
Outdated
Show resolved
Hide resolved
| FileUtils.copyURLToFile(location.toURL(), pluginFile); | ||
| } catch (URISyntaxException | IOException e) { | ||
| CredentialsProvider credentialsProvider = getCredentialsProvider(); | ||
| if (credentialsProvider != null) { |
There was a problem hiding this comment.
Fixed in 1be02a1. Now it can really be null in case no credentials have been set.
| } | ||
| HttpGet httpGet = new HttpGet(urlString); | ||
| try { | ||
| httpclient.execute(httpGet, new FileDownloadResponseHandler(pluginFile), context); |
There was a problem hiding this comment.
this appears to revert #285 ?
(logging the actual download url after redirects)
There was a problem hiding this comment.
Yes, indeed, but the redirect are handled now within HttpClient, so if you are interested in this just enable the logging from HttpClient....
Update: Since this is easy to do, I am gonna restore the logging
…nmanager/config/Credentials.java Co-authored-by: Tim Jacomb <[email protected]>
| // get final URI (after all redirects) | ||
| List<URI> locations = context.getRedirectLocations(); | ||
| if (locations != null) { | ||
| logVerbose(String.format("Downloading %s from %s", plugin.getName(), locations.get(locations.size() - 1))); |
There was a problem hiding this comment.
isn't this done after the request has been made?
| logVerbose(String.format("Downloading %s from %s", plugin.getName(), locations.get(locations.size() - 1))); | |
| logVerbose(String.format("Downloaded %s from %s", plugin.getName(), locations.get(locations.size() - 1))); |
and shouldn't it be added to the catch clause in that case?
There was a problem hiding this comment.
Good catch, now logged in finally (so that it covers both success and failure): 968d7c0
| // get final URI (after all redirects) | ||
| List<URI> locations = context.getRedirectLocations(); | ||
| if (locations != null) { | ||
| logVerbose(String.format("%s %s from %s", success ? "Downloaded" : "Tried downloading", plugin.getName(), locations.get(locations.size() - 1))); |
There was a problem hiding this comment.
nice, if you're feeling extra amazing it would be great to log the failure case always and verbose for the regular case, but not required to ship this
Thanks!
plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks like a default value is missing,
➜ plugin-installation-manager-tool git:(feature/basic-auth) ✗ java -jar plugin-management-cli/target/jenkins-plugin-manager-2.6.2-SNAPSHOT.jar
Exception in thread "main" java.lang.NullPointerException
at org.kohsuke.args4j.spi.DelimitedOptionHandler.printDefaultValue(DelimitedOptionHandler.java:62)
at org.kohsuke.args4j.CmdLineParser.createDefaultValuePart(CmdLineParser.java:358)
at org.kohsuke.args4j.CmdLineParser.printOption(CmdLineParser.java:335)
at org.kohsuke.args4j.CmdLineParser.printUsage(CmdLineParser.java:303)
at org.kohsuke.args4j.CmdLineParser.printUsage(CmdLineParser.java:273)
at org.kohsuke.args4j.CmdLineParser.printUsage(CmdLineParser.java:263)
at io.jenkins.tools.pluginmanager.cli.Main.main(Main.java:33)
…nmanager/impl/PluginManager.java Co-authored-by: Tim Jacomb <[email protected]>
Indeed, seem this is kohsuke/args4j#166. Will check for a workaround. Update: Should be fixed by eac1477 |
…llation-manager-tool.git into feature/basic-auth
|
there was a nullpointer when run without credentials, as the CLI was setting credentials to null when not used. I added a safety net in the configuration |
|
Thanks for the contribution! It's been asked for a couple of times |
This closes #245