[JENKINS-28689] Make compatible with Workflow plugin by recena · Pull Request #4 · jenkinsci/ssh-agent-plugin

recena · 2015-07-16T15:27:43Z

Downstream of jenkinsci/pipeline-plugin#161

https://issues.jenkins-ci.org/browse/JENKINS-28689

Initial source code modifications
Implement a basic mocked SSH Server
Add a test to verify that SSH Agent is working how we expect
Add a test to verify that SSH Agent is working how we expect using a Workflow script
Implement SSHAgentStep, DescriptorImpl and SSHAgentExecution
Review the documentation
Add more tests
Snippet Generator must work

jenkinsadmin · 2015-07-16T18:02:47Z

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

…mock SSH Server

jglick · 2015-07-23T15:23:52Z

src/test/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentBase.java

Extending ExternalResource and using a @Rule would be better style.

(Though beware that I recall some problem with RestartableJenkinsRule not handling @Before/@After. Not sure if that also applies to extra @Rules.)

@jglick I checked, does not work fine. I decided to remove it.

…hanges proposed by Jesse

stephenc · 2015-08-05T10:53:56Z

🐛 the serialVersionUID fields are incorrect. I have supplied the correct values for the three that I found:

com.cloudbees.jenkins.plugins.sshagent.mina.MinaRemoteAgentStarter:    private static final long serialVersionUID = -3757105406876098311L;
com.cloudbees.jenkins.plugins.sshagent.mina.MinaRemoteAgentFactory.TomcatNativeInstalled:    private static final long serialVersionUID = 3234893369850673438L;
com.cloudbees.jenkins.plugins.sshagent.jna.JNRRemoteAgentStarter:    private static final long serialVersionUID = 5020446864184061252L;

…tephen's recommendations

recena · 2015-08-05T11:08:55Z

@stephenc The Serial Version UIDs has been reviewed. I have to investigate how the JVM works in theses cases.

stephenc · 2015-08-05T11:35:57Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/jna/JNRRemoteAgentStarter.java

🐜 normally I would include a comment to explain that this was generated from the 1.7 version of the class and also include a @since 1.8 javadoc comment, e.g.

/** * Ensure consistent serialization. Value generated from the 1.7 release. * @since 1.8 */

stephenc · 2015-08-05T11:37:51Z

I've only 🐜s so here is my 🐝 (obviously I'd prefer if you fix the 🐜s but I am no longer blocking on the changes proposed)

…UID was generated from 1.7

recena · 2015-08-05T11:51:38Z

@stephenc I don' like 🐜 in my PRs ;)

stephenc · 2015-08-05T11:52:09Z

🐝

jglick · 2015-08-05T13:47:07Z

🐝 again. (One of the two comments about help.html was ignored, but it could also be done elsewhere, such as in the plugin wiki: documenting the main limitation of the current implementation, that restarting in the middle of the block works only insofar as sh steps use their private key soon after starting.)

recena · 2015-08-05T13:56:25Z

@reviewbybees done

ghost · 2015-08-05T13:56:50Z

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.

stephenc · 2015-08-05T14:08:35Z

As principal author of this plugin, and as the plugin was developed on CloudBees, Inc. time, I view this plugin's maintainers as including any CloudBees engineer. There seems to be some permissions problems on this repo at the present time, so I am merging this on behalf of @recena until he gets his permissions fixed.

[JENKINS-28689] Make compatible with Workflow plugin

KostyaSha · 2015-08-05T14:12:25Z

👎 for 31 unsquashed commits

recena · 2015-08-05T14:15:45Z

@KostyaSha Thanks for your feedback

KostyaSha · 2015-08-05T14:17:20Z

@recena i'm not against content, i'm against 31 commits for one feature. This is unreal headache for history. And it not even a refactoring...

stephenc · 2015-08-05T14:17:20Z

@KostyaSha as a maintainer, I prefer unsquashed commits in plugin repos that I am a maintainer of

recena · 2015-08-05T14:20:31Z

@KostyaSha In this case I considered that the commits were important. They are telling the path until the final solution. Anyway, thanks so much for your advice.

jglick · 2015-08-05T14:49:33Z

Filed JENKINS-29810 for stronger restart behavior.

batmat · 2015-10-09T11:33:30Z

pom.xml

Isn't <optional>true</optional> missing here?

This is gonna make compulsory the installation of that part of the workflow plugin when using ssh-agent, right? For me, it does not sound totally right. Or I'm missing something?

I read the feature that requested that PR as: "if workflow is present, then contribute a step to it [but if absent, then just stay still]"

@batmat Let me take a look.

@batmat they are adding hard dep to workflow-step-api in all plugins. Even credentials-binding. I got reply that they (CB) don't care :(

No problem @recena see also the question on dev list that triggered this discussion just in case https://groups.google.com/forum/#!topic/jenkinsci-dev/K7pf_W8NgqA

[JENKINS-28689] Make compatible with Workflow plugin

846bc99

recena changed the title ~~[JENKINS-28689] Make compatible with Workflow plugin~~ [WiP] [JENKINS-28689] Make compatible with Workflow plugin Jul 16, 2015

Manuel Recena and others added 4 commits July 20, 2015 10:48

[JENKINS-28689] Make compatible with Workflow plugin

5f0f54b

[JENKINS-28689] Make compatible with Workflow plugin

f850c52

[JENKINS-28689] Added a new test (sshAgentAvailable)

646ae61

[JENKINS-28689] Adding a POC for a basic mock SSH Server

c08b1dd

recena force-pushed the JENKINS-28689 branch from c658338 to c08b1dd Compare July 21, 2015 23:41

Manuel Recena added 2 commits July 22, 2015 17:51

[JENKINS-28689] Adding a POC for a basic mock SSH Server

7d1b620

[JENKINS-28689] SSHAgentBuildWrapperTest has been adapted to use the …

9ea550e

…mock SSH Server

recena closed this Jul 23, 2015

recena reopened this Jul 23, 2015

Manuel Recena added 2 commits July 23, 2015 11:35

[JENKINS-28689] Merge upstream master

6dbe8ad

[JENKINS-28689] Tests has been reviewed. Clean up source code

fdb6350

jglick reviewed Jul 23, 2015
View reviewed changes

Manuel Recena and others added 3 commits July 23, 2015 18:19

[JENKINS-28689] Tests has been reviewed. Clean up source code

96e792f

[JENKINS-28689] Improved the way how Mocked SSH Server is launched. C…

2e3e1d3

…hanges proposed by Jesse

[JENKINS-28689] Compilation errors

c3b7490

[JENKINS-28689] Serial Version UIDs has been reviewed following the S…

0cefdd3

…tephen's recommendations

stephenc reviewed Aug 5, 2015
View reviewed changes

[JENKINS-28689] Included javadocs to explain that these SerialVersion…

050eebf

…UID was generated from 1.7

stephenc added a commit that referenced this pull request Aug 5, 2015

Merge pull request #4 from recena/JENKINS-28689

5a72bf8

[JENKINS-28689] Make compatible with Workflow plugin

stephenc merged commit 5a72bf8 into jenkinsci:master Aug 5, 2015

recena mentioned this pull request Aug 6, 2015

Updated workflow version #7

Merged

KostyaSha referenced this pull request in tfennelly/jenkins Aug 25, 2015

Fixing NITs from @KostyaSha

7adbeef

batmat reviewed Oct 9, 2015
View reviewed changes

recena mentioned this pull request Feb 29, 2016

There was one test without SSH Agent wrapper #10

Merged

jglick mentioned this pull request Apr 8, 2024

Fail the entire build when a requested credential cannot be found #144

Merged

jglick mentioned this pull request Jul 29, 2024

Simplified plugin impl, removed unreliable onResume #152

Merged

Uh oh!

Conversation

recena commented Jul 16, 2015

Uh oh!

jenkinsadmin commented Jul 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenc commented Aug 5, 2015

Uh oh!

recena commented Aug 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenc commented Aug 5, 2015

Uh oh!

recena commented Aug 5, 2015

Uh oh!

stephenc commented Aug 5, 2015

Uh oh!

jglick commented Aug 5, 2015

Uh oh!

recena commented Aug 5, 2015

Uh oh!

ghost commented Aug 5, 2015

Uh oh!

stephenc commented Aug 5, 2015

Uh oh!

KostyaSha commented Aug 5, 2015

Uh oh!

recena commented Aug 5, 2015

Uh oh!

KostyaSha commented Aug 5, 2015

Uh oh!

stephenc commented Aug 5, 2015

Uh oh!

recena commented Aug 5, 2015

Uh oh!

jglick commented Aug 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants