search

cirruslabs / cirrus-ci-agent

Agent to execute Cirrus CI tasks
Mozilla Public License 2.0
13 stars 6 forks source link

Improve Piper Closing #216

Closed fkorotkov closed 2 years ago

fkorotkov commented 2 years ago

Follow up to #215 with the idea from #214

I tested changes from #215 with integration tests in Cirrus Cloud and found out that daemon integration tests is failing. With further investigation it appeared that we have a similar test in the agent which is passing but actually should've been failing (test is running for 1 minute and doesn't respect the ctx's timeout).

In order to improve the tests I started passing ctx and properly log such errors. After that I reapplied the idea from #214 and everything seems to be working fine.

edigaryev commented 2 years ago

It seems that in your approach the read FD might still be closed before child process(es) have finished writing to the write FD, which will cause incomplete data, as explained in https://github.com/cirruslabs/cirrus-ci-agent/pull/215.

I've solved this in https://github.com/cirruslabs/cirrus-ci-agent/pull/217, and also ported your piper's context.Context support to get the both of the worlds.

fkorotkov commented 2 years ago

Closing in favor of #217