Stapler 1.250

[jglick]

Changes

@reviewbybees

[ghost]

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.

[daniel-beck]

Could we maybe get a more thorough review on this one? IIRC both previous Stapler upgrades broke Jenkins.

[daniel-beck]

Actual regression.

WARNING: Error while serving http://localhost:51807/jenkins/job/test0/descriptorByName/hudson.scm.CvsRepository/fillCompressionLevelItems
…
Caused by: java.lang.invoke.WrongMethodTypeException: cannot convert MethodHandle()ListBoxModel to (Object)Object
	at java.lang.invoke.MethodHandle.asType(MethodHandle.java:724)
	at java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:596)
	at org.kohsuke.stapler.Function$MethodFunction.invoke(Function.java:330)

[jglick]

@i386 I am guessing this is a consequence of jenkinsci/stapler#96; do you plan to take a look?

[jglick]

Or perhaps from jenkinsci/stapler@bd67c1e, which does not seem to have been done via PR at all.

[jglick]

Confirmed with git-bisect that jenkinsci/stapler#96 is responsible. Working on a fix.

[i386]

Nice catch @jglick

[egok]

why 1.249 instead of 1.250?

[jglick]

Well, 1.249 had the fix for the stated bug, 1.250 just corrected an obscure regression introduced by an unrelated change in 1.249.

[oleg-nenashev]

I would vote for 1.250. And it is better to just remove changelog change from the PR

[daniel-beck]

Given how problematic recent Stapler upgrades were, this shouldn't be merged into the weekly release that will serve as the base for next week's security update.

[svanoort]

🐝 -- I've looked at the changelog and nothing is jumping out at me as immediately problematic (though that may not mean much, since I'm not familiar with common Stapler problem areas).

Built it, ran it locally, installed plugins and created+ran jobs -- seems to run fine, plugins install, etc.

Not sure what else to do to properly exercise this one?

[jglick]

@reviewbybees done

Not sure what else to do to properly exercise this one?

Great, that is more than most PRs will get.

[oleg-nenashev]

@svanoort @jglick I propose to ensure there is no binary compatibility breakages before merging. Ideally we also need to ensure that ATH/PCT do not become more broken with this change.

[jglick]

ensure there is no binary compatibility breakages before merging

How do you propose to do that?

ensure that ATH/PCT do not become more broken

Without merging? How?

[oleg-nenashev]

1. Code review. I went through changes, and they seem to be fine from the compatibility perspective at least
2. Just build a custom core locally and run tests against it.

[jglick]

Sorry but I do not have the time or resources to run a potentially unbounded set of tests against a local build. Since when is that a prerequisite for merging changes to core?

[oleg-nenashev]

It is not a prerequisite, but we had pretty bad regressions on this year due to Stapler upgrades.

Feel free to merge if you are ready to take responsibility for handling the fallout if something goes wrong. I did it once on this year, and the Stapler maintainers (@kohsuke and @vivek) did not follow the commitments they made after that fallout. @jglick and we would have another regression, which has been prevented by you.

So I am not comfortable about merging before we test it using the tools we have.

[jglick]

if you are ready to take responsibility for handling the fallout if something goes wrong

Of course if any regression is discovered I would do my best to respond at once.

[daniel-beck]

If this breaks things again (for the fifth time in a year!) I'm not going to be happy.