Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make trilead-api as optional as it contains some algo users may no want to use #199

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

olamy
Copy link
Member

@olamy olamy commented Mar 15, 2024

Signed-off-by: Olivier Lamy olamy@apache.org

Testing done

Submitter checklist

…nt to use

Signed-off-by: Olivier Lamy <olamy@apache.org>
@olamy olamy marked this pull request as ready for review March 19, 2024 00:40
@olamy olamy requested a review from a team as a code owner March 19, 2024 00:40
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

And conversely I see that mina-sshd-api-core lists ssh-credentials as an optional dep. (It uses @Extension(optional = true) which would better be switched to @OptionalExtension as here, to avoid warnings on startup in the unlikely case ssh-credentials is not in fact enabled.)

@jglick jglick merged commit 7fcbaef into jenkinsci:master Mar 19, 2024
15 checks passed
MarkEWaite added a commit to MarkEWaite/bom that referenced this pull request Mar 23, 2024
…enkinsci#3039)"

jenkinsci/ssh-credentials-plugin#199 makes
the trilead-api optional.  Likely needs some tuning to allow plugin
compatibility tests to pass.

This reverts commit 3fb5748.
MarkEWaite added a commit to jenkinsci/bom that referenced this pull request Mar 23, 2024
…3039)" (#3049)

jenkinsci/ssh-credentials-plugin#199 makes
the trilead-api optional.  Likely needs some tuning to allow plugin
compatibility tests to pass.

This reverts commit 3fb5748.
MarkEWaite added a commit to MarkEWaite/bom that referenced this pull request Mar 25, 2024
jenkinsci/ssh-credentials-plugin#199 makes the
trilead-api plugin an optional dependency of the ssh-credentials plugin.
That exposed a latent issue in the subversion plugin that is resolved
in jenkinsci/subversion-plugin#284 .

jenkinsci#3050 describes the details of
the test failures in the plugin bill of mateerials.  The failing tests
can be seen with the commands:

 PLUGINS=subversion TEST=CompareAgainstBaselineCallableTest bash local-test.sh
 PLUGINS=mina-sshd-api-core LINE=weekly bash local-test.sh

No need to block later releases, since the pull request thet fixes the
issue has been submitted and is ready for review.
@Dohbedoh
Copy link
Contributor

Dohbedoh commented May 2, 2024

Shouldn't the trilead authenticator implementation eventually be moved to trilead-api plugin ? Like it is the case for Jsch, see #152, and mina-sshd.

@olamy olamy deleted the trilead-api-optional branch May 3, 2024 00:19
@olamy
Copy link
Member Author

olamy commented May 3, 2024

Shouldn't the trilead authenticator implementation eventually be moved to trilead-api plugin ? Like it is the case for Jsch, see #152, and mina-sshd.

@Dohbedoh very good idea. Do you have time to take care of this? If not, let me know and I will.

@Dohbedoh
Copy link
Contributor

Dohbedoh commented May 8, 2024

I can take a look. But will need some coordination / release plan between those 2 plugins to avoid circular dependencies.

@olamy
Copy link
Member Author

olamy commented May 8, 2024

I can take a look. But will need some coordination / release plan between those 2 plugins to avoid circular dependencies.

sounds good. We can coordinate at the pub :)

@basil
Copy link
Member

basil commented May 14, 2024

After this PR, in the context of https://github.com/jenkinsci/acceptance-test-harness

import com.cloudbees.jenkins.plugins.sshcredentials.SSHAuthenticatorFactory
ExtensionList.lookup(SSHAuthenticatorFactory.class)

returns the empty list instead of the two Trilead implementations, causing jenkinsci/acceptance-test-harness#1539 and jenkinsci/acceptance-test-harness#1540 (39 new ATH failures).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants