-
Bug
-
Resolution: Fixed
-
Major
-
None
Here's a trivial example of a general problem:
pipeline {
agent any
stages {
stage ("Example") {
steps {
// Install dependencies
sh 'npm install'
echo 'Execute build that creates artifacts'
echo 'Execute step that fails to create the expected junit test file'
}
}
}
post {
always {
junit 'reports/**'
echo 'This step and everything after it won't run.'
archive '**'
echo "send email to people with final build status"
}
}
}
I have an always block, but I can't actually guarantee that all steps in that always block will all always attempt to run.
In Scripted Pipeline I could wrap each of these in a try-catch, but I can't do that in Declarative.
I can reorder these steps to put the ones that I know won't fail earlier, but what about the email step? I want that to be last, so it gets the proper build status in the email.
abayer suggested a catchError block. But depending on which step fails, that doesn't get the right behavior either. I want each step in the always to run exactly once no matter the build status. If the second step fails the catchError can't tell which one failed and not to run the first step again.
Logically what I would expect to work:
pipeline {
agent any
stages {
stage ("Example") {
steps {
// Install dependencies
sh 'npm install'
echo 'Execute build that creates artifacts'
echo 'Execute step that fails to create the expected junit test file'
}
}
}
post {
always {
junit 'reports/**'
}
always {
echo 'This step and everything after it won't run.'
}
always {
archive '**
}
always {
echo "send email to people with final build status"
}
}
}
However, I'm not allowed to have more than one always block.
- relates to
-
JENKINS-41519 Post stages should have well defined order
-
- Closed
-
- links to
[JENKINS-41239] Error during always block skips execution of remaining steps
Ryan Campbell
added a comment - The idea of multiple always blocks makes sense, but this definitely feels like an RFE.
Ryan Campbell
added a comment - The idea of multiple always blocks makes sense, but this definitely feels like an RFE. Adding a bit more detail - we capture whole closure blocks of steps at the beginning of the build, when we process the Jenkinsfile's pipeline block, so that we can defer execution of those blocks 'til later in the run. If we wanted to capture each step individually (as would be needed to guarantee each of the steps in a block were executed), we'd have to somehow record each step, its arguments, any nested steps, any steps or functions that get passed as arguments, etc without actually evaluating any of it until later. That's theoretically possible, but unless I'm missing something obvious, glaringly non-trivial. Closures represent far and away the easiest way to defer execution of a chunk of steps until later.
So that brings up the question of multiple always (or any other condition type) blocks - that would also require some fairly significant changes, since post is actually represented as a map behind the scenes - we'd have to change that to be a list of condition name/step block pairs, and then iterate over them in the condition execution order, i.e., first all the always in the order they're specified in the Jenkinsfile, then all the changed, etc...it could be done, but it would take some work and I'm not entirely comfortable with that change in the first place.
Either way - this is definitely not going to be addressed in 1.0.
Sam Van Oort
added a comment - I think this sort of RFE is a good litmus test for when a user needs to shift from declarative to full pipeline, because this level of complexity breaks from the goals of declarative. We want simplicity, straightforward use that applies best practices, and are aiming to cover the 80% case rather than 100% of possibilities.
This is equivalent to doing a try-catch within a finally clause. Even in Java or native pipeline groovy code, this is an iffy practice at best and should be avoided whenever possible.
On these grounds, would suggest we classify this as "won't fix" or maybe even "can't fix."
Sam Van Oort
added a comment - I think this sort of RFE is a good litmus test for when a user needs to shift from declarative to full pipeline, because this level of complexity breaks from the goals of declarative. We want simplicity, straightforward use that applies best practices, and are aiming to cover the 80% case rather than 100% of possibilities.
This is equivalent to doing a try-catch within a finally clause. Even in Java or native pipeline groovy code, this is an iffy practice at best and should be avoided whenever possible.
On these grounds, would suggest we classify this as "won't fix" or maybe even "can't fix." Sam Van Oort
added a comment - Not trying to say it's a ridiculous idea, just that we have to draw a line in the sand somewhere, or declarative will end up just being native pipeline with an extra layer of abstraction added.
Sam Van Oort
added a comment - Not trying to say it's a ridiculous idea, just that we have to draw a line in the sand somewhere, or declarative will end up just being native pipeline with an extra layer of abstraction added. | Attachment | New: Screen Shot 2017-01-25 at 12.32.41 PM.png [ 35617 ] |
abayer
I disagree. This is exactly kind of thing that we do not want going into 1.0 because it will be hard to change later. Also, it will encourage users to build their own hacks to work around this. We should do the right thing now.
I understand (basically) why this problematic but it a major issue. It first appeared in Scripted but could be worked around. I'm not sure how we address it in Declarative, but we absolutely need to.
At core, this is a regression from even Freestyle behavior.
Here's a couple publishers at the end of a Freestyle job:
When this runs, both of these fail (I have no step in that job, there's nothing to work with). But they both run, even tho the archive step fails.
Building remotely on osx_mbp (highmem) in workspace /Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux Archiving artifacts ERROR: No artifacts found that match the file pattern "test/**". Configuration error? ERROR: ‘test/**’ doesn’t match anything, but ‘**’ does. Perhaps that’s what you mean? Build step 'Archive the artifacts' changed build result to FAILURE [htmlpublisher] Archiving HTML reports... [htmlpublisher] Archiving at PROJECT level /Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux to /var/jenkins_home/jobs/full-build-linux/htmlreports/HTML_Report ERROR: Directory '/Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux' exists but failed copying to '/var/jenkins_home/jobs/full-build-linux/htmlreports/HTML_Report'. Finished: FAILURE
My assertion is that the the
{post} block in Declarative is an improved version of publishers.Users are going to take their Freestyle jobs, try to turn them into Declarative. They see the {always} block and treat it like their beloved publishers (which Scripted didn't do well - {try-catch} hell). However, they will eventually encounter a rude gotcha - when one step in their lone {always} block fails the rest don't run.
From a Groovy execution standpoint, I understand why the later steps don't run. There are cases where I would want this behavior, some set of steps that I want to always do at the end of the job but if one of those fails don't do the others. I'm not sure which is the more common, but we need to support both or people will do stupid things trying to do the one we don't support.
Figuring out how to wrap all each method call in the post sections with error handlers sounds like a little slice of hell, likely a bug farm, and also confusing since it doesn't be have at all like script.
Allowing multiple blocks of the same in in {post}
seems the cleanest way to do this. It's logical from a scripting standpoint, but also clear from a declarative standpoint. The downside is we're basically going end up with the second example in the description being the normal way to do things for most users, and that's not great.
On the other hand, really what most users would probably want would be more like:
post {
always {
junit 'reports/**'
echo 'This step and everything after it in this block won't run.'
archive '**
}
always { echo "send email to people with final build status" }
always { echo "send hipchat to people with final build status" }
}
Some parts they want always-always, some they're okay if they do bail.
1.0 is closed for anything but showstopper bugs - so regardless, this isn't going to be addressed in 1.0. And I still disagree on the premise - I don't think there's anything unreasonable about a user discovering that steps in a block after a failed step didn't execute and therefore changing their ordering around in the future. And yes, this is a difference from freestyle behavior - I'm fine with that.
We can look at this after 1.0. IMO, this is more of an RFE than a bug and not a showstopper.
I'm not a big fan of multiple always blocks but that might be the easiest way to let users decide what should happen. Otherwise, if I put several dependent things in an always block and the first one fails I could cause more damage but running the remaining steps.
The other big advantage here is the ability to these steps on the post block of individual stages. You can do archive '**' on the success block of a specific stage (why archive if the build fails) and also capture the final failure if the archive or something else downstream fails.
So this is an interesting question. The current architecture is such that we can't actually break down each step and execute them individually, so we can't guarantee execution of all the steps. Changing this would be nontrivial at best. I don't think there's a plausible chance of addressing this in the 1.0 timeframe, and even post-1.0, no guarantees - I can go into detail on the problems with that if you'd like.