Skip to content
2 changes: 1 addition & 1 deletion pkg/app/piped/planpreview/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ func (b *builder) cloneHeadCommit(ctx context.Context, headBranch, headCommit st
}

func (b *builder) findTriggerApps(ctx context.Context, repo git.Repo, apps []*model.Application, headCommit string) (triggerApps []*model.Application, failedResults []*model.ApplicationPlanPreviewResult, err error) {
d := trigger.NewDeterminer(repo, headCommit, b.commitGetter, b.logger)
d := trigger.NewDeterminer(repo, headCommit, b.commitGetter, true, b.logger)
for _, app := range apps {
shouldTrigger, err := d.ShouldTrigger(ctx, app)
if err != nil {
Expand Down
53 changes: 43 additions & 10 deletions pkg/app/piped/trigger/determiner.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,19 @@ type Determiner struct {
repo git.Repo
targetCommit string
commitGetter LastTriggeredCommitGetter
logger *zap.Logger
// Flag `ignoreUserConfig` set to `true` will force check changes and use it to determine
// the application deployment should be triggered or not, regardless of the user's configuration.
ignoreUserConfig bool
logger *zap.Logger
}

func NewDeterminer(repo git.Repo, targetCommit string, cg LastTriggeredCommitGetter, logger *zap.Logger) *Determiner {
func NewDeterminer(repo git.Repo, targetCommit string, cg LastTriggeredCommitGetter, ignoreUserConfig bool, logger *zap.Logger) *Determiner {
return &Determiner{
repo: repo,
targetCommit: targetCommit,
commitGetter: cg,
logger: logger.Named("determiner"),
repo: repo,
targetCommit: targetCommit,
commitGetter: cg,
ignoreUserConfig: ignoreUserConfig,
logger: logger.Named("determiner"),
}
}

Expand All @@ -57,6 +61,22 @@ func (d *Determiner) ShouldTrigger(ctx context.Context, app *model.Application)
zap.String("target-commit", d.targetCommit),
)

// TODO: Add logic to determine trigger or not based on other configuration than onCommit.
return d.shouldTriggerOnCommit(ctx, app, logger)
}

func (d *Determiner) shouldTriggerOnCommit(ctx context.Context, app *model.Application, logger *zap.Logger) (bool, error) {
deployConfig, err := loadDeploymentConfiguration(d.repo.GetPath(), app)
if err != nil {
return false, err
}

// Not trigger in case users disable auto trigger deploy on change and the user config is unignorable.
if deployConfig.Trigger.OnCommit.Disabled && !d.ignoreUserConfig {
logger.Info(fmt.Sprintf("auto trigger deployment disabled for application, hash: %s", d.targetCommit))
return false, nil
}

preCommit, err := d.commitGetter.Get(ctx, app.Id)
if err != nil {
logger.Error("failed to get last triggered commit", zap.Error(err))
Expand Down Expand Up @@ -84,12 +104,25 @@ func (d *Determiner) ShouldTrigger(ctx context.Context, app *model.Application)
return false, err
}

deployConfig, err := loadDeploymentConfiguration(d.repo.GetPath(), app)
if err != nil {
return false, err
// TODO: Remove deprecated `deployConfig.TriggerPaths` configuration.
checkingPaths := make([]string, 0, len(deployConfig.Trigger.OnCommit.Paths)+len(deployConfig.TriggerPaths))
// Note: deployConfig.TriggerPaths or deployConfig.Trigger.OnCommit.Paths may contain "" (empty string)
// in case users use one of them without the other, that cause unexpected "" path in the checkingPaths list
// leads to always trigger deployment since "" path matched all other paths.
// The below logic is to remove that "" path from checking path list, will remove after remove the
// deprecated deployConfig.TriggerPaths.
for _, p := range deployConfig.Trigger.OnCommit.Paths {
if p != "" {
checkingPaths = append(checkingPaths, p)
}
}
for _, p := range deployConfig.TriggerPaths {
if p != "" {
checkingPaths = append(checkingPaths, p)
}
}

touched, err := isTouchedByChangedFiles(app.GitPath.Path, deployConfig.TriggerPaths, changedFiles)
touched, err := isTouchedByChangedFiles(app.GitPath.Path, checkingPaths, changedFiles)
if err != nil {
return false, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/app/piped/trigger/trigger.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func (t *Trigger) checkNewCommits(ctx context.Context) error {
if err != nil {
continue
}
d := NewDeterminer(gitRepo, headCommit.Hash, t.commitStore, t.logger)
d := NewDeterminer(gitRepo, headCommit.Hash, t.commitStore, false, t.logger)

for _, app := range apps {
shouldTrigger, err := d.ShouldTrigger(ctx, app)
Expand Down
16 changes: 16 additions & 0 deletions pkg/config/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ type GenericDeploymentSpec struct {
SealedSecrets []SealedSecretMapping `json:"sealedSecrets"`
// List of directories or files where their changes will trigger the deployment.
// Regular expression can be used.
// Deprecated: use Trigger.Paths instead.
Copy link
Member

Choose a reason for hiding this comment

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

The docs in dev should be updated too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in a separated PR 🙆‍♀️

TriggerPaths []string `json:"triggerPaths,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

We should keep both of fields to avoid breaking changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see, then mark it as deprecated and remove it later 👍 Lets me address this

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed by 6591559

Copy link
Member

Choose a reason for hiding this comment

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

better update our docs as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do it by a separated PR 🙏

// The trigger configuration use to determine trigger logic.
Trigger Trigger `json:"trigger"`
// The maximum length of time to execute deployment before giving up.
// Default is 6h.
Timeout Duration `json:"timeout,omitempty" default:"6h"`
Expand All @@ -55,6 +58,19 @@ type DeploymentPlanner struct {
AlwaysUsePipeline bool `json:"alwaysUsePipeline"`
}

type Trigger struct {
// Configuration in case changes on commit are found.
OnCommit OnCommit `json:"onCommit"`
}

type OnCommit struct {
// Control trigger new deployment on Git change or not.
Disabled bool `json:"disabled,omitempty"`
// List of directories or files where their changes will trigger the deployment.
// Regular expression can be used.
Paths []string `json:"paths,omitempty"`
}

func (s *GenericDeploymentSpec) Validate() error {
if s.Pipeline != nil {
for _, stage := range s.Pipeline.Stages {
Expand Down
5 changes: 5 additions & 0 deletions pkg/config/deployment_cloudrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ func TestCloudRunDeploymentConfig(t *testing.T) {
expectedSpec: &CloudRunDeploymentSpec{
GenericDeploymentSpec: GenericDeploymentSpec{
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
},
},
},
Input: CloudRunDeploymentInput{
AutoRollback: true,
Expand Down
5 changes: 5 additions & 0 deletions pkg/config/deployment_ecs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ func TestECSDeploymentConfig(t *testing.T) {
expectedSpec: &ECSDeploymentSpec{
GenericDeploymentSpec: GenericDeploymentSpec{
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
},
},
},
Input: ECSDeploymentInput{
ServiceDefinitionFile: "/path/to/servicedef.yaml",
Expand Down
5 changes: 5 additions & 0 deletions pkg/config/deployment_kubernetes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ func TestKubernetesDeploymentConfig(t *testing.T) {
},
},
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
},
},
},
Input: KubernetesDeploymentInput{
AutoRollback: true,
Expand Down
5 changes: 5 additions & 0 deletions pkg/config/deployment_lambda_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ func TestLambdaDeploymentConfig(t *testing.T) {
expectedSpec: &LambdaDeploymentSpec{
GenericDeploymentSpec: GenericDeploymentSpec{
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
},
},
},
Input: LambdaDeploymentInput{
FunctionManifestFile: "function.yaml",
Expand Down
20 changes: 20 additions & 0 deletions pkg/config/deployment_terraform_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ func TestTerraformDeploymentConfig(t *testing.T) {
expectedSpec: &TerraformDeploymentSpec{
GenericDeploymentSpec: GenericDeploymentSpec{
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
},
},
},
Input: TerraformDeploymentInput{},
},
Expand All @@ -51,6 +56,11 @@ func TestTerraformDeploymentConfig(t *testing.T) {
expectedSpec: &TerraformDeploymentSpec{
GenericDeploymentSpec: GenericDeploymentSpec{
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
},
},
},
Input: TerraformDeploymentInput{
Workspace: "dev",
Expand All @@ -73,6 +83,11 @@ func TestTerraformDeploymentConfig(t *testing.T) {
},
},
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
},
},
},
Input: TerraformDeploymentInput{
Workspace: "dev",
Expand Down Expand Up @@ -108,6 +123,11 @@ func TestTerraformDeploymentConfig(t *testing.T) {
},
},
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
},
},
},
Input: TerraformDeploymentInput{
Workspace: "dev",
Expand Down
46 changes: 46 additions & 0 deletions pkg/config/deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ package config

import (
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/pipe-cd/pipe/pkg/model"
)
Expand Down Expand Up @@ -251,3 +253,47 @@ func TestValidateMentions(t *testing.T) {
})
}
}

func TestGenericTriggerConfiguration(t *testing.T) {
testcases := []struct {
fileName string
expectedKind Kind
expectedAPIVersion string
expectedSpec interface{}
expectedError error
}{
{
fileName: "testdata/application/generic-trigger.yaml",
expectedKind: KindKubernetesApp,
expectedAPIVersion: "pipecd.dev/v1beta1",
expectedSpec: &KubernetesDeploymentSpec{
GenericDeploymentSpec: GenericDeploymentSpec{
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommit{
Disabled: false,
Paths: []string{
"deployment.yaml",
},
},
},
},
Input: KubernetesDeploymentInput{
AutoRollback: true,
},
},
expectedError: nil,
},
}
for _, tc := range testcases {
t.Run(tc.fileName, func(t *testing.T) {
cfg, err := LoadFromYAML(tc.fileName)
require.Equal(t, tc.expectedError, err)
if err == nil {
assert.Equal(t, tc.expectedKind, cfg.Kind)
assert.Equal(t, tc.expectedAPIVersion, cfg.APIVersion)
assert.Equal(t, tc.expectedSpec, cfg.spec)
}
})
}
}
7 changes: 7 additions & 0 deletions pkg/config/testdata/application/generic-trigger.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
trigger:
onCommit:
paths:
- deployment.yaml