Move JSch SSHAuthenticatorFactory from ssh-credentials-plugin to this plugin#1
Move JSch SSHAuthenticatorFactory from ssh-credentials-plugin to this plugin#1bpedman wants to merge 2 commits intojenkinsci:masterfrom
Conversation
Move the implementation of the JSch SSHAuthenticatorFactory from the ssh-credential plugin to this plugin. This enables other plugins to depend on this plugin which has JSch on its classpath. Using the ssh- credential plugin's SSHAuthenticator class caused problems in some cases because the JSch library was not available on the classpath. This provides a cleaner implementation of the SSHAuthenticatorFactory extension point. Other libraries may do the same thing by providing their own plugin which registers SSHAuthenticatorFactory implementations. This also bumps the version of JSch used, mainly because there are newer things in this version that some users may need access to. This also sets the version scheme to <jsch-version>.<package-revision> so that the primary version can be based on the JSch version used but allow us to rev the version in case there are other implementation or package changes.
|
Corresponding ssh-credentials-plugin pull request: jenkinsci/ssh-credentials-plugin#19 |
| <deps.apache-sshd.version>0.12.0</deps.apache-sshd.version> | ||
| <deps.jsch.version>0.1.52</deps.jsch.version> | ||
| <deps.ssh-credentials-plugin.version>1.13-SNAPSHOT</deps.ssh-credentials-plugin.version> | ||
| <deps.ssh-credentials-plugin.version>2.0-SNAPSHOT</deps.ssh-credentials-plugin.version> |
There was a problem hiding this comment.
If you are only using the version in one place in the pom then it is a total anti-pattern to use properties to specify the version. The only time it makes sense to use properties to specify the version is when there are multiple dependencies or dependencies and plugins that need to be aligned on the same version. In those cases using a property is worth the risk. In all other cases using a property exposes the build and downstream builds to additional risks of instability.
Probably not something to address in this PR but rather in a separate PR.
There was a problem hiding this comment.
I am just curious why you say this is an anti-pattern. Can you enlighten me?
I have followed this practice for quite a while in quite a few projects and have not had any ill side affects and it makes it easy for me to find all the dependency versions used in a project.
There was a problem hiding this comment.
Because property substitution is quazai undefined. Does a system property affect the transitive dependency resolution of a dependent pom? What happens if I locally define a property in my Pom with the same name and a different value?
The uncertainty is worth the risk where two or more versions need to be synchronised but if you only have the version in one and only one place it is making the Pom more verbose than it needs to be... Which I an anti-pattern
There was a problem hiding this comment.
Actually looking at the whole PR, you are actually introducing the anti-pattern in this PR, so my personal view is 👎 on introducing the anti-pattern... note that this is just an advisory view from a Maven PMC member and committer and is not binding on the plugin maintainer
|
@bpedman thank you very much for your work - I've cherry-picked your main commit as a 113590d and it's been released with the plugin 0.1.54.1 version - that's why I'm closing this PR. The plan for next steps:
|
|
That sounds great. Thank you for your work. I am looking forward to being able to get off my custom plugin builds on our server. |
I ran into the same issues that @zregvart had in his original email posting here: http://jenkins-ci.361315.n4.nabble.com/How-to-handle-optional-non-plugin-dependencies-td4722603.html and found the pull request that was opened here jenkinsci/ssh-credentials-plugin#7 but was never merged. I went ahead and followed the suggestions by @stephenc to pull the SSHAuthenticatorFactory extensions into this plugin to make the dependency tree cleaner. I have tested this out and it solves my issues and seems to be the right way to go.
There will be a corresponding PR for the ssh-credentials-plugin as well.