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-36871] JNLP4-connect implementation #2492

Merged
merged 10 commits into from
Oct 20, 2016

Conversation

stephenc
Copy link
Member

@stephenc stephenc commented Aug 3, 2016

See JENKINS-36871

This adds the protocol as opt-in, however integrating the protocol required reworking some other classes.

JnlpAgentReceiver was actually broken (both in binary compatibility and functional compatibility) by the introduction of JNLP3-connect and we cannot identify any consumers other than a CloudBees closed source plugin, so breaking binary compatibility again is not critical

Todo

  • Test old remoting.jar against new code
  • Test new remoting.jar against old code
  • Restore the cookie behaviour (but done right this time)
  • Perhaps investigate issuing clients with TLS certificates (but would require a UI for managing them)

@jenkinsci/code-reviewers @reviewbybees

See also jenkinsci/remoting#92

…ation

Todo

- [ ] Restore the cookie behaviour (but done right this time)
- [ ] Perhaps investigate issuing clients with TLS certificates (but would require a UI for managing them)
@ghost
Copy link

ghost commented Aug 3, 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.

@oleg-nenashev oleg-nenashev added work-in-progress The PR is under active development, not ready to the final review needs-more-reviews Complex change, which would benefit from more eyes labels Aug 3, 2016
@stephenc
Copy link
Member Author

stephenc commented Aug 8, 2016

Needs matching release of remoting, but functionally complete

@stephenc stephenc removed the work-in-progress The PR is under active development, not ready to the final review label Aug 8, 2016
@@ -56,10 +47,29 @@
* @author Kohsuke Kawaguchi
* @since 1.467
*/
@Extension @Symbol("jnlp")
@Extension
@Symbol("jnlp")
Copy link
Member

Choose a reason for hiding this comment

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

sneaky :-)

@oleg-nenashev
Copy link
Member

@jenkinsci/code-reviewers Before going forward, it would be definitely great to know your opinion regarding this binary-incompatible change. @stephenc asked in the Dev List in Twitter and got no response, but it's not a 100% guarantee it's not being used anywhere. But actually it's highly unlikely

@oleg-nenashev oleg-nenashev added the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 13, 2016
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Remoting 3 is available. This PR is unblocked

keyStore.setKeyEntry("jenkins", privateKey, password, new X509Certificate[]{identityCertificate});
}
} catch (KeyStoreException e) {
// ignore
Copy link
Member

Choose a reason for hiding this comment

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

Logging at least?

@@ -180,7 +180,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>remoting</artifactId>
<version>2.61</version>
<version>3.0-20160811.164749-2</version> <!-- TODO pick up release of remoting -->
Copy link
Member

Choose a reason for hiding this comment

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

remoting-3 is ready

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll refresh this PR

@stephenc stephenc removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 18, 2016
@oleg-nenashev
Copy link
Member

🐝 Test failure is unrelated

@jenkinsci/code-reviewers
As per voting results from the previous governance meeting (summary), we can proceed with the common PR integration approach.

Integration of remoting 3 is being discussed here. By now there are 3 +1s and no -1s. So technically this PR qualifies the merge criteria.

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Oct 18, 2016
@oleg-nenashev
Copy link
Member

Merging after the last call for the comments in the developer mailing list.

@oleg-nenashev oleg-nenashev merged commit 71cbe0c into jenkinsci:master Oct 20, 2016
@stephenc
Copy link
Member Author

w00t!

@stephenc stephenc deleted the jenkins-36871 branch October 20, 2016 12:52
oleg-nenashev added a commit that referenced this pull request Oct 23, 2016
@jtnord
Copy link
Member

jtnord commented Dec 2, 2016

How did I not see that this broke binary and source backwards compatibility...
Are we getting Jenkins 3.0 now :-)

@stephenc
Copy link
Member Author

stephenc commented Dec 2, 2016

@jtnord... what ya complaining about... you're getting to rip out all the code that we open sourced and replace with references to the open sourced version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants