Skip to content

Redeclare JSch dependency to the jsch-plugin#7

Closed
zregvart wants to merge 1 commit intojenkinsci:masterfrom
zregvart:master
Closed

Redeclare JSch dependency to the jsch-plugin#7
zregvart wants to merge 1 commit intojenkinsci:masterfrom
zregvart:master

Conversation

@zregvart
Copy link
Member

@zregvart zregvart commented Oct 5, 2014

So I've created the ssh-plugin and forked it under the jenkinsci/jsch-plugin, this changes the JSch dependency to that plugin. I need this for the ssh-credentials support in ssh-plugin.

Copy link
Member

Choose a reason for hiding this comment

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

Why optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi Jessie, thanks for the comment. As I understand, the ssh-credential plugin can be used with both Jsch and Trilead SSH implementations, and it's up to the plugin to choose which one to use and redeclare it as non-optional.

Copy link
Member

Choose a reason for hiding this comment

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

This plugin does not do anything with Trilead (that I can see). It does offer authenticators for use with JSch which other plugins may call as an API. Since the proposed jsch-plugin offers no UI or any other extensions, there is no reason (that I know of) not to have it installed, so there is no reason to complicate things by making this dependency optional. But @stephenc may have some motivation I am not aware of.

Copy link
Member

Choose a reason for hiding this comment

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

@jglick take your fight against optional elsewhere. I'm fine with this being optional ;-)

@stephenc
Copy link
Member

stephenc commented Oct 9, 2014

Have you cut a release of jsch plugin... also I would advise adding a .0 to the version number of jsch that you package in case you identify an issue with your plugin and need a new release that includes the same version of jsch... e.g. see mapdb plugin where the version number is composed of the mapdb version number with another segment added.

@zregvart
Copy link
Member Author

zregvart commented Oct 9, 2014

@stephenc thanks for chiming in, I pushed the release somewhat after creating the pull request (that's why the build failed) and now I'm too lazy to push another. I guess I can always add a fourth version number to the jsch-plugin if need be (or make it non semver like 0.1.42-1).

@jglick the ssh-credential plugin indeed uses trilead, so I guess it's ok to keep jsch optional -- on par with trilead. Might want to consider creating a dependency plugin for trilead, in the manner of jsch-plugin, to prevent class[loading|casting] issues in other plugins.

@jglick
Copy link
Member

jglick commented Oct 9, 2014

The Trilead dependency is getting sucked in via jenkins-core, which is bad. Most of this dep is being used for the crypto package, but there is also some unwanted usage: the deprecated SFTPClient could be moved into any newly split plugin, but there is also hudson.cli.PrivateKeyProvider, and CLI has not yet been split into a plugin. So we cannot have Trilead made into a plugin until that is fixed.

@stephenc
Copy link
Member

stephenc commented Oct 9, 2014

In an ideal world you might invert the dependency and move the jack
specific stuff to the jsch plugin and have that depend (non optionally) on
credentials and ssh-credentials

If we do that for trilead's then they would be nicely symmetry

On Thursday, 9 October 2014, Jesse Glick notifications@github.com wrote:

The Trilead dependency is getting sucked in via jenkins-core, which is
bad. Most of this dep is being used for the crypto package, but there is
also some unwanted usage: the deprecated SFTPClient could be moved into
any newly split plugin, but there is also hudson.cli.PrivateKeyProvider,
and CLI has not yet been split into a plugin. So we cannot have Trilead
made into a plugin until that is fixed.


Reply to this email directly or view it on GitHub
#7 (comment)
.

Sent from my phone

@stephenc
Copy link
Member

stephenc commented Jun 9, 2016

superceded by #19

@stephenc stephenc closed this Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants