[chore] Move Trilead Authenticator implementation from SSH Credentials #147
Conversation
|
Last commit is a test with incrementals from jenkinsci/ssh-credentials-plugin#206 |
|
I am not sure of the motivation for this change; I did not check how many plugins use those classes, maybe only the SSH Build Agents. But I am not sure if it is the right place for the classes too. It is another reason to make the change to the new library or stop maintaining the SSH plugins on my side and put them in adoption and start publishing the other version I have with the new library. |
There was a problem hiding this comment.
Unfortunately for a situation like this where deps are reversed, there is not any good way of releasing the changes incrementally. Nor can you release this as written first and then fix up the dep version to be non-incremental later, since -P-consume-incrementals is enforced during the release process (for good reason). All you can do is release jenkinsci/ssh-credentials-plugin#206 and then edit this PR to pick up the released version and immediately release this too (so you had better make sure maintainers of both plugins are ready to pull the trigger). And users needed to be warned in release notes, compatibleSinceVersion, etc. that they cannot update ssh-credentials in isolation—they should update both (from the GUI it suffices to request update of trilead-api).
bom changes can and should be prepared in advance: just file a draft PR picking up both PRs as incremental versions and add full-test or weekly-test. When both plugins have been released, edit the versions and mark ready for review.
Dohbedoh
force-pushed
the
chore/sshconnectors
branch
from
33b9c04
to
6307c1c
Compare
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>ssh-credentials</artifactId> | ||
| <optional>true</optional> | ||
| </dependency> |
There was a problem hiding this comment.
should have an optional dep on credentials for the Username/Password based authenticator.
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| @OptionalExtension(requirePlugins = {"ssh-credentials"}) |
There was a problem hiding this comment.
this uses StandardUsernamePasswordCredentials not SSH ones, so the following?
| @OptionalExtension(requirePlugins = {"ssh-credentials"}) | |
| @OptionalExtension(requirePlugins = {"plain-credentials"}) |
There was a problem hiding this comment.
StandardUsernamePasswordCredentials is defined in credentials. plain-credentials is for string & text credentials which are newer.
There was a problem hiding this comment.
| @OptionalExtension(requirePlugins = {"ssh-credentials"}) | |
| @OptionalExtension(requirePlugins = {"credentials"}) |
There was a problem hiding this comment.
@jtnord ssh-credentials is required since this is a com.cloudbees.jenkins.plugins.sshcredentials.SSHAuthenticator implementation. ssh-credentials should be sufficient since it has a direct required dependency on credentials ? Or do we want to include both ssh-credentials and credentials ?
| assertThat(instance.isAuthenticated(), is(true)); | ||
| } | ||
|
|
||
| private Object createPasswordAuthenticatedSshServer() throws InvocationTargetException, NoSuchMethodException, ClassNotFoundException, InstantiationException, IllegalAccessException { |
|
@kuisathaverat do you think is it something we could move forward? it certainly needs some more testing and coordination when releasing it but will cleanup dependencies. |
Dohbedoh
force-pushed
the
chore/sshconnectors
branch
from
6307c1c
to
6e48666
Compare
(See also jenkinsci/ssh-credentials-plugin#206)
Moving Trilead specific implementation from SSH Credentials as proposed in jenkinsci/ssh-credentials-plugin#199.
This probably need some planning and coordination between those 2 plugins and the bom. Also I am not sure how to avoid a circular dependency
trilead-api->ssh-credentials->trilead-api. Could not prevent it with deps exclusions..cc @olamy
Submitter checklist