Skip to content

Move built-in JSch implmentations to jsch-plugin#19

Closed
bpedman wants to merge 2 commits intojenkinsci:masterfrom
bpedman:master
Closed

Move built-in JSch implmentations to jsch-plugin#19
bpedman wants to merge 2 commits intojenkinsci:masterfrom
bpedman:master

Conversation

@bpedman
Copy link
Contributor

@bpedman bpedman commented Jun 8, 2016

See jenkinsci/jsch-plugin#1 for justification on this change. Basically this moves the implementation specific SSHAuthenticatorFactory extensions to another plugin.

I am not sure if this requires a major version bump or some notice of backwards incompatibility. However, seeing as both I and @zregvart had the same issues, I wonder if using the SSHAuthenticator from another plugin every actually worked at all.

Impementation of the SSHAuthenticatorFactorys for JSch have moved to the
jsch-plugin for a cleaner dependency structure.
@stephenc
Copy link
Member

stephenc commented Jun 9, 2016

👍 on the principle

</pluginRepositories>

<dependencies>
<!-- regular dependencies -->
Copy link
Member

Choose a reason for hiding this comment

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

we need to bump the major version number for this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@stephenc
Copy link
Member

stephenc commented Jun 9, 2016

👎 on merging as is... to have this mergable, include the major version bump in the pom.xml

Required since the JSch implementation has moved and may break existing
consumers.
@stephenc
Copy link
Member

stephenc commented Jun 17, 2016

OK, I'm not merging yet as we cannot cut a release without coordinating with releases of https://github.com/jenkinsci/jsch-plugin and https://github.com/jenkinsci/ssh-plugin which will need to have their dependency changes coordinated (ssh-plugin is the only user of the api that we are removing, so it will need a dependency change and bump so that it depends on the jsch-plugin and the newer ssh-credentials plugin).

Let's get those ducks in a row and have the maintainers on board for the coordinated releases

(Such a pity there is no staging support in the Jenkins Maven Repo... that would make coordinated releases easier)

/CC @zregvart @edmund-wagner @johnny-b-goode @ljader

@stephenc
Copy link
Member

👍 on these changes though... just we cannot merge and release yet

@jglick jglick requested a review from stephenc September 13, 2017 17:51
@jglick
Copy link
Member

jglick commented Sep 13, 2017

Please let us get this released. I do not think it is even incompatible: if you just install ssh-credentials,

def f = ExtensionList.lookup(com.cloudbees.jenkins.plugins.sshcredentials.SSHAuthenticatorFactory)[0];
println(f);
f.getClass().getClassLoader().loadClass('com.jcraft.jsch.Session')

com.cloudbees.jenkins.plugins.sshcredentials.impl.JSchSSHPasswordAuthenticator$Factory@3ad05c2a
java.lang.ClassNotFoundException: com.jcraft.jsch.Session
	at jenkins.util.AntClassLoader.findClassInComponents(AntClassLoader.java:1374)
	at jenkins.util.AntClassLoader.findClass(AntClassLoader.java:1327)
	at jenkins.util.AntClassLoader.loadClass(AntClassLoader.java:1080)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at java_lang_ClassLoader$loadClass.call(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:125)
	at Script1.run(Script1.groovy:3)
	at …

In other words, the implementation in this plugin could never have worked to begin with.

@ljader
Copy link
Member

ljader commented Sep 13, 2017

Yes, this should be moved forward - to delete JSch authenticators (eg. JSchSSHPasswordAuthenticator) from ssh-credentials-plugin and cut a major release of ssh-credentials-plugin.

@oleg-nenashev
Copy link
Member

My suggestion would be to just deprecate the current implementation (and inherit classes?) and then to spin the release without a major version bump. It would be safer for plugin dependencies

@oleg-nenashev
Copy link
Member

My suggestion would be to just deprecate the current implementation (and inherit classes?) and then to spin the release without a major version bump. It would be safer for plugin dependencies

Nope, it would cause a cyclic dependency

Copy link
Member

@stephenc stephenc left a comment

Choose a reason for hiding this comment

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

Needs a rebase

@olamy
Copy link
Member

olamy commented Sep 1, 2022

done with #152

@olamy olamy closed this Sep 1, 2022
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.

6 participants