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.

Sign up for GitHub

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

Jump to bottom

Stapler 1.253 #3103

Closed
wants to merge 4 commits into from
Closed

Stapler 1.253 #3103

wants to merge 4 commits into from

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 20, 2017 •
edited
Loading

Changelog and diffs.

Also Commons Codec changelog.

Proposed changelog entries

  • Stapler library upgraded to 1.253:
    • Improved performance for some Blue Ocean operations.
    • Various changes which could be of interest to plugin developers, for which see component changelog.
  • Commons Codec library upgraded to 1.9.

Desired reviewers

@jenkinsci/code-reviewers @reviewbybees

@jglick jglick requested a review from oleg-nenashev October 20, 2017 23:14
@jglick jglick added the needs-more-reviews Complex change, which would benefit from more eyes label Oct 20, 2017
@ghost
Copy link

ghost commented Oct 20, 2017

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 approved these changes Oct 20, 2017
View reviewed changes
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.

🐝 for the integration, but the changelog entry could be improved a bit. E.g. the binary compatibility may worth mentioning

oleg-nenashev
oleg-nenashev previously requested changes Oct 21, 2017
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are Enforcer issues:

[WARNING] Rule 3: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:

Failed while enforcing RequireUpperBoundDeps. The error(s) are [

Require upper bound dependencies error for commons-codec:commons-codec:1.8 paths to dependency are:

+-org.jenkins-ci.main:jenkins-core:2.86-SNAPSHOT
  +-commons-codec:commons-codec:1.8
and
+-org.jenkins-ci.main:jenkins-core:2.86-SNAPSHOT
  +-org.jenkins-ci.main:cli:2.86-SNAPSHOT
    +-commons-codec:commons-codec:1.4
and
+-org.jenkins-ci.main:jenkins-core:2.86-SNAPSHOT
  +-commons-httpclient:commons-httpclient:3.1-jenkins-1
    +-commons-codec:commons-codec:1.8
and
+-org.jenkins-ci.main:jenkins-core:2.86-SNAPSHOT
  +-org.acegisecurity:acegi-security:1.0.7
    +-commons-codec:commons-codec:1.3
and
+-org.jenkins-ci.main:jenkins-core:2.86-SNAPSHOT
  +-org.kohsuke.stapler:stapler-jrebel:1.253
    +-org.kohsuke.stapler:stapler:1.253
      +-commons-codec:commons-codec:1.9
]

@oleg-nenashev
Copy link
Member

Commons Codec 1.9 has been introduced by me in jenkinsci/stapler@4d59a74, upper bounds in Stapler

@oleg-nenashev
Copy link
Member

I did it in the Stapler Core because of the following:

Require upper bound dependencies error for commons-codec:commons-codec:1.8 paths to dependency are:
+-org.kohsuke.stapler:stapler:1.254-SNAPSHOT
  +-commons-codec:commons-codec:1.8
and
+-org.kohsuke.stapler:stapler:1.254-SNAPSHOT
  +-org.jvnet.hudson:htmlunit:2.6-hudson-2
    +-commons-codec:commons-codec:1.4
and
+-org.kohsuke.stapler:stapler:1.254-SNAPSHOT
  +-org.jvnet.hudson:htmlunit:2.6-hudson-2
    +-commons-httpclient:commons-httpclient:3.1
      +-commons-codec:commons-codec:1.2
and
+-org.kohsuke.stapler:stapler:1.254-SNAPSHOT
  +-org.apache.httpcomponents:httpmime:4.5
    +-org.apache.httpcomponents:httpclient:4.5
      +-commons-codec:commons-codec:1.9

@jglick jglick dismissed oleg-nenashev’s stale review October 23, 2017 13:48

Enforcer issue addressed.

@jglick jglick requested a review from oleg-nenashev October 23, 2017 13:49
oleg-nenashev
oleg-nenashev reviewed Oct 23, 2017
View reviewed changes
core/src/main/java/jenkins/slaves/EncryptedSlaveAgentJnlpFile.java
@@ -70,7 +70,7 @@ public void generateResponse(StaplerRequest req, StaplerResponse res, Object nod
final ByteArrayOutputStream baos = new ByteArrayOutputStream();
StaplerResponse temp = new ResponseImpl(req.getStapler(), new HttpServletResponseWrapper(res) {
@Override public ServletOutputStream getOutputStream() throws IOException {
return new FilterServletOutputStream(baos);
return new FilterServletOutputStream(baos, super.getOutputStream());
Copy link
Member

Choose a reason for hiding this comment

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

Could it cause issues for downstream deps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess? Ask @jtnord who requested this.

Copy link
Member Author

Choose a reason for hiding this comment

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

See discussion: jenkinsci/stapler#131 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

this is the wrong change IIUC. that wraps the output stream of the response - which will return false information.

The correct fix is https://github.com/jenkinsci/jenkins/pull/3033/files#diff-8991f368b3f7346bf83c7081c719c809R118

Copy link
Member

Choose a reason for hiding this comment

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

FilterServletOutputStream is likely to be removed in a future version - its only there to do home grown GZIP which is causing more harm that good, and other places have abused it to do things like this...

@oleg-nenashev oleg-nenashev requested a review from jtnord October 25, 2017 16:07
@jtnord
Copy link
Member

jtnord commented Oct 25, 2017

I beleive this is missing other changes for dependencies to align with stapler and servlet 3.x spec
see #3033

@oleg-nenashev
Copy link
Member

@jtnord @jglick Would it be possible to prevent the binary compatibility breakage by adding a bridge method somehow?

@jglick
Copy link
Member Author

jglick commented Oct 25, 2017

adding a bridge method

-1, too crazy. We could restore and deprecate the original constructor (and just have the new methods throw UnsupportedOperationException or something). But it looks like this PR just needs to be replaced by #3033 (after that bumps up to the release version), right @jtnord?

@jtnord
Copy link
Member

jtnord commented Oct 25, 2017

Would it be possible to prevent the binary compatibility breakage

As I commented on the upstream PR to stapler :`

currently on 2 uses outside of stapler that I found - I have PRs for both, both of which means this can be completely removed in #130

@jtnord
Copy link
Member

jtnord commented Oct 26, 2017

@jglick I think we should close this in favour of #3033

@jglick
Copy link
Member Author

jglick commented Oct 26, 2017

OK.

@jglick jglick closed this Oct 26, 2017
@jglick jglick deleted the stapler-1.253 branch October 26, 2017 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev Awaiting requested review from oleg-nenashev

@jtnord jtnord Awaiting requested review from jtnord

Assignees
No one assigned
Labels
needs-more-reviews Complex change, which would benefit from more eyes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jglick @oleg-nenashev @jtnord