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 @@ -307,7 +307,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)
for _, app := range apps {
shouldTrigger, err := d.ShouldTrigger(ctx, app)
shouldTrigger, err := d.ShouldTrigger(ctx, app, true)
if err != nil {
// We only need the environment name
// so the returned error can be ignorable.
Expand Down
41 changes: 36 additions & 5 deletions pkg/app/piped/trigger/determiner.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,31 @@ func NewDeterminer(repo git.Repo, targetCommit string, cg LastTriggeredCommitGet
}

// ShouldTrigger decides whether a given application should be triggered or not.
func (d *Determiner) ShouldTrigger(ctx context.Context, app *model.Application) (bool, error) {
// 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.
func (d *Determiner) ShouldTrigger(ctx context.Context, app *model.Application, ignoreUserConfig bool) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

How about moving ignoreUserConfig to Determiner while initializing it? In that way, we can keep this function simple as it is.

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, lets me adopt that 🙆‍♀️

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 8dffc8d

logger := d.logger.With(
zap.String("app", app.Name),
zap.String("app-id", app.Id),
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, ignoreUserConfig, logger)
}

func (d *Determiner) shouldTriggerOnCommit(ctx context.Context, app *model.Application, ignoreUserConfig bool, 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.Disable && !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 +102,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 @@ -230,7 +230,7 @@ func (t *Trigger) checkNewCommits(ctx context.Context) error {
d := NewDeterminer(gitRepo, headCommit.Hash, t.commitStore, t.logger)

for _, app := range apps {
shouldTrigger, err := d.ShouldTrigger(ctx, app)
shouldTrigger, err := d.ShouldTrigger(ctx, app, false)
if err != nil {
t.logger.Error(fmt.Sprintf("failed to check application: %s", app.Id), zap.Error(err))
continue
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 OnCommitConfig `json:"onCommit"`
}

type OnCommitConfig struct {
// Control trigger new deployment on Git change or not.
Disable bool `json:"disable,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

"Disabled" because we are using the same format at other places.

Copy link
Member

Choose a reason for hiding this comment

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

nit: Do we need the Config suffix?

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 b9bc4cc

// 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: OnCommitConfig{
Disable: 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: OnCommitConfig{
Disable: 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: OnCommitConfig{
Disable: 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: OnCommitConfig{
Disable: 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: OnCommitConfig{
Disable: 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: OnCommitConfig{
Disable: false,
},
},
},
Input: TerraformDeploymentInput{
Workspace: "dev",
Expand All @@ -73,6 +83,11 @@ func TestTerraformDeploymentConfig(t *testing.T) {
},
},
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnCommit: OnCommitConfig{
Disable: 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: OnCommitConfig{
Disable: 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: OnCommitConfig{
Disable: 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