-
Notifications
You must be signed in to change notification settings - Fork 243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIXED JENKINS-53804] Call post conditions as early as possible #291
base: master
Are you sure you want to change the base?
Conversation
Specifically, try to catch errors from directives and execute the post conditions for the build/stage right afterwards, while we're still inside any enclosing directives - i.e., still in top-level `agent` when a top-level `credentials` explodes. Also reworked post handling in general for stages when I realized that an error in a directive before execution of a stage's steps or nested stages would actually result in `post` for the stage not being run at all, which is not exactly ideal.
Note that I've put this in |
...and there are a pile o' test failures, suggesting I screwed something else up. =) |
Yup, something's wrong with how stage errors are logged, at a minimum. I'll pick this up either tomorrow or after Jenkins World Nice next week. |
if (firstErrorSeen == null) { | ||
firstErrorSeen = e | ||
} | ||
// TODO: Ignoring errors in post because we already errored in a directive anyway so this would just mask things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful to track these exceptions as suppressed exceptions rather than just dropping them, or are exceptions after the first one generally useless to the end user? I am imagining something like:
if (firstErrorSeen == null) {
firstErrorSeen = e
} else {
firstErrorSeen.addSuppressed(e)
}
try {
executePostBuild(root)
} catch (Throwable e2) {
firstErrorSeen.addSuppressed(e2)
}
@abayer Any chance you can resolve the merge conflicts here? Should this remain open? |
agent
when a top-levelcredentials
explodes.post
for the stage not being run at all, which is not exactly ideal.