Make BulkChange auto-closeable #2428

Merged
merged 2 commits into from Jun 29, 2016

Conversation

7 participants
@kohsuke
Member

kohsuke commented Jun 28, 2016

so that it can be used as below:

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

@reviewbybees

Make BulkChange auto-closeable
so that it can be used as below:

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

@kohsuke kohsuke referenced this pull request in jenkinsci/github-organization-folder-plugin Jun 28, 2016

Merged

Add remove unbalance #17

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees 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.

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.

@@ -35,6 +35,8 @@
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
+
+import static org.bouncycastle.asn1.bc.BCObjectIdentifiers.bc;

This comment has been minimized.

@stephenc

stephenc Jun 28, 2016

Member

🐛

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Jun 28, 2016

Member

🐛 on the bouncycastle import

Member

stephenc commented Jun 28, 2016

🐛 on the bouncycastle import

@@ -25,6 +25,7 @@
import hudson.model.Saveable;
+import java.io.Closeable;

This comment has been minimized.

@jtnord

jtnord Jun 28, 2016

Member

NIT: should be java.lang.AutoCloseable according to the commit description

@jtnord

jtnord Jun 28, 2016

Member

NIT: should be java.lang.AutoCloseable according to the commit description

This comment has been minimized.

@johnou

johnou Jun 28, 2016

Member

+1

This comment has been minimized.

@jglick

jglick Jun 29, 2016

Member

Well Closeable extends AutoCloseable so this works as well. I do not see any harm in implementing the stronger Closeable.

@jglick

jglick Jun 29, 2016

Member

Well Closeable extends AutoCloseable so this works as well. I do not see any harm in implementing the stronger Closeable.

+ * Alias for {@link #abort()} to make {@link BulkChange} auto-closeable.
+ */
+ @Override
+ public void close() {

This comment has been minimized.

@johnou

johnou Jun 28, 2016

Member

abort will be invoked every time BulkChange is used as an auto-closable resource.. shouldn't commit be invoked on success?

@johnou

johnou Jun 28, 2016

Member

abort will be invoked every time BulkChange is used as an auto-closable resource.. shouldn't commit be invoked on success?

This comment has been minimized.

@johnou

johnou Jun 28, 2016

Member

ah I see, subsequent calls to close / abort will be ignored because of the completed flag.

@johnou

johnou Jun 28, 2016

Member

ah I see, subsequent calls to close / abort will be ignored because of the completed flag.

@jglick jglick removed the needs-fix label Jun 29, 2016

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jun 29, 2016

Member

🐝

Member

jglick commented Jun 29, 2016

🐝

@kohsuke kohsuke merged commit 213363d into master Jun 29, 2016

1 check passed

Jenkins This pull request looks good
Details

@kohsuke kohsuke deleted the closeable-bulkchange branch Jun 29, 2016

oleg-nenashev added a commit that referenced this pull request Jul 3, 2016

alvarolobato added a commit to alvarolobato/jenkins that referenced this pull request Jul 7, 2016

Merge branch 'master' of github.com:jenkinsci/jenkins into refactor-m…
…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 (#2435)
  Noting #2417, #2428, #2424, #2390, #2421, #2425, #2419, #1976
  [FIXED JENKINS-25416][JENKINS-28790] Do not inject build variables into maven process optionally (#1976)
  [JENKINS-20187] - Handle growing files when creating a tar file. (#2419)
  Noting merge of JENKINS-36280
  [JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored (#2426)
  [FIXED JENKINS-36232] NPE during SCM polling (#2425)
  [JENKINS-21486] Fixing optional dependency version resolution. (#2421)
  Stray import.
  [JENKINS-36280] Add some tests
  Update BUILD_TAG description to mention the replacement of slashes with dashes (#2417)
  Tests did not match naming pattern so were never being executed
  [JENKINS-32027]  Avoiding to refresh codemirror through the layoutUpdateCallback (#2390)
  [FIXED JENKINS-36277] Check that process working dir exists (#2424)
  [JENKINS-36280] Address review comments
  Make BulkChange auto-closeable
  [FIXED JENKINS-36280] Enable DescriptorVisibilityFilter for Slave's ComputerLauncher, RetentionStrategy and NodeProperty
  ...

samatdav added a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016

samatdav added a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016

[FIXED JENKINS-36277] Check that process working dir exists (#2424)
[JENKINS-32027]  Avoiding to refresh codemirror through the layoutUpdateCallback (#2390)

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

Divergence introduced with 117d69b.

Tests did not match naming pattern so were never being executed

[JENKINS-21486] Fixing optional dependency version resolution. (#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 (#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 (#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 #2429 is merged

Noting merge of JENKINS-36280

[JENKINS-20187] - Handle growing files when creating a tar file. (#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 (#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 #2417, #2428, #2424, #2390, #2421, #2425, #2419, #1976

[JENKINS-25416][JENKINS-28790] - Fix the parametersReferencedFromPropertiesShouldRetainBackslashes() test, which relies on the legacy behavior (#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment