pipe-cd / pipecd

The One CD for All {applications, platforms, operations}
https://pipecd.dev
Apache License 2.0
1.09k stars 153 forks source link

Use chan only to notify command is done #5244

Closed Warashi closed 1 month ago

Warashi commented 1 month ago

What this PR does / why we need it:

The original IsRunning method has a bug when the command exits with an error. After the command is done with an error, IsRunning returns true once if GracefulStop is not called. This comes from using chan error to both pass the result and notify the finish. This PR splits these things to chan struct{} and atomic.Pointer[error].

Which issue(s) this PR fixes:

The flaky test TestGracefulStopCommand.

Fixes #4630

Does this PR introduce a user-facing change?:

Warashi commented 1 month ago

I'll run the test 5 times and make this PR ready for review if all tests are passed.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 23.96%. Comparing base (c1a06aa) to head (d10876a). Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
pkg/app/launcher/cmd/launcher/binary.go 84.61% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5244 +/- ## ========================================== + Coverage 23.95% 23.96% +0.01% ========================================== Files 437 437 Lines 46997 47005 +8 ========================================== + Hits 11258 11267 +9 Misses 34835 34835 + Partials 904 903 -1 ``` | [Flag](https://app.codecov.io/gh/pipe-cd/pipecd/pull/5244/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pipe-cd) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/pipe-cd/pipecd/pull/5244/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pipe-cd) | `23.96% <84.61%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pipe-cd#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Warashi commented 1 month ago

I realized there are no test for the result of GracefulStop, so I added that.

Warashi commented 1 month ago

I'll run the test 5 times and make this PR ready for review if all tests are passed.

The tests are passed 5 times.

https://github.com/pipe-cd/pipecd/actions/runs/11100844567/job/30838021571

スクリーンショット 2024-09-30 16 07 47