Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-57826

catchError(buildResult: hudson.model.Result.SUCCESS, stageResult: hudson.model.Result.FAILURE) doesn't set stage to FAILURE

    Details

    • Similar Issues:

      Description

      If I have a stage with a step in it:

                               ...
                               catchError(buildResult: hudson.model.Result.SUCCESS,
                                         message: 'RPM build failed, but allowing job to continue',
                                         stageResult: hudson.model.Result.FAILURE) {
                                  sh label: env.STAGE_NAME,
                                     script: 'exit 2'
                              }
      

      With a post block for the stage:

                          post {
                              always {
                                  archiveArtifacts artifacts: 'artifacts/**'
                              }
                              success {
                                  println "${env.STAGE_NAME}: SUCCESS"
                              }
                              unstable {
                                  println "${env.STAGE_NAME}: UNSTABLE"
                              }
                              failure {
                                  println "${env.STAGE_NAME}: FAILED"
                              }
                          }
      

       I actually get the:

      My Test Stage: SUCCESS
      

      telling me that the stage was a SUCCESS, not a FAILURE like the:

      catchError(..., stageResult: hudson.model.Result.FAILURE) is supposed to be making it.

      In both the stage view and the Blue Ocean view, it does show as a failed stage. The post processing block just doesn't see it that way.

      As an aside, is there a variable set for the stage that I could use in my println above so that I could just do a single println in the always block rather than having to repeat it through the success, unstable and failure blocks?

        Attachments

          Issue Links

            Activity

            Hide
            dnusbaum Devin Nusbaum added a comment -

            Yeah, we need to change how post conditions are handed in Declarative to make this work if you are using catchError with a buildResult and stageResult that do not match. My first thought would be to add a new overload of BuildCondition.getCombinedResult that takes two new FlowNode parameters, one for the start of the stage or parallel block the post is associated with, and one for the end, and then use them to call StatusAndTiming.findWarningBetween (maybe move that method to workflow-api?), and then update all of the callers in Declarative to use the new version of BuildCondition.getCombinedResult. CC Andrew Bayer.

            Show
            dnusbaum Devin Nusbaum added a comment - Yeah, we need to change how post conditions are handed in Declarative to make this work if you are using catchError with a buildResult and stageResult that do not match. My first thought would be to add a new overload of BuildCondition.getCombinedResult that takes two new FlowNode parameters, one for the start of the stage or parallel block the post is associated with, and one for the end, and then use them to call StatusAndTiming.findWarningBetween (maybe move that method to workflow-api?), and then update all of the callers in Declarative to use the new version of BuildCondition.getCombinedResult . CC Andrew Bayer .
            Hide
            dnusbaum Devin Nusbaum added a comment -

            BTW as a temporary workaround, if you are ok with marking the build and stage result as unstable, you can just use warnError instead. Also note that my proposed fix for JENKINS-57537 is going to break the way you are using buildResult: hudson.model.Result.SUCCESS, so I wouldn't go too much further with custom result for catchError in Declarative until that is fixed and you can go back to writing buildResult: 'SUCCESS'.

            Show
            dnusbaum Devin Nusbaum added a comment - BTW as a temporary workaround, if you are ok with marking the build and stage result as unstable, you can just use warnError instead. Also note that my proposed fix for  JENKINS-57537 is going to break the way you are using buildResult: hudson.model.Result.SUCCESS , so I wouldn't go too much further with custom result for catchError in Declarative until that is fixed and you can go back to writing buildResult: 'SUCCESS' .
            Hide
            brianjmurrell Brian J Murrell added a comment -

            Isn't the post for a stage supposed to be acting on the result of that stage?  I'm wondering how/why any kind of combined result or overall job result is even a factor in assessing which post blocks to run for the stage when catchError is successfully setting the result for that stage.

            as a temporary workaround, if you are ok with marking the build and stage result as unstable

            That won't work unfortunately. UNSTABLE in our workflow specifically prevents landing since it's the result of having failed junit test results. So we can't live with marking the overall build result as UNSTABLE when only stages that are not supposed to block landing fail.

            break the way you are using buildResult: hudson.model.Result.SUCCESS

            That's fine.  I am using it completely understanding that it's only temporary while I wait for JENKINS-57537 to be available.

            Show
            brianjmurrell Brian J Murrell added a comment - Isn't the post for a stage supposed to be acting on the result of that stage ?  I'm wondering how/why any kind of combined result or overall job result is even a factor in assessing which post blocks to run for the stage when catchError is successfully setting the result for that stage. as a temporary workaround, if you are ok with marking the build and stage result as unstable That won't work unfortunately. UNSTABLE in our workflow specifically prevents landing since it's the result of having failed junit test results. So we can't live with marking the overall build result as UNSTABLE when only stages that are not supposed to block landing fail. break the way you are using buildResult: hudson.model.Result.SUCCESS That's fine.  I am using it completely understanding that it's only temporary while I wait for  JENKINS-57537 to be available.
            Hide
            dnusbaum Devin Nusbaum added a comment -

            Isn't the post for a stage supposed to be acting on the result of that stage?  I'm wondering how/why any kind of combined result or overall job result is even a factor in assessing which post blocks to run for the stage when catchError is successfully setting the result for that stage.

            Before this change there was no such thing as a stage result (and technically with recent changes there still isn't, we just have step results that are aggregated at the stage level). There was no way to make existing code that deals with results use the new step result API transparently, so we still need to be able to handle anything setting the overall build result or throwing an exception (if the current build result is UNSTABLE, but the stage threw an exception (which doesn't change the build result directly), we only want to run the FAILURE post condition, not the UNSTABLE one, hence BuildCondition.combineResults), unless we make post blocks only work for catchError, unstable, and warnError, and the few other steps that are using the new API, but that would break many (maybe most?) existing use cases, especially around things like currentBuild.result, which is and always will be oblivious to the new step-level API.

            Show
            dnusbaum Devin Nusbaum added a comment - Isn't the  post  for a stage supposed to be acting on the result of  that stage ?  I'm wondering how/why any kind of combined result or overall job result is even a factor in assessing which  post  blocks to run for the stage when  catchError  is successfully setting the result for that stage. Before this change there was no such thing as a stage result (and technically with recent changes there still isn't, we just have step results that are aggregated at the stage level). There was no way to make existing code that deals with results use the new step result API transparently, so we still need to be able to handle anything setting the overall build result or throwing an exception (if the current build result is UNSTABLE, but the stage threw an exception (which doesn't change the build result directly), we only want to run the FAILURE post condition, not the UNSTABLE one, hence BuildCondition.combineResults ), unless we make post blocks only work for catchError , unstable , and warnError , and the few other steps that are using the new API, but that would break many (maybe most?) existing use cases, especially around things like currentBuild.result , which is and always will be oblivious to the new step-level API.
            Hide
            abayer Andrew Bayer added a comment -

            Yeah, we're going to need to add per-stage status support to Declarative post for sure.

            Show
            abayer Andrew Bayer added a comment - Yeah, we're going to need to add per-stage status support to Declarative post for sure.
            Hide
            brianjmurrell Brian J Murrell added a comment -

            (At the risk of preaching to the choir, but also not wanting to leave it unsaid) regardless of how it's achieved, having the post stages react and act according to the stage status set by catchError (and friends) is paramount because (again, to be obvious, but explicit,) having the post stages not act according to status of the stage I think is just going to add even more confusion to this already confusing big can of worms.

            Any estimate on the size/effort of fixing the post processing, since I think for many cases, the (excellent!, BTW) advancements made in the Pipeline Basic Steps plugin really get nullified by not being able to post based on the stage status that can now be set.  I wouldn't imagine there are very many pipelines that don't do at least a minimal amount of post based on the result of the stage.  But maybe I am wrong about that guesstimate.

            Show
            brianjmurrell Brian J Murrell added a comment - (At the risk of preaching to the choir, but also not wanting to leave it unsaid) regardless of how it's achieved, having the post stages react and act according to the stage status set by catchError (and friends) is paramount because (again, to be obvious, but explicit,) having the post stages not act according to status of the stage I think is just going to add even more confusion to this already confusing big can of worms. Any estimate on the size/effort of fixing the post processing, since I think for many cases, the (excellent!, BTW) advancements made in the Pipeline Basic Steps plugin really get nullified by not being able to post based on the stage status that can now be set.  I wouldn't imagine there are very many pipelines that don't do at least a minimal amount of post based on the result of the stage.  But maybe I am wrong about that guesstimate.
            Hide
            abayer Andrew Bayer added a comment -
            Show
            abayer Andrew Bayer added a comment - PR up! https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/330 - this should do the trick.
            Hide
            abayer Andrew Bayer added a comment -

            Woo, merged - it'll be in 1.3.9, which I should release soon.

            Show
            abayer Andrew Bayer added a comment - Woo, merged - it'll be in 1.3.9, which I should release soon.

              People

              • Assignee:
                abayer Andrew Bayer
                Reporter:
                brianjmurrell Brian J Murrell
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: