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

[JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored #2426

Merged
merged 3 commits into from
Jul 1, 2016
Merged

[JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored #2426

merged 3 commits into from
Jul 1, 2016

Conversation

jglick
Copy link
Member

@jglick jglick commented Jun 28, 2016

JENKINS-35906

Corrects a regression from #2371.

@reviewbybees

@ghost
Copy link

ghost commented Jun 28, 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.

@kohsuke
Copy link
Member

kohsuke commented Jun 28, 2016

I feel like we should be killing Descriptor.newInstance() and also we do some magic during databinding a tree of JSON objects so that any Descriptor.newInstance() overrides are honored, but 🐝 to fix regressions

@kohsuke
Copy link
Member

kohsuke commented Jun 28, 2016

Descriptor.NewInstanceBindInterceptor is the magic I was referring to. I looked at the code and this interceptor only gets installed via Descriptor.newInstance(), which this change ensures we will be using.

@jglick
Copy link
Member Author

jglick commented Jun 28, 2016

this interceptor only gets installed via Descriptor.newInstance()

Right, so it works on nested calls, but at top level you still need to ask for newInstance.

@oleg-nenashev
Copy link
Member

🐝 , conflicts with #2422

@oleg-nenashev
Copy link
Member

@reviewbybees done in any case.
@jglick Could you investigate the NPE risk?

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Jun 29, 2016
@oleg-nenashev oleg-nenashev mentioned this pull request Jun 29, 2016
Closed
@oleg-nenashev
Copy link
Member

🐛 since newInstanceFromRadioList may return null according to the code. I propose to add annotation to this method and explicitly check null in the code

@oleg-nenashev
Copy link
Member

🐝

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 30, 2016
@daniel-beck daniel-beck added the regression-fix Pull request that fixes a regression in one of the previous Jenkins releases label Jun 30, 2016
@oleg-nenashev oleg-nenashev merged commit 1d176d1 into jenkinsci:master Jul 1, 2016
@jglick jglick deleted the SCMS.parseSCM-JENKINS-35906 branch July 1, 2016 15:15
@oleg-nenashev oleg-nenashev mentioned this pull request Jul 5, 2016
Merged
alvarolobato added a commit to alvarolobato/jenkins that referenced this pull request Jul 7, 2016
@alvarolobato
Merge branch 'master' of github.com:jenkinsci/jenkins into refactor-m…
45d0920 
…aven-builder

* 'master' of github.com:jenkinsci/jenkins: (161 commits)
  updated changelog for release
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release jenkins-2.12
  [JENKINS-25416][JENKINS-28790] - Fix the parametersReferencedFromPropertiesShouldRetainBackslashes() test, which relies on the legacy behavior (jenkinsci#2435)
  Noting jenkinsci#2417, jenkinsci#2428, jenkinsci#2424, jenkinsci#2390, jenkinsci#2421, jenkinsci#2425, jenkinsci#2419, jenkinsci#1976
  [FIXED JENKINS-25416][JENKINS-28790] Do not inject build variables into maven process optionally (jenkinsci#1976)
  [JENKINS-20187] - Handle growing files when creating a tar file. (jenkinsci#2419)
  Noting merge of JENKINS-36280
  [JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored (jenkinsci#2426)
  [FIXED JENKINS-36232] NPE during SCM polling (jenkinsci#2425)
  [JENKINS-21486] Fixing optional dependency version resolution. (jenkinsci#2421)
  Stray import.
  [JENKINS-36280] Add some tests
  Update BUILD_TAG description to mention the replacement of slashes with dashes (jenkinsci#2417)
  Tests did not match naming pattern so were never being executed
  [JENKINS-32027]  Avoiding to refresh codemirror through the layoutUpdateCallback (jenkinsci#2390)
  [FIXED JENKINS-36277] Check that process working dir exists (jenkinsci#2424)
  [JENKINS-36280] Address review comments
  Make BulkChange auto-closeable
  [FIXED JENKINS-36280] Enable DescriptorVisibilityFilter for Slave's ComputerLauncher, RetentionStrategy and NodeProperty
  ...
samatdav pushed a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016
@jglick @samatdav
[JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are h…
533b029 
…onored (jenkinsci#2426)

* [FIXED JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored.

* [FIXED JENKINS-36043] Work around fragile form submission design in multi-branch-project-plugin.
samatdav pushed a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016
@vjuranek @samatdav
[FIXED JENKINS-36277] Check that process working dir exists (jenkinsc…
8a41f4c 
…i#2424)

[JENKINS-32027]  Avoiding to refresh codemirror through the layoutUpdateCallback (jenkinsci#2390)

Update BUILD_TAG description to mention the replacement of slashes with dashes (jenkinsci#2417)

Divergence introduced with 117d69b.

Tests did not match naming pattern so were never being executed

[JENKINS-21486] Fixing optional dependency version resolution. (jenkinsci#2421)

Without it version of optional dependencies is set to "X.Y;resolution:=optional" which causes problems when using hudson.util.VersionNumber comparison methods.

Make BulkChange auto-closeable

so that it can be used as below:

    try (BulkChange bc = new BulkChange(o)) {
       ...
       bc.commit();
    }

Stray import.

[FIXED JENKINS-36232] NPE during SCM polling (jenkinsci#2425)

* [FIXED JENKINS-36232] NPE during SCM polling

* add test to trigger the "no veto" code path that has the NPE

* incorporate remark on using @issue reference

[JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored (jenkinsci#2426)

* [FIXED JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored.

* [FIXED JENKINS-36043] Work around fragile form submission design in multi-branch-project-plugin.

[FIXED JENKINS-36280] Enable DescriptorVisibilityFilter for Slave's ComputerLauncher, RetentionStrategy and NodeProperty

[JENKINS-36280] Address review comments

[JENKINS-36280] Add some tests

The tests pass locally, but they will not run until jenkinsci#2429 is merged

Noting merge of JENKINS-36280

[JENKINS-20187] - Handle growing files when creating a tar file. (jenkinsci#2419)

* [JENKINS-20187] - Handle growing files when creating a tar file.

* - Introduce try-with-resources
- Remove exception login
- Throw new exception with filename and addSuppresed

* Address jglick comments

[FIXED JENKINS-25416][JENKINS-28790] Do not inject build variables into maven process optionally (jenkinsci#1976)

* [FIXED JENKINS-25416][JENKINS-28790] Do not inject build variables into maven process optionally

* [JENKINS-25416][JENKINS-28790] Fixed typo in test method name

* [JENKINS-25416][JENKINS-28790] Suggest alternatives in help

* [JENKINS-25416][JENKINS-28790] Use less hackish approach to preserving legacy behaviour

Noting jenkinsci#2417, jenkinsci#2428, jenkinsci#2424, jenkinsci#2390, jenkinsci#2421, jenkinsci#2425, jenkinsci#2419, jenkinsci#1976

[JENKINS-25416][JENKINS-28790] - Fix the parametersReferencedFromPropertiesShouldRetainBackslashes() test, which relies on the legacy behavior (jenkinsci#2435)

The test fails after jenkinsci@84ee34f

[maven-release-plugin] prepare release jenkins-2.12

[maven-release-plugin] prepare for next development iteration

updated changelog for release

merge upstream

merge upstream

removed  variable

fixed form neabling issue

merge upstream
olivergondza pushed a commit that referenced this pull request Aug 23, 2016
@jglick @olivergondza
[JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are h…
27d8f06 
…onored (#2426)

* [FIXED JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored.

* [FIXED JENKINS-36043] Work around fragile form submission design in multi-branch-project-plugin.

(cherry picked from commit 1d176d1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs-more-reviews Complex change, which would benefit from more eyes ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback regression-fix Pull request that fixes a regression in one of the previous Jenkins releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jglick @kohsuke @oleg-nenashev @daniel-beck