Server logs should be written when integration tests fail #4804
Conversation
niloc132
added
build
docker
NoDocumentationNeeded
NoReleaseNotesNeeded
| // Specify that makeImage is finalized by buildAndRun - that is, in this configuration buildAndRun | ||
| // must run after makeImage finishes |
There was a problem hiding this comment.
Your comment buildAndRun must run after makeImage is a bit at odds with the typical gradle terminology. I'm digging in now; I don't know if finalizedBy is the technically correct solution here (it may very well be...).
https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:finalizer_tasks
Finalizer tasks are useful in situations where the build creates a resource that has to be cleaned up regardless of the build failing or succeeding.
https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:ordering_tasks
When you use the “must run after” ordering rule you specify that taskB must always run after taskA, whenever both taskA and taskB will be run. This is expressed as taskB.mustRunAfter(taskA).
There was a problem hiding this comment.
mustRunAfter/shouldRunAfter only sets an ordering requirement, but does not mandate that the earlier or later task run at all, they can run or not run independently.
On the other hand, b.dependsOn(a) means that if b is requested to run, a must run first and must be successful, whereas a.finalizedBy(b) means that if a will run, b must run afterwards (regardless of whether or not a is successful).
| onlyIf { buildAndRun.get().state.failure != null } | ||
| doLast { | ||
| if (buildAndRun.get().state.failure != null) { | ||
| throw new GradleException('Docker task failed, see earlier task failures for details') |
There was a problem hiding this comment.
I might add a comment this isn't an illegal state, we just want to preserve / propagate the failure, correct? I wonder if we should have a separate task that propagates this failure, instead of attaching it to the Sync task? (which may have specific semantics attached to sync-related failures.)
There was a problem hiding this comment.
Correct - not an illegal state (and the choice of GradleException is I thought intended to convey that), but just a failure. If this task did not throw an exception (or otherwise fail), it wouldn't be clear to other tasks that they should/shouldn't continue (e.g. "if it failed, dont continue to build downstream artifacts" vs "if it failed, make sure we dump the server's logs").
There was a problem hiding this comment.
Note also that a new task with actions (i.e. "fail if that other thing failed") but no inputs or outputs can never be up to date. I'm not aware of Sync specific errors, but can add a comment here to call out what is going on, besides the wall of text written at the top of this section.
niloc132
dismissed stale reviews from devinrsmith and jcferretti
via
a406a21
Fixes #4766