Using backoff Do instead of WaitNext function to avoid potential panic#5388
Using backoff Do instead of WaitNext function to avoid potential panic#5388khanhtc1202 merged 2 commits intomasterfrom
Conversation
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
khanhtc1202
requested review from
Warashi,
ffjlabo,
kentakozuka,
nghialv and
t-kikuc
as code owners
| handler.Terminate() | ||
| finalStatus := s.executeStage(sig, s.deployment.Stages[0]) | ||
| assert.Equal(t, model.StageStatus_STAGE_RUNNING, finalStatus) | ||
| assert.Equal(t, model.StageStatus_STAGE_FAILURE, finalStatus) |
There was a problem hiding this comment.
This test assertion is changed due to the previous version of s.reportStageStatus use WaitNext, which return no error on context cancel, this should be STAGE_FAILURE as mentioned in this change.
ref: https://github.com/pipe-cd/pipecd/blob/master/pkg/app/pipedv1/controller/scheduler.go#L463
| handler.Cancel() | ||
| finalStatus := s.executeStage(sig, s.deployment.Stages[0]) | ||
| assert.Equal(t, model.StageStatus_STAGE_CANCELLED, finalStatus) | ||
| assert.Equal(t, model.StageStatus_STAGE_FAILURE, finalStatus) |
There was a problem hiding this comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5388 +/- ##
==========================================
+ Coverage 25.87% 25.88% +0.01%
==========================================
Files 447 447
Lines 48233 48207 -26
==========================================
- Hits 12478 12477 -1
+ Misses 34784 34759 -25
Partials 971 971 ☔ View full report in Codecov by Sentry. |
|
📝 For my understanding It happened the bug related to the |
3 participants
What this PR does:
Using backoff.Do in pipedv1 codebase instead of backoff.WaitNext
Why we need it:
In the backoff package, we have two functions that handle retry logic: WaitNext and Do. The main difference between them (aside from the interface) is that WaitNext does not raise an error in case the context is canceled/stopped, while Do does. So if using WaitNext and the returned value are used as a check for further logic, it may cause panic due to no error returned while the value is nil either.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: