Fix to directly use WAIT_APPROVAL timeout #1455
Conversation
|
Code coverage for golang is |
| ) | ||
| defer ticker.Stop() | ||
|
|
||
| timer := time.NewTimer(time.Duration(e.StageConfig.WaitApprovalStageOptions.Timeout) * time.Second) |
There was a problem hiding this comment.
just to confirm, the bug caused by this deleted time.Second annotation, is that right? 🤔
(Timeout default value currently set as 6h already ref: config/deployment)
There was a problem hiding this comment.
I think so. It always overflows and looks like being time.NewTimer(-315919h31m53.884942336s) at runtime.
https://play.golang.org/p/WhPnXvUp7Ci
There was a problem hiding this comment.
I see, I met this issue before 😂 ( time.Minute * time.Minute overflow in my case )
There was a problem hiding this comment.
And one more minor question, it looks like the default value is of type Duration already, is that time.Duration(...) is necessary for the new code? 👀
ref: https://github.com/pipe-cd/pipe/pull/1455/files/a1af59eadce3ebcda7efd88f6a151f843d9026a2#diff-02a6821aa5f1210155af9610f8bb1b7683ed309d3ad56a533f0505d12ae6ffd6R57
There was a problem hiding this comment.
Look closely. The type of Duration used by Config is our own Type for making it easier to parse. So we have to parse it to time.Duration when using 😄
There was a problem hiding this comment.
Yeh, but it's a defined type based on time.Duration (ref: config/duration.go ). Still looks weird to me, but never mind if it works that way 😅
There was a problem hiding this comment.
I mean
e.StageConfig.WaitApprovalStageOptions.Timeout.Duration()should be fine 😂
ref: https://github.com/pipe-cd/pipe/blob/master/pkg/config/duration.go#L25
There was a problem hiding this comment.
Ah, you're right, let me fix.
nakabonne
left a comment
nakabonne
left a comment
There was a problem hiding this comment.
I think so. It always overflows and looks like being time.NewTimer(-315919h31m53.884942336s) at runtime.
https://play.golang.org/p/WhPnXvUp7Ci
|
/lgtm |
|
Code coverage for golang is |
|
/lgtm |
|
Nice. Thank you. |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
We just need to use it directly. It could overflow.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: