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

Unique exception for input step abort

    XMLWordPrintable

Details

    Description

      When using input in conjunction with milestone it is impossible to detect the difference between the user choosing to "abort" (e.g. say "no" to the prompt) and the build being abandoned because it was superseded. Both cases throw FlowInterruptedException

      This makes it impossible to take meaningful actions upon the user choosing to abort that you would not want to take if the input goes un-answered.

      One possible solution would be for the input-invoked Abort to throw a unique exception type which extends FlowInterruptedException.

      Attachments

        Issue Links

          Activity

            jglick Jesse Glick added a comment -

            You can already check for org.jenkinsci.plugins.workflow.support.steps.input.Rejection as one of the causes but this is not @Whitelisted or otherwise convenient.

            jglick Jesse Glick added a comment - You can already check for org.jenkinsci.plugins.workflow.support.steps.input.Rejection as one of the causes but this is not @Whitelisted or otherwise convenient.
            abayer Andrew Bayer added a comment -

            So we can't switch to a different exception here - too many places expect/handle FlowInterruptedException for all abort cases, including input, and FlowInterruptedException is a final class, so we can't extend it. As Jesse mentioned, looking for Rejection in the causes is the best approach - I'll get a PR up to whitelist FlowInterruptedException#getCauses() and post an example here once it's up.

            abayer Andrew Bayer added a comment - So we can't switch to a different exception here - too many places expect/handle FlowInterruptedException for all abort cases, including input , and FlowInterruptedException is a final class, so we can't extend it. As Jesse mentioned, looking for Rejection in the causes is the best approach - I'll get a PR up to whitelist FlowInterruptedException#getCauses() and post an example here once it's up.
            abayer Andrew Bayer added a comment -

            Added whitelisting in https://github.com/jenkinsci/workflow-step-api-plugin/pull/32 - once that's merged and released, you can do something like this:

            try {
              ...
            } catch (FlowInterruptedException e) {
              if (e.getCauses().any { it instanceof org.jenkinsci.plugins.workflow.support.steps.input.Rejection }) {
              echo "This build was aborted due to user input."
            }
            
            abayer Andrew Bayer added a comment - Added whitelisting in https://github.com/jenkinsci/workflow-step-api-plugin/pull/32 - once that's merged and released, you can do something like this: try { ... } catch (FlowInterruptedException e) { if (e.getCauses().any { it instanceof org.jenkinsci.plugins.workflow.support.steps.input.Rejection }) { echo "This build was aborted due to user input." }
            crussell52 Chris Russell added a comment -

            Implemented suggestions from here and other issue(s) related to challenges accurately detecting abort conditions that works reasonably well and covers detection of input rejection.  Details posted here, if it is useful to anybody else:

            https://issues.jenkins-ci.org/browse/JENKINS-28822?focusedCommentId=346492&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-346492

             

            crussell52 Chris Russell added a comment - Implemented suggestions from here and other issue(s) related to challenges accurately detecting abort conditions that works reasonably well and covers detection of input rejection.  Details posted here, if it is useful to anybody else: https://issues.jenkins-ci.org/browse/JENKINS-28822?focusedCommentId=346492&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-346492  
            hogarthj James Hogarth added a comment -

            abayer  "Lemme think on JENKINS-41272 and leave this sitting here for the moment."

            Have you thought about this? I've got a developer who wants to access the causes for an abort but of course getCauses() on FlowInteruptedException isn't whitelisted yet by default...

             

            hogarthj James Hogarth added a comment - abayer   "Lemme think on JENKINS-41272 and leave this sitting here for the moment." Have you thought about this? I've got a developer who wants to access the causes for an abort but of course getCauses() on FlowInteruptedException isn't whitelisted yet by default...  
            dnusbaum Devin Nusbaum added a comment -

            Given JENKINS-41272 went with the approach of exposing the causes as JSON to avoid having to whitelist all causes as well, I think it would make sense to take the same approach here. If anyone still wants to work on it, I would file a new PR to add new methods to FlowInterruptedException similar to the approach in https://github.com/jenkinsci/workflow-support-plugin/pull/78.

            dnusbaum Devin Nusbaum added a comment - Given JENKINS-41272 went with the approach of exposing the causes as JSON to avoid having to whitelist all causes as well, I think it would make sense to take the same approach here. If anyone still wants to work on it, I would file a new PR to add new methods to FlowInterruptedException similar to the approach in https://github.com/jenkinsci/workflow-support-plugin/pull/78 .
            jglick Jesse Glick added a comment -

            exposing the causes as JSON to avoid having to whitelist all causes as well

            That is also in line with what I proposed as an api step.

            jglick Jesse Glick added a comment - exposing the causes as JSON to avoid having to whitelist all causes as well That is also in line with what I proposed as an api step .
            jeffpearce Jeff Pearce added a comment -

            I'm willing to try dnusbaum's suggested fix (exposing the causes as JSON) if that still seems like the best approach

            jeffpearce Jeff Pearce added a comment - I'm willing to try dnusbaum 's suggested fix (exposing the causes as JSON) if that still seems like the best approach

            People

              Unassigned Unassigned
              crussell52 Chris Russell
              Votes:
              4 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

                Created:
                Updated: