[JENKINS-23786] Permit the Shell plugin to set a build result as unstable via a return code #2563

stochmial · 2016-09-23T14:04:16Z

Continuation of the pull request: #1325

The CommandInterpreter.join(Proc p) method is supposed to be an extension point to allow an UNSTABLE build result to be set, but it has no access to the Build object with which to do so. Add a getBuild() method that returns the build passed to perform(...). A better API would be a new join(...) with more arguments, but that'd break out of tree plugins that might already be using this for other purposes.

Currently a shell job has to make a HTTP call back to Jenkins to set its build result as unstable. This is slow, requires the slave to have access to the master's HTTP interface, and is fiddly. The alternative, the TextFinder plugin, is no better. Instead, allow a job to set the build result to unstable with a return value. Adds the Advanced parameter "unstableReturn" which, if non-zero, is the code the script must return to set the build as unstable.

daniel-beck · 2016-09-26T01:12:43Z

core/src/main/java/hudson/tasks/BatchFile.java

@@ -73,7 +84,13 @@ public String getDisplayName() {

        @Override
        public Builder newInstance(StaplerRequest req, JSONObject data) {
-            return new BatchFile(data.getString("command"));
+            final String unstableReturnStr = data.getString("unstableReturn");


Why is this even using newInstance? Should just be databinding.

fixed (hopefully) in next commit

daniel-beck · 2016-09-26T01:13:19Z

core/src/main/java/hudson/tasks/BatchFile.java

@@ -40,10 +40,17 @@
 */
 public class BatchFile extends CommandInterpreter {
    @DataBoundConstructor
-    public BatchFile(String command) {
+    public BatchFile(String command, Integer unstableReturn) {


Use of @DataboundSetter for new arguments would be preferable over adding new constructors.

fixed (hopefully) in next commit

daniel-beck · 2016-09-26T01:13:48Z

core/src/main/java/hudson/tasks/CommandInterpreter.java

+      * Useful for {@link #join(Proc p)} for setting build results.
+      *
+     * @return The build being run, or null if outside perform(...)
+     * @since 1.573


daniel-beck · 2016-09-26T01:14:07Z

core/src/main/java/hudson/tasks/CommandInterpreter.java

@@ -97,6 +118,7 @@ public boolean perform(AbstractBuild<?,?> build, Launcher launcher, TaskListener
                Util.displayIOException(e, listener);
                e.printStackTrace(listener.fatalError(Messages.CommandInterpreter_CommandFailed()));
            }
+            this.build = null;


Shouldn't this be in the finally block?

Refactored in next commit

daniel-beck · 2016-09-26T01:15:21Z

core/src/main/java/hudson/tasks/Shell.java

    @DataBoundConstructor
-    public Shell(String command) {
+    public Shell(String command, Integer unstableReturn) {


See BatchFile.

fixed (hopefully) in next commit

daniel-beck · 2016-09-26T01:16:54Z

core/src/main/resources/hudson/tasks/BatchFile/config.jelly

@@ -28,4 +28,10 @@ THE SOFTWARE.
           description="${%description(rootURL)}">
    <f:textarea name="command" value="${instance.command}" class="fixed-width" />
  </f:entry>
+  <f:advanced>
+    <f:entry title="errorlevel to set build unstable"
+             description="${%unstableReturn_description(rootURL)}">


No argument needed as there's no placeholder in the localized string.

added help section and fixed in next commit

daniel-beck · 2016-09-26T01:17:10Z

core/src/main/resources/hudson/tasks/BatchFile/config.jelly

@@ -28,4 +28,10 @@ THE SOFTWARE.
           description="${%description(rootURL)}">
    <f:textarea name="command" value="${instance.command}" class="fixed-width" />
  </f:entry>
+  <f:advanced>
+    <f:entry title="errorlevel to set build unstable"


Unclear why this isn't just using databinding.

fixed (hopefully) in next commit

daniel-beck · 2016-09-26T01:17:24Z

core/src/main/resources/hudson/tasks/BatchFile/config.jelly

+  <f:advanced>
+    <f:entry title="errorlevel to set build unstable"
+             description="${%unstableReturn_description(rootURL)}">
+      <f:number clazz="positive-number" name="unstableReturn" value="${instance.unstableReturn}" min="1" max="255" step="1" />


Are these min/max values universal?

daniel-beck · 2016-09-26T01:18:17Z

core/src/main/resources/hudson/tasks/Shell/config.groovy

+
+f.advanced() {
+
+    f.entry(title:_("Return code to set build unstable"), description:_("If set, the script return code that will be interpreted as an unstable build result.")) {


Why is this inline, when the BatchFile equivalent is in the view's default locale resource file?

fixed (hopefully) in next commit

daniel-beck · 2016-09-26T01:19:04Z

core/src/main/resources/hudson/tasks/Shell/config.groovy

+f.advanced() {
+
+    f.entry(title:_("Return code to set build unstable"), description:_("If set, the script return code that will be interpreted as an unstable build result.")) {
+        f.number(name: "unstableReturn", value: instance?.unstableReturn, min:1, max:255, step:1)


Where's the 'positive number' part present in BatchFile's form? Not needed? Then why is it present there?

fixed (hopefully) in next commit

stochmial · 2016-09-26T13:55:02Z

Hi Daniel, thanks for review. I think the next commit should fix all requests. I'm not sure why one of the tests failed. I don't think the changes are related to this test. I need assistance from someone more experienced to get more understanding.

daniel-beck · 2016-09-26T18:04:40Z

@stochmial Legitimate failure. FindBugs complained as follows:

req must be non-null but is marked as nullable [hudson.tasks.BatchFile$DescriptorImpl] At BatchFile.java:[line 97] NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE

daniel-beck · 2016-09-26T18:13:39Z

core/src/main/java/hudson/tasks/BatchFile.java

-        public Builder newInstance(StaplerRequest req, JSONObject data) {
-            return new BatchFile(data.getString("command"));
+        public Builder newInstance(StaplerRequest req, JSONObject formData) {
+            return req.bindJSON(BatchFile.class,formData);


This is the FindBugs issue since req is CheckForNull.

Still not clear to me why this is even overridden. Doesn't the default Descriptor#newInstance(…) do the exact same thing?

Of course this annotation could soon be obsolete.

daniel-beck

We're getting really close here. Only few minor issues left IMO.

daniel-beck · 2016-09-27T07:35:51Z

core/src/main/resources/hudson/tasks/BatchFile/help-unstableReturn.html

+<div>
+    If set, the batch errorlevel result that will be interpreted as an unstable build result.
+    In WinNT4 and beyond, the ERRORLEVEL is stored as a Long - that is, a four byte, signed integer;
+    yielding maximum and minimum values of 2147483647 and -2147483648, respectively.


Are all of these values actually used?

I tested a simple batch file with a single line: 'exit /b -2147483648' and the errorlevel is set to '-2147483648'. It works on win7, etc. I think it's better not to limit the available range, although hardly used.

daniel-beck · 2016-09-27T07:36:24Z

core/src/main/resources/hudson/tasks/BatchFile/help-unstableReturn.html

@@ -0,0 +1,5 @@
+<div>
+    If set, the batch errorlevel result that will be interpreted as an unstable build result.
+    In WinNT4 and beyond, the ERRORLEVEL is stored as a Long - that is, a four byte, signed integer;


WinNT4? Why not Windows NT 4? Is that a common identifier of some kind?

I agree this help text should be corrected. Not sure how much detail is needed here. I thought this text to be rather a hint than the complete documentation. How about:

"If set, the batch errorlevel result that will be interpreted as an unstable build result. If the final errorlevel matches the value, the build results will be set to unstable and next steps will be continued. Supported values match the widest errorlevel range for Windows like systems. In Windows NT4 and beyond the ERRORLEVEL is stored as a four byte, signed integer, yielding maximum and minimum values of 2147483647 and -2147483648, respectively. Older versions of Windows use 2 bytes. DOS like systems use single byte, yelding errorlevels between 0-255 ."

daniel-beck · 2016-09-27T07:37:23Z

core/src/main/resources/hudson/tasks/Shell/help-unstableReturn.html

@@ -0,0 +1,3 @@
+<div>
+    If set, the shell exit code that will be interpreted as an unstable build result. On Unix-like systems the value between 0-255.


Second sentence missing its verb.

Also, seems overly terse, could use an explanation what this means. E.g. not aborting the build if that's the result, but continuing.

How about:
“If set, the shell exit code that will be interpreted as an unstable build result. If the exit code matches the value, the build results will be set to 'unstable' and next steps will be continued. On Unix-like it is a value between 0-255.”

daniel-beck · 2016-09-27T07:39:26Z

core/src/main/java/hudson/tasks/CommandInterpreter.java

@@ -64,6 +66,13 @@ public boolean perform(AbstractBuild<?,?> build, Launcher launcher, BuildListene
        return perform(build,launcher,(TaskListener)listener);
    }

+    /**
+     * Allow the user to define a result for "unstable".


Not actually describing what this does.

Missing @since TODO.

daniel-beck · 2016-09-27T07:40:59Z

core/src/main/resources/hudson/tasks/BatchFile/config.jelly

@@ -28,4 +28,9 @@ THE SOFTWARE.
           description="${%description(rootURL)}">
    <f:textarea name="command" value="${instance.command}" class="fixed-width" />
  </f:entry>
+  <f:advanced>
+    <f:entry title="${%Errorlevel to set build unstable}" field="unstableReturn" >
+      <f:number name="unstableReturn" value="${instance.unstableReturn}" min="-2147483648" max="2147483647" step="1" />


Why is name needed here, doesn't field take care of this?

daniel-beck · 2016-09-27T07:41:30Z

core/src/main/resources/hudson/tasks/Shell/config.groovy

+
+f.advanced() {
+    f.entry(title:_("Exit code to set build unstable"), field: "unstableReturn") {
+        f.number(clazz:"positive-number", name: "unstableReturn", value: instance?.unstableReturn, min:1, max:255, step:1)


Form validation allows 0...255, while this has the range of 1...255, why?

There is a general question if we should allow users to define exit code zero as an unstable build? I think better not.

Let's start with 'no' and open up if there's an actual need for it.

daniel-beck · 2016-09-27T07:43:58Z

core/src/main/java/hudson/tasks/BatchFile.java

+        /**
+         * Performs on-the-fly validation of the exit code.
+         */
+        public FormValidation doCheckUnstableReturn(@QueryParameter String value) {


@Restricted(DoNotUse.class) wouldn't be a bad idea.

daniel-beck · 2016-09-27T07:46:53Z

I didn't point them out individually, but a whole bunch of @since TODO and @Restricted are missing here.

@Restricted for anything not intended for use in plugins, e.g. all doCheck seem like good candidates.
@since TODO required for every protected or public field or method newly added, unless @Restricted (in which case it's optional IMO, but we're a bit inconsistent here).

TODO is literal, do not fill in the version you expect this to go in. Better TODO than wrong if nobody catches it.

stochmial · 2016-09-28T05:38:01Z

All requests should be corrected.

daniel-beck

Minor messaging issues, but otherwise okay 👍

@jenkinsci/code-reviewers Please review.

daniel-beck · 2016-09-28T06:54:33Z

core/src/main/resources/hudson/tasks/Messages.properties

@@ -81,3 +83,5 @@ Maven.NotMavenDirectory={0} doesn\u2019t look like a Maven directory
 Maven.NoExecutable=Couldn\u2019t find any executable in {0}

 Shell.DisplayName=Execute shell
+Shell.invalid_exit_code_range=Invalid exit code value: {0}. Check help section


Missing trailing period.

I'm not sure, because no messages there have periods in the end.

BatchFile.invalid_exit_code_range has, so one of these is wrong.

Ok, fixed by removing all trailing periods.

daniel-beck · 2016-09-28T06:55:27Z

core/src/main/resources/hudson/tasks/Messages.properties

@@ -81,3 +83,5 @@ Maven.NotMavenDirectory={0} doesn\u2019t look like a Maven directory
 Maven.NoExecutable=Couldn\u2019t find any executable in {0}

 Shell.DisplayName=Execute shell
+Shell.invalid_exit_code_range=Invalid exit code value: {0}. Check help section
+Shell.invalid_exit_code_zero=Exit code zero is not allowed to make build unstable


Form validation does not prevent users from saving the form. I expect the '0' value to actually be saved. So, strongly discouraged, with explanation that it's never going to be successful perhaps?

I changed from error to warning and added explanation in help message.

stochmial · 2016-09-28T08:09:59Z

Is there anything else needed to finish this? I.e. sqashing my commits? I would rather leave rigerc commits as they are and squash only mine.

daniel-beck · 2016-09-28T13:29:16Z

@stochmial In the current state, I would definitely squash when merging, so to retain @ringerc's commits, that would be useful. But please wait to do that until the reviews are finished.

oleg-nenashev · 2016-10-02T15:01:27Z

Noticed that I have not submitted the previous review :( redoing it

stochmial · 2016-10-03T11:44:18Z

@oleg: I still don't see your review.

oleg-nenashev · 2016-10-02T15:01:51Z

core/src/main/java/hudson/tasks/BatchFile.java

@@ -56,6 +65,20 @@ protected String getFileExtension() {
        return ".bat";
    }

+    public final Integer getUnstableReturn() {


Add CheckForNull.
🐜

oleg-nenashev · 2016-10-02T15:03:20Z

core/src/main/java/hudson/tasks/BatchFile.java

+                return FormValidation.warning(hudson.tasks.Messages.BatchFile_invalid_exit_code_zero());
+            }
+            if (unstableReturn < Integer.MIN_VALUE || unstableReturn > Integer.MAX_VALUE) {
+                return FormValidation.error(hudson.tasks.Messages.BatchFile_invalid_exit_code_range(unstableReturn));


Does Batch really support so wide return code range?

I tested a simple batch file with a single line: 'exit /b -2147483648' and the errorlevel is set to '-2147483648'. It works on win7, etc. I think it's better not to limit the available range, although hardly used.

oleg-nenashev · 2016-10-02T15:04:08Z

core/src/main/java/hudson/tasks/Shell.java

@@ -94,6 +102,20 @@ protected String getFileExtension() {
        return ".sh";
    }

+    public final Integer getUnstableReturn() {


CheckForNull

oleg-nenashev

Test implementation needs fix

oleg-nenashev · 2016-10-03T13:05:11Z

test/src/test/java/hudson/tasks/ShellTest.java

+    }
+
+    @Issue("JENKINS-23786")
+    public void testUnstableReturn() throws Exception {


🐛 No @Test annotation. JUnit 4 is not actually running this test

I would also add "Unix" to the test name

oleg-nenashev · 2016-10-03T13:05:47Z

test/src/test/java/hudson/tasks/ShellTest.java

+        b = rule.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0).get());
+
+        /* Creating unstable=0 produces unstable=null */
+        assertNull( createNewShell("",0).getUnstableReturn() );


Maybe needs splitting to several tests (or needs some comments)

I split and refactored tests.

oleg-nenashev · 2016-10-03T13:06:44Z

test/src/test/java/hudson/tasks/ShellTest.java

+
+    @Issue("JENKINS-23786")
+    public void testUnstableReturn() throws Exception {
+        if(Functions.isWindows())


🐜 Should be assumeThat in order to mark the test as skipped instead of passed.

oleg-nenashev · 2016-10-03T13:07:34Z

test/src/test/java/hudson/tasks/BatchFileTest.java

+    }
+
+    @Issue("JENKINS-23786")
+    public void testUnstableReturn() throws Exception {


Same. Missing Test annotation and no assumeThat()

oleg-nenashev · 2016-10-03T13:09:21Z

core/src/main/resources/hudson/tasks/BatchFile/config.jelly

@@ -28,4 +28,9 @@ THE SOFTWARE.
           description="${%description(rootURL)}">
    <f:textarea name="command" value="${instance.command}" class="fixed-width" />
  </f:entry>
+  <f:advanced>
+    <f:entry title="${%Errorlevel to set build unstable}" field="unstableReturn" >


Better to use ERRORLEVEL then

Changed 'Errorlevel' and 'ErrorLevel' to 'ERRORLEVEL' in all messageas.

oleg-nenashev · 2016-10-03T13:10:02Z

@stochmial Done, sorry for the delay

stochmial · 2016-10-05T06:37:50Z

I refactored tests and verified they are triggered.

oleg-nenashev · 2016-10-05T10:35:26Z

Looks good to me 👍

oleg-nenashev · 2016-10-05T10:35:53Z

@daniel-beck Are you fine with the revised version?

daniel-beck · 2016-10-07T12:14:58Z

@oleg-nenashev Code looks good, haven't run it for a while though.

If @stochmial could commit to fixing followup issues during the next few weeks that would be great.

stochmial · 2016-10-07T12:52:49Z

I am ready to fix issues. I also added push permissions to all participants

stochmial · 2016-10-07T12:55:13Z

I can see all check finished successfully but the status still says 'Some checks haven’t completed yet'. An error in CI process?

oleg-nenashev · 2016-10-09T21:56:42Z

I suppose it's just another glitch on the new CI instance.
One of the builds has passed, hence it's ready to go.

Not sure in which weekly release it will land

oleg-nenashev · 2016-10-10T06:14:13Z

I would like to squash these 18 commits, but there are two authors => it can be done only manually

stochmial · 2016-10-10T06:26:37Z

I squashed my commits

oleg-nenashev · 2016-10-10T06:45:04Z

Thanks! Could you also update the commit message? Just to add issue prefix in order to let the SCM <=> JIRA integration to mark the issue properly?

refactor and fix remove duplication and fix FB errors fix integration tests consistency fixes consistency fixes changed error message to warning fix comment and warning message fix comment fix junit tests fix junit tests fix junit tests fix junit tests clean import a commit to trigger jenkins checks

stochmial · 2016-10-10T11:52:54Z

Done

daniel-beck · 2016-10-15T20:19:36Z

@oleg-nenashev But… the commit message wasn't updated to be bot compatible?

oleg-nenashev · 2016-10-15T20:39:53Z

Haha, my bad since I have not checked the new commit message :(
I'll note it in the ticket

ringerc · 2016-10-18T04:24:43Z

Thanks for pushing this through. It's been two years since I wrote that first little patch but I stopped using Jenkins (stability/reliability issues with Matrix plugin + EC2-plugin) and didn't have a chance to pursue it. It's nice to see you did it better and got it in. I'm about to return to Jenkins for another project so it'll be nice to have.

stochmial · 2016-10-19T12:45:41Z

Thanks all for support

ringerc added 3 commits September 22, 2016 15:56

JENKINS-23786: Allow batch files to set an exit code for unstable builds

4b067fd

stochmial mentioned this pull request Sep 23, 2016

[WIP] JENKINS-23786: Permit the Shell plugin to set a build result as unstable via a return code #1325

Closed

daniel-beck requested changes Sep 26, 2016

View reviewed changes

daniel-beck reviewed Sep 26, 2016

View reviewed changes

daniel-beck requested changes Sep 27, 2016

View reviewed changes

daniel-beck reviewed Sep 27, 2016

View reviewed changes

daniel-beck approved these changes Sep 28, 2016

View reviewed changes

oleg-nenashev reviewed Oct 3, 2016

View reviewed changes

oleg-nenashev requested changes Oct 3, 2016

View reviewed changes

oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Oct 5, 2016

oleg-nenashev approved these changes Oct 5, 2016

View reviewed changes

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 Oct 9, 2016

oleg-nenashev removed the needs-more-reviews Complex change, which would benefit from more eyes label Oct 9, 2016

oleg-nenashev changed the title ~~[WIP] JENKINS-23786: Permit the Shell plugin to set a build result as unstable via a return code - rebased~~ [JENKINS-23786] Permit the Shell plugin to set a build result as unstable via a return code - rebased Oct 10, 2016

oleg-nenashev changed the title ~~[JENKINS-23786] Permit the Shell plugin to set a build result as unstable via a return code - rebased~~ [JENKINS-23786] Permit the Shell plugin to set a build result as unstable via a return code Oct 10, 2016

stochmial force-pushed the shell-setunstable-rebased branch from 7b5b040 to 33c3cae Compare October 10, 2016 06:24

stochmial force-pushed the shell-setunstable-rebased branch from 33c3cae to 359548e Compare October 10, 2016 11:52

oleg-nenashev merged commit 720b75f into jenkinsci:master Oct 11, 2016

oleg-nenashev added a commit that referenced this pull request Oct 15, 2016

Noting #2581, #2561, #2577, #1894, #2563 and #2585

b2627ed


		f.advanced() {

		f.entry(title:_("Return code to set build unstable"), description:_("If set, the script return code that will be interpreted as an unstable build result.")) {

		@@ -0,0 +1,3 @@
		<div>
		If set, the shell exit code that will be interpreted as an unstable build result. On Unix-like systems the value between 0-255.

[JENKINS-23786] Permit the Shell plugin to set a build result as unstable via a return code #2563

[JENKINS-23786] Permit the Shell plugin to set a build result as unstable via a return code #2563

Conversation

stochmial commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stochmial commented Sep 26, 2016

daniel-beck commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stochmial Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Sep 27, 2016

stochmial commented Sep 28, 2016

daniel-beck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stochmial commented Sep 28, 2016

daniel-beck commented Sep 28, 2016

oleg-nenashev commented Oct 2, 2016

stochmial commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Oct 3, 2016

stochmial commented Oct 5, 2016

oleg-nenashev commented Oct 5, 2016

stochmial Sep 27, 2016 •

edited

Loading

oleg-nenashev Oct 3, 2016 •

edited

Loading