Add timeout for wait approval #1386
Conversation
|
Code coverage for golang is
|
pkg/config/deployment.go
Outdated
| Approvers []string `json:"approvers"` | ||
| } | ||
|
|
||
| var defaultWaitApprovalTimeout int64 = 600 |
There was a problem hiding this comment.
What should be the default value for timeout?
pkg/config/deployment.go
Outdated
|
|
||
| // WaitStageOptions contains all configurable values for a WAIT_APPROVAL stage. | ||
| type WaitApprovalStageOptions struct { | ||
| // number of seconds to timeout wait approval |
There was a problem hiding this comment.
nit: "The maximum length of time to wait before giving up."
pkg/config/deployment.go
Outdated
| // WaitStageOptions contains all configurable values for a WAIT_APPROVAL stage. | ||
| type WaitApprovalStageOptions struct { | ||
| // number of seconds to timeout wait approval | ||
| Timeout int64 `json:"timeout"` |
There was a problem hiding this comment.
Could you please change this to use Duration instead of int64?
https://github.com/pipe-cd/pipe/blob/master/pkg/config/duration.go#L23
pkg/config/deployment.go
Outdated
| Approvers []string `json:"approvers"` | ||
| } | ||
|
|
||
| var defaultWaitApprovalTimeout int64 = 600 |
There was a problem hiding this comment.
The default value could be 6 * time.Hour
| return model.StageStatus_STAGE_FAILURE | ||
| } | ||
| case <-timer.C: | ||
| e.LogPersister.Info("Timeout wait approval") |
There was a problem hiding this comment.
w.LogPersister.Errorf("Timed out %v", e.StageConfig.WaitApprovalStageOptions.Timeout)
|
@sanposhiho Many thanks for your great effort. |
|
@nghialv Thanks you for your reviews, fixed them :D |
|
Code coverage for golang is
|
pkg/config/deployment.go
Outdated
| Approvers []string `json:"approvers"` | ||
| } | ||
|
|
||
| var defaultWaitApprovalTimeout = Duration(6 * time.Hour) |
There was a problem hiding this comment.
nit, should move to the top of this file.
There was a problem hiding this comment.
For this I'm still debating. I'm trying to figure out where to define the default values.
Currently most of all default values are populated at the pakcages where they are used. We have not debated this point until now, but I followed that rule because this current design can hide the way how to use each configuration field and leave it to each package that uses them.
But on the dark side, it can't guarantee that all field is populated when it parsed. Happy to hear your thoughts.
There was a problem hiding this comment.
Currently most of all default values are populated at the packages where they are used.
Got it, I think I'm okay with this convention 👍 What I mentioned here is, should we move this default value to the top of this file and make it const, currently it's been used before declared make it hard to read.
For the point of "where to define the default value", I think we should debate and address it by another PR. 😁
There was a problem hiding this comment.
should we move this default value to the top of this file and make it const,
I think we should debate and address it by another PR.
I am fine with these things. To be honest I was thinking about where to define and how to configure the default value of our config. And I think all of them should be defined in the config package (as public consts) and loaded when the configuration is parsed. So the comment about the default value is the same in that package too. The docs refer to them too.
It would be nice if we can define the default value by go tag (as JSON tag). There are some libs for doing that thing but still not found a good enough one.
Let me make a PR for this issue.
There was a problem hiding this comment.
It would be nice if we can define the default value by go tag (as JSON tag).
👍 👍 👍
There was a problem hiding this comment.
Okay, let's debate about all of them on another PR.
should we move this default value to the top of this file and make it const,
@sanposhiho Could you address it if no objection to that? 😄
There was a problem hiding this comment.
I've just caught up on this conversation. OK, move the default value to the top 👍
| Name: model.StageWaitApproval, | ||
| WaitApprovalStageOptions: &WaitApprovalStageOptions{ | ||
| Approvers: []string{"foo", "bar"}, | ||
| Timeout: defaultWaitApprovalTimeout, // set default timeout value when nothing sets on config |
There was a problem hiding this comment.
nit
WaitApprovalStageOptions: &WaitApprovalStageOptions{
Approvers: []string{"foo", "bar"},
// Use defaultWaitApprovalTimeout on unset timeout value for WaitApprovalStage.
Timeout: defaultWaitApprovalTimeout,or remove this comment and place it to implement source at L137
|
Code coverage for golang is
|
|
Thank you. |
|
thank you |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1371
Does this PR introduce a user-facing change?: