Closed tragiclifestories closed 3 years ago
The one peculiarity I think is worth calling out with this approach is that the ConsoleReady
lifecycle hooks might get called with a stopped console now. We only use that hook to log out some console metadata at the moment though so not going to cause any problems with anu/utopia
I had this problem last time - basically there are no existing tests around attachment at all which is a pity so it's a non-trivial additional piece of work, but I might bash my head against it for a while ...
Just to reassure you, I did get a very useful review from Dyson and a thumbs up, just all in slack DMs.
Honest 😉
The current behaviour for attachment is to wait in a loop for console pods to become ready, and then try to create the attachment. This leads to two potential problems if the command passed to the console is very short lived:
In both of these cases, theatre will error out, but it is not necessarily an error: the command may merely have completed successfully. This matters a great deal for Dispatcher console creation, since if (to take a common case) we successfully run database migrations for some app too quick for theatre, then we do not want to fail the pipeline.
The solution in this commit is to simply remove the check for a stopped pod (1) in the wait-until-ready loop altogether (it only matters for attachment) and then handle the attachment error (2) such that we write the logs to the attachment out stream and exit with the appropriate status (so we're still red if the console failed ... )