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

[JENKINS-32120] Register Bouncy Castle on the remote agent by using Bouncy Castle API plugin #14

Merged
merged 7 commits into from Jun 20, 2016

Conversation

Projects
None yet
5 participants
@alvarolobato
Copy link
Member

commented Jun 15, 2016

@reviewbybees

This comment has been minimized.

Copy link

commented Jun 15, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

if (channel instanceof Channel) {
try {
InstallBouncyCastleJCAProvider.on((Channel) channel);
logger.println("[ssh-agent] Registered BouncyCastle on the remote agent");

This comment has been minimized.

Copy link
@recena

recena Jun 15, 2016

Contributor

Great log!

@recena

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2016

🐝

InstallBouncyCastleJCAProvider.on((Channel) channel);
logger.println("[ssh-agent] Registered BouncyCastle on the remote agent");
} catch (Exception e) {
logger.println("[ssh-agent] WARNING: could not register BouncyCastle on the remote agent." + e.getMessage());

This comment has been minimized.

Copy link
@stephenc

stephenc Jun 15, 2016

Member

if you passed a TaskListener this could be e.printStackTrace(listener.error("[ssh-agent] WARNING: could not register BouncyCastle on the remote agent."));

@stephenc

This comment has been minimized.

Copy link
Member

commented Jun 15, 2016

🐜 the helper method could have a more conventional signature using TaskListener which would allow for more flexible usage

@alvarolobato

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2016

@stephenc changed.

listener.getLogger().println("[ssh-agent] Registered BouncyCastle on the remote agent");
} catch (Exception e) {
e.printStackTrace(listener.error("[ssh-agent] WARNING: could not register BouncyCastle on the remote agent."));
}

This comment has been minimized.

Copy link
@stephenc

stephenc Jun 15, 2016

Member

🐜 no need for the WARNING as the logged line will get an ERROR: prefix anyway

This comment has been minimized.

Copy link
@recena

recena Jun 15, 2016

Contributor

I prefer not to use e.printStackTrace when you can handle the behavior, print a user-friendly message and avoid an ugly trace in the UI.

This comment has been minimized.

Copy link
@stephenc

stephenc Jun 15, 2016

Member

It may not be the prettiest but it will let you know what failed.

We do not expect it to fail, so it is not reasonable to expect us to anticipate all the potential failure modes and parse them (and test) so we can generate meaningful messages.

This comment has been minimized.

Copy link
@recena

recena Jun 15, 2016

Contributor

As user I don't want to see a stack trace, simply something for understanding that it was wrong. As administrator or developer, I want these details but for that, exist the log files.

Different point of views, only that.

This comment has been minimized.

Copy link
@alvarolobato

alvarolobato via email Jun 16, 2016

Author Member
@stephenc

This comment has been minimized.

Copy link
Member

commented Jun 15, 2016

🐝 (with one minor 🐜)

@alvarolobato

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2016

Removed "Warning"

@alvarolobato

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2016

/**
* Helper class for common remote tasks
*/
public class RemoteHelper {

This comment has been minimized.

Copy link
@amuniz

amuniz Jun 16, 2016

Member

Maybe this class could be package visible only. This is currently public API usable from other plugins (and I fear others will use it).

No public API is not needed, in general.

This comment has been minimized.

Copy link
@alvarolobato

alvarolobato Jun 16, 2016

Author Member

Not possible, being used from other two packages. I just added a @Restricted

@amuniz

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

🐝

I would make the RemoteHelper class not visible outside though.

@amuniz

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

re-🐝

@alvarolobato

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2016

@reviewbybees

This comment has been minimized.

Copy link

commented Jun 17, 2016

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@recena recena merged commit 193634c into jenkinsci:master Jun 20, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.