Skip to content

Add timeout for wait approval #1386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pipecd-bot merged 7 commits into from
Jan 12, 2021
Merged

Add timeout for wait approval #1386

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fc698d2
Add config for set timeout on wait approval
sanposhiho Jan 8, 2021
a391566
Return failure when timeout
sanposhiho Jan 8, 2021
c5f82fd
Fix comment
sanposhiho Jan 11, 2021
b733c0e
Use Duration instead of int64
sanposhiho Jan 11, 2021
cc58dc2
Fix error message
sanposhiho Jan 11, 2021
7790acc
Move default value definition
sanposhiho Jan 12, 2021
a07728e
Move comment
sanposhiho Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/app/piped/executor/waitapproval/waitapproval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus {
)
defer ticker.Stop()

timer := time.NewTimer(time.Duration(e.StageConfig.WaitApprovalStageOptions.Timeout) * time.Second)

e.LogPersister.Info("Waiting for an approval...")
for {
select {
Expand All @@ -73,6 +75,9 @@ func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus {
default:
return model.StageStatus_STAGE_FAILURE
}
case <-timer.C:
e.LogPersister.Errorf("Timed out %v", e.StageConfig.WaitApprovalStageOptions.Timeout)
return model.StageStatus_STAGE_FAILURE
}
}
}
Expand Down
12 changes: 10 additions & 2 deletions pkg/config/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package config
import (
"encoding/json"
"fmt"
"time"

"github.com/pipe-cd/pipe/pkg/model"
)
Expand Down Expand Up @@ -66,13 +67,13 @@ type DeploymentCommitMatcher struct {

// DeploymentPipeline represents the way to deploy the application.
// The pipeline is triggered by changes in any of the following objects:
// - Target PodSpec (Target can be Deployment, DaemonSet, StatefullSet)
// - Target PodSpec (Target can be Deployment, DaemonSet, StatefulSet)
// - ConfigMaps, Secrets that are mounted as volumes or envs in the deployment.
type DeploymentPipeline struct {
Stages []PipelineStage `json:"stages"`
}

// PiplineStage represents a single stage of a pipeline.
// PipelineStage represents a single stage of a pipeline.
// This is used as a generic struct for all stage type.
type PipelineStage struct {
Id string
Expand Down Expand Up @@ -133,6 +134,9 @@ func (s *PipelineStage) UnmarshalJSON(data []byte) error {
if len(gs.With) > 0 {
err = json.Unmarshal(gs.With, s.WaitApprovalStageOptions)
}
if s.WaitApprovalStageOptions.Timeout <= 0 {
s.WaitApprovalStageOptions.Timeout = defaultWaitApprovalTimeout
}
case model.StageAnalysis:
s.AnalysisStageOptions = &AnalysisStageOptions{}
if len(gs.With) > 0 {
Expand Down Expand Up @@ -220,9 +224,13 @@ type WaitStageOptions struct {

// WaitStageOptions contains all configurable values for a WAIT_APPROVAL stage.
type WaitApprovalStageOptions struct {
// The maximum length of time to wait before giving up.
Timeout Duration `json:"timeout"`
Approvers []string `json:"approvers"`
}

var defaultWaitApprovalTimeout = Duration(6 * time.Hour)

@khanhtc1202 khanhtc1202 Jan 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, should move to the top of this file.

@nakabonne nakabonne Jan 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@khanhtc1202 khanhtc1202 Jan 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😁

@nghialv nghialv Jan 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@khanhtc1202 khanhtc1202 Jan 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we can define the default value by go tag (as JSON tag).

👍 👍 👍

@nakabonne nakabonne Jan 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😄

@sanposhiho sanposhiho Jan 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just caught up on this conversation. OK, move the default value to the top 👍


// AnalysisStageOptions contains all configurable values for a K8S_ANALYSIS stage.
type AnalysisStageOptions struct {
// How long the analysis process should be executed.
Expand Down
1 change: 1 addition & 0 deletions pkg/config/deployment_terraform_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func TestTerraformDeploymentConfig(t *testing.T) {
Name: model.StageWaitApproval,
WaitApprovalStageOptions: &WaitApprovalStageOptions{
Approvers: []string{"foo", "bar"},
Timeout: defaultWaitApprovalTimeout, // set default timeout value when nothing sets on config

@khanhtc1202 khanhtc1202 Jan 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
},
{
Expand Down