-
Notifications
You must be signed in to change notification settings - Fork 104
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-32503] Update dependency of beanutils #81
Conversation
No PR builds? 0_o |
Commit that introduced that test c3004de |
Any help is welcome ;) |
This test also fails on 1.8.3 that used in Jenkins. |
@rsandell maybe you can help? :) |
* It is also possible to transform it to `withXXX()` to have clear vision. | ||
*/ | ||
@DataBoundSetter | ||
// public void setItems(List<String> items) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course it dead code, because it WIP
public FluentSetter() {} | ||
|
||
/** | ||
* WIP This tests passes in stapler, but fluent doesn't work in jenkins... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @vivek
|
it's known, i have no power to fix scalarTest that already in failure state for current jenkins core and seems i'm missing something... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 excepting the dead code concern
@KostyaSha could you please rebase it to the master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be shipped IMHO. though it's still marked as WiP. PTAL @jglick
@@ -44,7 +44,7 @@ | |||
<dependency> | |||
<groupId>commons-beanutils</groupId> | |||
<artifactId>commons-beanutils</artifactId> | |||
<version>1.7.0</version> | |||
<version>1.9.3</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too new, and see #134.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be revisited? We are running into some serious dependency issues because its at 1.8.3
*/ | ||
@DataBoundSetter | ||
// public void setItems(List<String> items) { | ||
public FluentSetter setItems(List<String> items) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems potentially useful, but why is it filed here? Should be split into a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fluentsetters was exactly the reason to update beanutils that provides FluentPropertyBeanIntrospector to be used in core... But i had working testcase here and failing in core + unclear test failure stuck me.
When we tried with oleg it seems passed test. Kohsuke changed databinding
in core, so maybe it already works without beanutils update. Need try
again.
|
Compare #188. |
@jglick sp if everything is solved now, then core and test could simplify code with using fluent setters. Requires changes to return |
Potentially useful new API, but would need various sorts of work to be mergeable. |
https://issues.jenkins-ci.org/browse/JENKINS-32503
Testing PR build with dependency update as is.
Jenkins is already on 1.8.3 https://github.com/jenkinsci/jenkins/blob/74c534207ca0b97b0345bf413b63f2450ccc8d04/core/pom.xml#L277-L279
That fails
This change is