Properly surface errors from build commands #1168
Conversation
charliemirabile
force-pushed
the
build_exit
branch
from
a41b57a to
3d3e8f4
Compare
|
Not sure if this needs unit tests / how to test if the maintainers think it does. |
|
I think an integration test could be handy. Surprisingly, we test build command already. Probably https://github.com/containers/podman-compose/blob/main/tests/integration/build_fail/test_podman_compose_build_fail.py test could be enhanced to test this situation as well in separate test. |
|
Other than this, this is great find, thanks for submitting a PR. |
charliemirabile
force-pushed
the
build_exit
branch
2 times, most recently
from
3ed32dc to
e041d82
Compare
|
Ok, I have added an integration test. I fear that it isn't perfect, because while it will definitely pass 100% with the changes in this PR, it is possible that it could pass erroneously on the current main branch because it is basically relying on a race condition to surface the original bug (the image that fails needs to finish first and then the successful one finishes second so that the failing status is erased by the new status code of 0). I can't really think of any better way to test this - you could make the sleep command in the successful image arbitrarily long in order to increase the chances of it winning the race, but you can't ever get it 100% because the scheduler could just choose not to run the failing build, and the longer you make it, the longer the test takes. the 0.5s sleep is plenty on my maching to have a 100% failure rate on the main branch, but I have a reasonably fast computer. |
charliemirabile
force-pushed
the
build_exit
branch
2 times, most recently
from
04196dd to
6ad344e
Compare
|
I just reread you message more carefully
If you would prefer that I merge the new test I created in a folder |
the commit 38b13a3 ("Use asyncio for subprocess calls") broke the way exit codes are reported from the podman compose build command. The tasks are awaited as they finish which means that if a later build finishes sucessfully after a failing build, it overwrites status. Previously the `parse_return_code` function would skip updating the status if the new return code was zero, but in removing it, this logic was not carried forward. Fixes: 38b13a3 ("Use asyncio for subprocess calls") Signed-off-by: charliemirabile <[email protected]>
charliemirabile
force-pushed
the
build_exit
branch
from
6ad344e to
2e7d83f
Compare
|
Thanks for writing a test, looks great. Having test in separate directory or not is something not worth arguing, especially since the rest of tests don't have consistency in this regard. Regarding sleep, in this case it's the best way to solve the issue even if we have a race condition in the test. The integration test suite is slow anyway, so a second here or there does not matter much. If we start to care about integration test performance, better avenue would be to parallelize it. |
fixes #476