From 06c5f5dcf4d66d139dcdf6907c1ee075da90f89b Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 22 Apr 2024 20:53:56 +0900 Subject: [PATCH 01/46] rename func to distinguish the trigger of skip stage Signed-off-by: t-kikuc --- pkg/app/piped/executor/analysis/analysis.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/executor/analysis/analysis.go b/pkg/app/piped/executor/analysis/analysis.go index 9dedaa34ef..328ad5a249 100644 --- a/pkg/app/piped/executor/analysis/analysis.go +++ b/pkg/app/piped/executor/analysis/analysis.go @@ -112,7 +112,7 @@ func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { for { select { case <-ticker.C: - if !e.checkSkipped(ctx) { + if !e.checkSkippedFromCmd(ctx) { continue } status = model.StageStatus_STAGE_SKIPPED @@ -339,7 +339,7 @@ func (e *Executor) buildAppArgs(customArgs map[string]string) argsTemplate { return args } -func (e *Executor) checkSkipped(ctx context.Context) bool { +func (e *Executor) checkSkippedFromCmd(ctx context.Context) bool { var skipCmd *model.ReportableCommand commands := e.CommandLister.ListCommands() From 022e9404ca6d95bb53b2883d76aeff8eb552df63 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 22 Apr 2024 22:06:37 +0900 Subject: [PATCH 02/46] Draft: impl check of skipping stage Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 70 +++++++++++++++++++++++++++++ pkg/config/application.go | 15 +++++-- pkg/git/repo.go | 1 + 3 files changed, 83 insertions(+), 3 deletions(-) create mode 100644 pkg/app/piped/executor/skipstage.go diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go new file mode 100644 index 0000000000..1c3e45e4ad --- /dev/null +++ b/pkg/app/piped/executor/skipstage.go @@ -0,0 +1,70 @@ +// Copyright 2024 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package executor + +import ( + "context" + "strings" + + "github.com/pipe-cd/pipecd/pkg/config" +) + +// based on stage's config. +func checkSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) (skip bool, err error) { + if opt.Paths == nil && opt.CommitMessagePrefix == "" { + // When no condition is specified for skipping. + return false, nil + } + + repo := in.Application.GitPath.Repo + clonedRepo, err := in.GitClient.Clone(ctx, repo.Id, repo.Remote, repo.Branch, "") + if err != nil { + return false, err + } + + // (1)と(2)はOR。どちらか一方でも満たせばスキップする。 + // (1)ファイルパスで判定する場合 + if opt.Paths != nil { + changedFiles, err := clonedRepo.ChangedFiles(ctx, in.RunningDSP.Revision(), in.TargetDSP.Revision()) + if err != nil { + return false, err + } + + // check whether changed files are included in opt.Paths. + // if any file is included, return true. + if opt.Paths != nil { + for _, path := range changedFiles { + // TODO use regex + if opt.Paths.Match(path) { + return true, nil + } + } + } + } + + // (2)Gitのコミットメッセージで判定する場合 + if opt.CommitMessagePrefix != "" { + commit, err := clonedRepo.GetCommitFromHash(ctx, in.TargetDSP.Revision()) + if err != nil { + return false, err + } + + if strings.HasPrefix(commit.Message, opt.CommitMessagePrefix) { + return true, nil + } + } + + return false, nil +} diff --git a/pkg/config/application.go b/pkg/config/application.go index de27d26ed8..7e79790c45 100644 --- a/pkg/config/application.go +++ b/pkg/config/application.go @@ -404,6 +404,13 @@ func (s *PipelineStage) UnmarshalJSON(data []byte) error { return err } +// SkipStageOptions contains all configurable values for skipping a stage. +type SkipStageOptions struct { + CommitMessagePrefix string `json:"commitMessagePrefix,omitempty"` + // CommitMessageSuffix string + Paths []string `json:"paths,omitempty"` +} + // WaitStageOptions contains all configurable values for a WAIT stage. type WaitStageOptions struct { Duration Duration `json:"duration"` @@ -413,9 +420,10 @@ type WaitStageOptions struct { type WaitApprovalStageOptions struct { // The maximum length of time to wait before giving up. // Defaults to 6h. - Timeout Duration `json:"timeout" default:"6h"` - Approvers []string `json:"approvers"` - MinApproverNum int `json:"minApproverNum" default:"1"` + Timeout Duration `json:"timeout" default:"6h"` + Approvers []string `json:"approvers"` + MinApproverNum int `json:"minApproverNum" default:"1"` + SkipStageOptions SkipStageOptions `json:"skipOn,omitempty"` } func (w *WaitApprovalStageOptions) Validate() error { @@ -448,6 +456,7 @@ type AnalysisStageOptions struct { Metrics []TemplatableAnalysisMetrics `json:"metrics,omitempty"` Logs []TemplatableAnalysisLog `json:"logs,omitempty"` HTTPS []TemplatableAnalysisHTTP `json:"https,omitempty"` + SkipStageOptions SkipStageOptions `json:"skipOn,omitempty"` } func (a *AnalysisStageOptions) Validate() error { diff --git a/pkg/git/repo.go b/pkg/git/repo.go index 4072bc8779..b817076d91 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -39,6 +39,7 @@ type Repo interface { ListCommits(ctx context.Context, visionRange string) ([]Commit, error) GetLatestCommit(ctx context.Context) (Commit, error) GetCommitHashForRev(ctx context.Context, rev string) (string, error) + GetCommitFromHash(ctx context.Context, commithash string) (Commit, error) ChangedFiles(ctx context.Context, from, to string) ([]string, error) Checkout(ctx context.Context, commitish string) error CheckoutPullRequest(ctx context.Context, number int, branch string) error From 59b14ab402f156d1162e165ff742d77c80bf685a Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 29 Apr 2024 09:42:51 +0900 Subject: [PATCH 03/46] Rename to 'prefixes' Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 19 +++++++++++-------- pkg/config/application.go | 2 +- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index 1c3e45e4ad..8c39717b96 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -23,7 +23,7 @@ import ( // based on stage's config. func checkSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) (skip bool, err error) { - if opt.Paths == nil && opt.CommitMessagePrefix == "" { + if opt.Paths == nil && len(opt.CommitMessagePrefixes) == 0 { // When no condition is specified for skipping. return false, nil } @@ -45,24 +45,27 @@ func checkSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) // check whether changed files are included in opt.Paths. // if any file is included, return true. if opt.Paths != nil { - for _, path := range changedFiles { + for _, _ = range changedFiles { // TODO use regex - if opt.Paths.Match(path) { - return true, nil - } + // if opt.Paths.Match(path) { + // return true, nil + // } + panic("skip-stage by path-pattern is not implemented yet") } } } // (2)Gitのコミットメッセージで判定する場合 - if opt.CommitMessagePrefix != "" { + if len(opt.CommitMessagePrefixes) > 0 { commit, err := clonedRepo.GetCommitFromHash(ctx, in.TargetDSP.Revision()) if err != nil { return false, err } - if strings.HasPrefix(commit.Message, opt.CommitMessagePrefix) { - return true, nil + for _, prefix := range opt.CommitMessagePrefixes { + if strings.HasPrefix(commit.Message, prefix) { + return true, nil + } } } diff --git a/pkg/config/application.go b/pkg/config/application.go index 7e79790c45..fe051005ef 100644 --- a/pkg/config/application.go +++ b/pkg/config/application.go @@ -406,7 +406,7 @@ func (s *PipelineStage) UnmarshalJSON(data []byte) error { // SkipStageOptions contains all configurable values for skipping a stage. type SkipStageOptions struct { - CommitMessagePrefix string `json:"commitMessagePrefix,omitempty"` + CommitMessagePrefixes []string `json:"commitMessagePrefixes,omitempty"` // CommitMessageSuffix string Paths []string `json:"paths,omitempty"` } From 691db8a0a23ffbbe5a4c5bf57456f54755a4c893 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 29 Apr 2024 09:55:50 +0900 Subject: [PATCH 04/46] separate to sub funcs Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 74 +++++++++++++++++++---------- 1 file changed, 48 insertions(+), 26 deletions(-) diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index 8c39717b96..6e1779be73 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -19,6 +19,7 @@ import ( "strings" "github.com/pipe-cd/pipecd/pkg/config" + "github.com/pipe-cd/pipecd/pkg/git" ) // based on stage's config. @@ -28,44 +29,65 @@ func checkSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) return false, nil } - repo := in.Application.GitPath.Repo - clonedRepo, err := in.GitClient.Clone(ctx, repo.Id, repo.Remote, repo.Branch, "") + appRepo := in.Application.GitPath.Repo + clonedRepo, err := in.GitClient.Clone(ctx, appRepo.Id, appRepo.Remote, appRepo.Branch, "") if err != nil { return false, err } // (1)と(2)はOR。どちらか一方でも満たせばスキップする。 // (1)ファイルパスで判定する場合 - if opt.Paths != nil { - changedFiles, err := clonedRepo.ChangedFiles(ctx, in.RunningDSP.Revision(), in.TargetDSP.Revision()) - if err != nil { - return false, err - } + skip, err = skipByPathPattern(ctx, in, opt, clonedRepo) + if err != nil { + return false, err + } + if skip { + return true, nil + } + + // (2)Gitのコミットメッセージで判定する場合 + skip, err = skipByCommitMessagePrefixes(ctx, in, opt, clonedRepo) + return skip, err +} + +func skipByPathPattern(ctx context.Context, in Input, opt config.SkipStageOptions, repo git.Repo) (skip bool, err error) { + if opt.Paths == nil { + return false, nil + } + + changedFiles, err := repo.ChangedFiles(ctx, in.RunningDSP.Revision(), in.TargetDSP.Revision()) + if err != nil { + return false, err + } - // check whether changed files are included in opt.Paths. - // if any file is included, return true. - if opt.Paths != nil { - for _, _ = range changedFiles { - // TODO use regex - // if opt.Paths.Match(path) { - // return true, nil - // } - panic("skip-stage by path-pattern is not implemented yet") - } + // check whether changed files are included in opt.Paths. + // if any file is included, return true. + if opt.Paths != nil { + for _, _ = range changedFiles { + // TODO use regex + // if opt.Paths.Match(path) { + // return true, nil + // } + panic("skip-stage by path-pattern is not implemented yet") } } - // (2)Gitのコミットメッセージで判定する場合 + return false, nil +} + +func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipStageOptions, repo git.Repo) (skip bool, err error) { if len(opt.CommitMessagePrefixes) > 0 { - commit, err := clonedRepo.GetCommitFromHash(ctx, in.TargetDSP.Revision()) - if err != nil { - return false, err - } + return false, nil + } + + commit, err := repo.GetCommitFromHash(ctx, in.TargetDSP.Revision()) + if err != nil { + return false, err + } - for _, prefix := range opt.CommitMessagePrefixes { - if strings.HasPrefix(commit.Message, prefix) { - return true, nil - } + for _, prefix := range opt.CommitMessagePrefixes { + if strings.HasPrefix(commit.Message, prefix) { + return true, nil } } From 7c37addd5e4d209e905eba8e9fd2627692e6f2d4 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 29 Apr 2024 11:11:46 +0900 Subject: [PATCH 05/46] Impl Repo.GetCommitFromRev Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 3 ++- pkg/git/repo.go | 19 ++++++++++++++++++- pkg/git/repo_test.go | 27 +++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index 6e1779be73..68ceaba6ff 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -80,7 +80,8 @@ func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipS return false, nil } - commit, err := repo.GetCommitFromHash(ctx, in.TargetDSP.Revision()) + // TODO + commit, err := repo.GetCommitFromRev(ctx, in.TargetDSP.Revision()) if err != nil { return false, err } diff --git a/pkg/git/repo.go b/pkg/git/repo.go index b817076d91..4016f56673 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -39,7 +39,7 @@ type Repo interface { ListCommits(ctx context.Context, visionRange string) ([]Commit, error) GetLatestCommit(ctx context.Context) (Commit, error) GetCommitHashForRev(ctx context.Context, rev string) (string, error) - GetCommitFromHash(ctx context.Context, commithash string) (Commit, error) + GetCommitFromRev(ctx context.Context, rev string) (Commit, error) ChangedFiles(ctx context.Context, from, to string) ([]string, error) Checkout(ctx context.Context, commitish string) error CheckoutPullRequest(ctx context.Context, number int, branch string) error @@ -141,6 +141,23 @@ func (r *repo) GetCommitHashForRev(ctx context.Context, rev string) (string, err return strings.TrimSpace(string(out)), nil } +// GetCommitFromRev returns the commit for the given hash. +func (r *repo) GetCommitFromRev(ctx context.Context, rev string) (Commit, error) { + args := []string{ + "show", + "--quiet", // suppress diff output + "--no-decorate", + fmt.Sprintf("--pretty=format:%s", commitLogFormat), + rev, + } + out, err := r.runGitCommand(ctx, args...) + if err != nil { + return Commit{}, formatCommandError(err, out) + } + + return parseCommit(string(out)) +} + // ChangedFiles returns a list of files those were touched between two commits. func (r *repo) ChangedFiles(ctx context.Context, from, to string) ([]string, error) { out, err := r.runGitCommand(ctx, "diff", "--name-only", from, to) diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index 65fa286e6c..9c54c449af 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -242,3 +242,30 @@ func Test_setGCAutoDetach(t *testing.T) { assert.Equal(t, false, got) } + +func TestGetCommitFromRev(t *testing.T) { + faker, err := newFaker() + require.NoError(t, err) + defer faker.clean() + + var ( + org = "test-repo-org" + repoName = "repo-get-commit-from-rev" + ctx = context.Background() + ) + + err = faker.makeRepo(org, repoName) + require.NoError(t, err) + r := &repo{ + dir: faker.repoDir(org, repoName), + gitPath: faker.gitPath, + } + + commits, err := r.ListCommits(ctx, "") + require.NoError(t, err) + assert.Equal(t, 1, len(commits)) + + commit, err := r.GetCommitFromRev(ctx, "HEAD") + require.NoError(t, err) + assert.Equal(t, commits[0].Hash, commit.Hash) +} From ced60541d53c6f244266141e207069ff2b779b09 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 29 Apr 2024 12:09:29 +0900 Subject: [PATCH 06/46] Impl skipByPathPattern() Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 51 ++++++++++++++++++----------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index 68ceaba6ff..20d87f360b 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -19,6 +19,7 @@ import ( "strings" "github.com/pipe-cd/pipecd/pkg/config" + "github.com/pipe-cd/pipecd/pkg/filematcher" "github.com/pipe-cd/pipecd/pkg/git" ) @@ -50,47 +51,57 @@ func checkSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) return skip, err } -func skipByPathPattern(ctx context.Context, in Input, opt config.SkipStageOptions, repo git.Repo) (skip bool, err error) { - if opt.Paths == nil { +func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipStageOptions, repo git.Repo) (skip bool, err error) { + if len(opt.CommitMessagePrefixes) > 0 { return false, nil } - changedFiles, err := repo.ChangedFiles(ctx, in.RunningDSP.Revision(), in.TargetDSP.Revision()) + commit, err := repo.GetCommitFromRev(ctx, in.TargetDSP.Revision()) if err != nil { return false, err } - // check whether changed files are included in opt.Paths. - // if any file is included, return true. - if opt.Paths != nil { - for _, _ = range changedFiles { - // TODO use regex - // if opt.Paths.Match(path) { - // return true, nil - // } - panic("skip-stage by path-pattern is not implemented yet") + for _, prefix := range opt.CommitMessagePrefixes { + if strings.HasPrefix(commit.Message, prefix) { + return true, nil } } return false, nil } -func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipStageOptions, repo git.Repo) (skip bool, err error) { - if len(opt.CommitMessagePrefixes) > 0 { +func skipByPathPattern(ctx context.Context, in Input, opt config.SkipStageOptions, repo git.Repo) (skip bool, err error) { + if opt.Paths == nil { return false, nil } - // TODO - commit, err := repo.GetCommitFromRev(ctx, in.TargetDSP.Revision()) + changedFiles, err := repo.ChangedFiles(ctx, in.RunningDSP.Revision(), in.TargetDSP.Revision()) if err != nil { return false, err } + // TODO: Which should we choose when no file is changed? (e.g. when SYNC command is executed from Console) + // hasOnlyPathToSkip() returns true in this case. + // if len(changedFiles) == 0 { + // return false, err + // } - for _, prefix := range opt.CommitMessagePrefixes { - if strings.HasPrefix(commit.Message, prefix) { - return true, nil + return hasOnlyPathsToSkip(opt.Paths, changedFiles) +} + +// hasOnlyPathsToSkip returns true if and only if all changed files are included in `skipPatterns`. +// If any changed file is not included in `skipPatterns`, it returns false. +// If `changedFiles` is empty, it returns true. +func hasOnlyPathsToSkip(skipPatterns []string, changedFiles []string) (bool, error) { + matcher, err := filematcher.NewPatternMatcher(skipPatterns) + if err != nil { + return false, err + } + + for _, changedFile := range changedFiles { + if !matcher.Matches(changedFile) { + return false, nil } } - return false, nil + return true, nil } From 3809d145d79addb0d5eb4f9946833598fad65e72 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 29 Apr 2024 12:32:30 +0900 Subject: [PATCH 07/46] Add skipOn options to Wait Stage Options Signed-off-by: t-kikuc --- pkg/config/application.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/config/application.go b/pkg/config/application.go index fe051005ef..5f384da126 100644 --- a/pkg/config/application.go +++ b/pkg/config/application.go @@ -413,7 +413,8 @@ type SkipStageOptions struct { // WaitStageOptions contains all configurable values for a WAIT stage. type WaitStageOptions struct { - Duration Duration `json:"duration"` + Duration Duration `json:"duration"` + SkipStageOptions SkipStageOptions `json:"skipOn,omitempty"` } // WaitStageOptions contains all configurable values for a WAIT_APPROVAL stage. From 7d7f9b7aeb8873a30bf9c4da102ceef8b47a6b0c Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 29 Apr 2024 12:39:01 +0900 Subject: [PATCH 08/46] Add check of skipping stage in Analysis,Wait,WaitApproval Stages Signed-off-by: t-kikuc --- pkg/app/piped/executor/analysis/analysis.go | 10 ++++++++++ pkg/app/piped/executor/skipstage.go | 2 +- pkg/app/piped/executor/wait/wait.go | 10 ++++++++++ pkg/app/piped/executor/waitapproval/waitapproval.go | 10 ++++++++++ 4 files changed, 31 insertions(+), 1 deletion(-) diff --git a/pkg/app/piped/executor/analysis/analysis.go b/pkg/app/piped/executor/analysis/analysis.go index 328ad5a249..ca84064d31 100644 --- a/pkg/app/piped/executor/analysis/analysis.go +++ b/pkg/app/piped/executor/analysis/analysis.go @@ -62,6 +62,16 @@ func Register(r registerer) { // Execute spawns and runs multiple analyzer that run a query at the regular time. // Any of those fail then the stage ends with failure. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { + // Skip the stage if needed based on the skip config. + skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.AnalysisStageOptions.SkipStageOptions) + if err != nil { + e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) + return model.StageStatus_STAGE_FAILURE + } + if skip { + return model.StageStatus_STAGE_SKIPPED + } + e.startTime = time.Now() ctx := sig.Context() options := e.StageConfig.AnalysisStageOptions diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index 20d87f360b..e4fdd4f1d4 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -24,7 +24,7 @@ import ( ) // based on stage's config. -func checkSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) (skip bool, err error) { +func CheckSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) (skip bool, err error) { if opt.Paths == nil && len(opt.CommitMessagePrefixes) == 0 { // When no condition is specified for skipping. return false, nil diff --git a/pkg/app/piped/executor/wait/wait.go b/pkg/app/piped/executor/wait/wait.go index db472bcbd1..4dfbb7f997 100644 --- a/pkg/app/piped/executor/wait/wait.go +++ b/pkg/app/piped/executor/wait/wait.go @@ -51,6 +51,16 @@ func Register(r registerer) { // Execute starts waiting for the specified duration. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { + // Skip the stage if needed based on the skip config. + skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitStageOptions.SkipStageOptions) + if err != nil { + e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) + return model.StageStatus_STAGE_FAILURE + } + if skip { + return model.StageStatus_STAGE_SKIPPED + } + var ( originalStatus = e.Stage.Status duration = defaultDuration diff --git a/pkg/app/piped/executor/waitapproval/waitapproval.go b/pkg/app/piped/executor/waitapproval/waitapproval.go index bd69460903..b4944e66b3 100644 --- a/pkg/app/piped/executor/waitapproval/waitapproval.go +++ b/pkg/app/piped/executor/waitapproval/waitapproval.go @@ -52,6 +52,16 @@ func Register(r registerer) { // Execute starts waiting until an approval from one of the specified users. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { + // Skip the stage if needed based on the skip config. + skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitApprovalStageOptions.SkipStageOptions) + if err != nil { + e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) + return model.StageStatus_STAGE_FAILURE + } + if skip { + return model.StageStatus_STAGE_SKIPPED + } + var ( originalStatus = e.Stage.Status ctx = sig.Context() From c4a529d1df2061108524aa87f1dc41faccc90f5a Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 29 Apr 2024 12:42:11 +0900 Subject: [PATCH 09/46] Fix comments Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index e4fdd4f1d4..3885df5a46 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -23,7 +23,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/git" ) -// based on stage's config. +// CheckSkipStage checks whether the stage should be skipped or not. func CheckSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) (skip bool, err error) { if opt.Paths == nil && len(opt.CommitMessagePrefixes) == 0 { // When no condition is specified for skipping. @@ -36,8 +36,7 @@ func CheckSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) return false, err } - // (1)と(2)はOR。どちらか一方でも満たせばスキップする。 - // (1)ファイルパスで判定する場合 + // Check by path pattern skip, err = skipByPathPattern(ctx, in, opt, clonedRepo) if err != nil { return false, err @@ -46,7 +45,7 @@ func CheckSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) return true, nil } - // (2)Gitのコミットメッセージで判定する場合 + // Check by prefix of Git commit message skip, err = skipByCommitMessagePrefixes(ctx, in, opt, clonedRepo) return skip, err } From e925ef7344dd3eda0a93c2d92ac3f2248edbcdd2 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Tue, 30 Apr 2024 10:08:50 +0900 Subject: [PATCH 10/46] draft: add an empty test file Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage_test.go | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 pkg/app/piped/executor/skipstage_test.go diff --git a/pkg/app/piped/executor/skipstage_test.go b/pkg/app/piped/executor/skipstage_test.go new file mode 100644 index 0000000000..999acb5447 --- /dev/null +++ b/pkg/app/piped/executor/skipstage_test.go @@ -0,0 +1,25 @@ +// Copyright 2024 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package executor + +import "testing" + +func TestSkipByCommitMessagePrefixes(t *testing.T) { + +} + +func TestHasOnlyPathsToSkip(t *testing.T) { + +} From ee89a23293b72b391ceb158659907979892356ae Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 20 May 2024 08:52:34 +0900 Subject: [PATCH 11/46] fix comments Signed-off-by: t-kikuc --- pkg/config/application.go | 3 +-- pkg/git/repo.go | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/config/application.go b/pkg/config/application.go index 5f384da126..fc3e53c320 100644 --- a/pkg/config/application.go +++ b/pkg/config/application.go @@ -407,8 +407,7 @@ func (s *PipelineStage) UnmarshalJSON(data []byte) error { // SkipStageOptions contains all configurable values for skipping a stage. type SkipStageOptions struct { CommitMessagePrefixes []string `json:"commitMessagePrefixes,omitempty"` - // CommitMessageSuffix string - Paths []string `json:"paths,omitempty"` + Paths []string `json:"paths,omitempty"` } // WaitStageOptions contains all configurable values for a WAIT stage. diff --git a/pkg/git/repo.go b/pkg/git/repo.go index 4016f56673..e0bed2ca5f 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -141,8 +141,8 @@ func (r *repo) GetCommitHashForRev(ctx context.Context, rev string) (string, err return strings.TrimSpace(string(out)), nil } -// GetCommitFromRev returns the commit for the given hash. -func (r *repo) GetCommitFromRev(ctx context.Context, rev string) (Commit, error) { +// GetCommitFromRev returns the commit for the given rev. +func (r *repo) GetCommitForRev(ctx context.Context, rev string) (Commit, error) { args := []string{ "show", "--quiet", // suppress diff output From 87efcd5d301b8817d02cfe77498fff472a1c588a Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 09:04:41 +0900 Subject: [PATCH 12/46] Fix func name Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 2 +- pkg/git/repo.go | 2 +- pkg/git/repo_test.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index 3885df5a46..1c49c607bd 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -55,7 +55,7 @@ func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipS return false, nil } - commit, err := repo.GetCommitFromRev(ctx, in.TargetDSP.Revision()) + commit, err := repo.GetCommitForRev(ctx, in.TargetDSP.Revision()) if err != nil { return false, err } diff --git a/pkg/git/repo.go b/pkg/git/repo.go index e0bed2ca5f..dc1911e72a 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -39,7 +39,7 @@ type Repo interface { ListCommits(ctx context.Context, visionRange string) ([]Commit, error) GetLatestCommit(ctx context.Context) (Commit, error) GetCommitHashForRev(ctx context.Context, rev string) (string, error) - GetCommitFromRev(ctx context.Context, rev string) (Commit, error) + GetCommitForRev(ctx context.Context, rev string) (Commit, error) ChangedFiles(ctx context.Context, from, to string) ([]string, error) Checkout(ctx context.Context, commitish string) error CheckoutPullRequest(ctx context.Context, number int, branch string) error diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index 9c54c449af..8befcbe267 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -243,7 +243,7 @@ func Test_setGCAutoDetach(t *testing.T) { assert.Equal(t, false, got) } -func TestGetCommitFromRev(t *testing.T) { +func TestGetCommitForRev(t *testing.T) { faker, err := newFaker() require.NoError(t, err) defer faker.clean() @@ -265,7 +265,7 @@ func TestGetCommitFromRev(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, len(commits)) - commit, err := r.GetCommitFromRev(ctx, "HEAD") + commit, err := r.GetCommitForRev(ctx, "HEAD") require.NoError(t, err) assert.Equal(t, commits[0].Hash, commit.Hash) } From 79fe4c5d05317b1faa3797fb2f11ce1dc58dc5f5 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 09:07:21 +0900 Subject: [PATCH 13/46] Rename struct from confusing one Signed-off-by: t-kikuc --- pkg/app/piped/executor/analysis/analysis.go | 2 +- pkg/app/piped/executor/skipstage.go | 6 +++--- pkg/app/piped/executor/wait/wait.go | 2 +- .../executor/waitapproval/waitapproval.go | 2 +- pkg/config/application.go | 18 +++++++++--------- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/app/piped/executor/analysis/analysis.go b/pkg/app/piped/executor/analysis/analysis.go index ca84064d31..4a5d1682e9 100644 --- a/pkg/app/piped/executor/analysis/analysis.go +++ b/pkg/app/piped/executor/analysis/analysis.go @@ -63,7 +63,7 @@ func Register(r registerer) { // Any of those fail then the stage ends with failure. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.AnalysisStageOptions.SkipStageOptions) + skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.AnalysisStageOptions.SkipOptions) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index 1c49c607bd..266d10ae70 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -24,7 +24,7 @@ import ( ) // CheckSkipStage checks whether the stage should be skipped or not. -func CheckSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) (skip bool, err error) { +func CheckSkipStage(ctx context.Context, in Input, opt config.SkipOptions) (skip bool, err error) { if opt.Paths == nil && len(opt.CommitMessagePrefixes) == 0 { // When no condition is specified for skipping. return false, nil @@ -50,7 +50,7 @@ func CheckSkipStage(ctx context.Context, in Input, opt config.SkipStageOptions) return skip, err } -func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipStageOptions, repo git.Repo) (skip bool, err error) { +func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipOptions, repo git.Repo) (skip bool, err error) { if len(opt.CommitMessagePrefixes) > 0 { return false, nil } @@ -69,7 +69,7 @@ func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipS return false, nil } -func skipByPathPattern(ctx context.Context, in Input, opt config.SkipStageOptions, repo git.Repo) (skip bool, err error) { +func skipByPathPattern(ctx context.Context, in Input, opt config.SkipOptions, repo git.Repo) (skip bool, err error) { if opt.Paths == nil { return false, nil } diff --git a/pkg/app/piped/executor/wait/wait.go b/pkg/app/piped/executor/wait/wait.go index 4dfbb7f997..ab62bbe7d5 100644 --- a/pkg/app/piped/executor/wait/wait.go +++ b/pkg/app/piped/executor/wait/wait.go @@ -52,7 +52,7 @@ func Register(r registerer) { // Execute starts waiting for the specified duration. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitStageOptions.SkipStageOptions) + skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitStageOptions.SkipOptions) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE diff --git a/pkg/app/piped/executor/waitapproval/waitapproval.go b/pkg/app/piped/executor/waitapproval/waitapproval.go index b4944e66b3..5b70bc3790 100644 --- a/pkg/app/piped/executor/waitapproval/waitapproval.go +++ b/pkg/app/piped/executor/waitapproval/waitapproval.go @@ -53,7 +53,7 @@ func Register(r registerer) { // Execute starts waiting until an approval from one of the specified users. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitApprovalStageOptions.SkipStageOptions) + skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitApprovalStageOptions.SkipOptions) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE diff --git a/pkg/config/application.go b/pkg/config/application.go index fc3e53c320..33bc6f3778 100644 --- a/pkg/config/application.go +++ b/pkg/config/application.go @@ -404,26 +404,26 @@ func (s *PipelineStage) UnmarshalJSON(data []byte) error { return err } -// SkipStageOptions contains all configurable values for skipping a stage. -type SkipStageOptions struct { +// SkipOptions contains all configurable values for skipping a stage. +type SkipOptions struct { CommitMessagePrefixes []string `json:"commitMessagePrefixes,omitempty"` Paths []string `json:"paths,omitempty"` } // WaitStageOptions contains all configurable values for a WAIT stage. type WaitStageOptions struct { - Duration Duration `json:"duration"` - SkipStageOptions SkipStageOptions `json:"skipOn,omitempty"` + Duration Duration `json:"duration"` + SkipOptions SkipOptions `json:"skipOn,omitempty"` } // WaitStageOptions contains all configurable values for a WAIT_APPROVAL stage. type WaitApprovalStageOptions struct { // The maximum length of time to wait before giving up. // Defaults to 6h. - Timeout Duration `json:"timeout" default:"6h"` - Approvers []string `json:"approvers"` - MinApproverNum int `json:"minApproverNum" default:"1"` - SkipStageOptions SkipStageOptions `json:"skipOn,omitempty"` + Timeout Duration `json:"timeout" default:"6h"` + Approvers []string `json:"approvers"` + MinApproverNum int `json:"minApproverNum" default:"1"` + SkipOptions SkipOptions `json:"skipOn,omitempty"` } func (w *WaitApprovalStageOptions) Validate() error { @@ -456,7 +456,7 @@ type AnalysisStageOptions struct { Metrics []TemplatableAnalysisMetrics `json:"metrics,omitempty"` Logs []TemplatableAnalysisLog `json:"logs,omitempty"` HTTPS []TemplatableAnalysisHTTP `json:"https,omitempty"` - SkipStageOptions SkipStageOptions `json:"skipOn,omitempty"` + SkipOptions SkipOptions `json:"skipOn,omitempty"` } func (a *AnalysisStageOptions) Validate() error { From faf376de793738f5944465a35f67f96582fbdbca Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 09:17:38 +0900 Subject: [PATCH 14/46] Refine order of args Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index 266d10ae70..cf98df98ad 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -37,7 +37,7 @@ func CheckSkipStage(ctx context.Context, in Input, opt config.SkipOptions) (skip } // Check by path pattern - skip, err = skipByPathPattern(ctx, in, opt, clonedRepo) + skip, err = skipByPathPattern(ctx, opt, clonedRepo, in.RunningDSP.Revision(), in.TargetDSP.Revision()) if err != nil { return false, err } @@ -46,16 +46,16 @@ func CheckSkipStage(ctx context.Context, in Input, opt config.SkipOptions) (skip } // Check by prefix of Git commit message - skip, err = skipByCommitMessagePrefixes(ctx, in, opt, clonedRepo) + skip, err = skipByCommitMessagePrefixes(ctx, opt, clonedRepo, in.TargetDSP.Revision()) return skip, err } -func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipOptions, repo git.Repo) (skip bool, err error) { +func skipByCommitMessagePrefixes(ctx context.Context, opt config.SkipOptions, repo git.Repo, targetRev string) (skip bool, err error) { if len(opt.CommitMessagePrefixes) > 0 { return false, nil } - commit, err := repo.GetCommitForRev(ctx, in.TargetDSP.Revision()) + commit, err := repo.GetCommitForRev(ctx, targetRev) if err != nil { return false, err } @@ -69,12 +69,12 @@ func skipByCommitMessagePrefixes(ctx context.Context, in Input, opt config.SkipO return false, nil } -func skipByPathPattern(ctx context.Context, in Input, opt config.SkipOptions, repo git.Repo) (skip bool, err error) { +func skipByPathPattern(ctx context.Context, opt config.SkipOptions, repo git.Repo, runningRev, targetRev string) (skip bool, err error) { if opt.Paths == nil { return false, nil } - changedFiles, err := repo.ChangedFiles(ctx, in.RunningDSP.Revision(), in.TargetDSP.Revision()) + changedFiles, err := repo.ChangedFiles(ctx, runningRev, targetRev) if err != nil { return false, err } From 152e82f0381c01db2c625c56a7a18626e2a970c7 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 10:25:50 +0900 Subject: [PATCH 15/46] add test of hasOnlyPathsToSkip Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage_test.go | 83 +++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/pkg/app/piped/executor/skipstage_test.go b/pkg/app/piped/executor/skipstage_test.go index 999acb5447..5321c02010 100644 --- a/pkg/app/piped/executor/skipstage_test.go +++ b/pkg/app/piped/executor/skipstage_test.go @@ -14,12 +14,93 @@ package executor -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) func TestSkipByCommitMessagePrefixes(t *testing.T) { } func TestHasOnlyPathsToSkip(t *testing.T) { + t.Parallel() + testcases := []struct { + name string + skipPatterns []string + changedFiles []string + expected bool + }{ + { + name: "no skip patterns", + skipPatterns: nil, + changedFiles: []string{"file1"}, + expected: false, + }, + { + name: "no changed files", + skipPatterns: []string{"file1"}, + changedFiles: nil, + expected: true, + }, + { + name: "no skip patterns and no changed files", + skipPatterns: nil, + changedFiles: nil, + expected: true, + }, + { + name: "skip pattern matches all changed files", + skipPatterns: []string{"file1", "file2"}, + changedFiles: []string{"file1", "file2"}, + expected: true, + }, + { + name: "skip pattern does not match changed files", + skipPatterns: []string{"file1", "file2"}, + changedFiles: []string{"file1", "file3"}, + expected: false, + }, + { + name: "skip files of a directory", + skipPatterns: []string{"dir1/*"}, + changedFiles: []string{"dir1/file1", "dir1/file2"}, + expected: true, + }, + { + name: "skip files of a directory recursively", + skipPatterns: []string{"dir1/**"}, + changedFiles: []string{"dir1/file1", "dir1/sub/file2"}, + expected: true, + }, + { + name: "skip files of a directory but not recursively", + skipPatterns: []string{"dir1/*"}, + changedFiles: []string{"dir1/file1", "dir1/sub/file2"}, + expected: false, + }, + { + name: "skip files with a specific extension of a directory recursively", + skipPatterns: []string{"dir1/**.yaml"}, + changedFiles: []string{"dir1/file1.yaml", "dir1/sub/file2.yaml"}, + expected: true, + }, + { + name: "skip files with a specific extension but not recursively", + skipPatterns: []string{"*.yaml"}, + changedFiles: []string{"dir1/file1.yaml"}, + expected: false, + }, + } + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + // We do not use t.Parallel() here due to https://pkg.go.dev/github.com/pipe-cd/pipecd/pkg/filematcher#PatternMatcher.Matches. + actual, err := hasOnlyPathsToSkip(tc.skipPatterns, tc.changedFiles) + assert.NoError(t, err) + assert.Equal(t, tc.expected, actual) + }) + } } From 2532f42f93c59b77e8c9ff9c9f6da7fe955c298e Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 10:36:25 +0900 Subject: [PATCH 16/46] Refine testcase names Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/app/piped/executor/skipstage_test.go b/pkg/app/piped/executor/skipstage_test.go index 5321c02010..d0351199f3 100644 --- a/pkg/app/piped/executor/skipstage_test.go +++ b/pkg/app/piped/executor/skipstage_test.go @@ -69,25 +69,25 @@ func TestHasOnlyPathsToSkip(t *testing.T) { expected: true, }, { - name: "skip files of a directory recursively", + name: "skip files recursively", skipPatterns: []string{"dir1/**"}, changedFiles: []string{"dir1/file1", "dir1/sub/file2"}, expected: true, }, { - name: "skip files of a directory but not recursively", + name: "skip files not recursively", skipPatterns: []string{"dir1/*"}, - changedFiles: []string{"dir1/file1", "dir1/sub/file2"}, + changedFiles: []string{"dir1/sub/file2"}, expected: false, }, { - name: "skip files with a specific extension of a directory recursively", + name: "skip files with the extension recursively", skipPatterns: []string{"dir1/**.yaml"}, changedFiles: []string{"dir1/file1.yaml", "dir1/sub/file2.yaml"}, expected: true, }, { - name: "skip files with a specific extension but not recursively", + name: "skip files with the extension not recursively", skipPatterns: []string{"*.yaml"}, changedFiles: []string{"dir1/file1.yaml"}, expected: false, From 8329c41a28d1924919401dfb2ca8e3e9caf1e775 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 10:44:59 +0900 Subject: [PATCH 17/46] rename 'expected' to 'skip' Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/app/piped/executor/skipstage_test.go b/pkg/app/piped/executor/skipstage_test.go index d0351199f3..b2829ca4db 100644 --- a/pkg/app/piped/executor/skipstage_test.go +++ b/pkg/app/piped/executor/skipstage_test.go @@ -30,67 +30,67 @@ func TestHasOnlyPathsToSkip(t *testing.T) { name string skipPatterns []string changedFiles []string - expected bool + skip bool }{ { name: "no skip patterns", skipPatterns: nil, changedFiles: []string{"file1"}, - expected: false, + skip: false, }, { name: "no changed files", skipPatterns: []string{"file1"}, changedFiles: nil, - expected: true, + skip: true, }, { name: "no skip patterns and no changed files", skipPatterns: nil, changedFiles: nil, - expected: true, + skip: true, }, { name: "skip pattern matches all changed files", skipPatterns: []string{"file1", "file2"}, changedFiles: []string{"file1", "file2"}, - expected: true, + skip: true, }, { name: "skip pattern does not match changed files", skipPatterns: []string{"file1", "file2"}, changedFiles: []string{"file1", "file3"}, - expected: false, + skip: false, }, { name: "skip files of a directory", skipPatterns: []string{"dir1/*"}, changedFiles: []string{"dir1/file1", "dir1/file2"}, - expected: true, + skip: true, }, { name: "skip files recursively", skipPatterns: []string{"dir1/**"}, changedFiles: []string{"dir1/file1", "dir1/sub/file2"}, - expected: true, + skip: true, }, { name: "skip files not recursively", skipPatterns: []string{"dir1/*"}, changedFiles: []string{"dir1/sub/file2"}, - expected: false, + skip: false, }, { name: "skip files with the extension recursively", skipPatterns: []string{"dir1/**.yaml"}, changedFiles: []string{"dir1/file1.yaml", "dir1/sub/file2.yaml"}, - expected: true, + skip: true, }, { name: "skip files with the extension not recursively", skipPatterns: []string{"*.yaml"}, changedFiles: []string{"dir1/file1.yaml"}, - expected: false, + skip: false, }, } @@ -100,7 +100,7 @@ func TestHasOnlyPathsToSkip(t *testing.T) { // We do not use t.Parallel() here due to https://pkg.go.dev/github.com/pipe-cd/pipecd/pkg/filematcher#PatternMatcher.Matches. actual, err := hasOnlyPathsToSkip(tc.skipPatterns, tc.changedFiles) assert.NoError(t, err) - assert.Equal(t, tc.expected, actual) + assert.Equal(t, tc.skip, actual) }) } } From a7c87b4e2c9b5cf45109bc9eb641799ff62b59da Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 11:01:44 +0900 Subject: [PATCH 18/46] Fix testcases Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/app/piped/executor/skipstage_test.go b/pkg/app/piped/executor/skipstage_test.go index b2829ca4db..eb416b0966 100644 --- a/pkg/app/piped/executor/skipstage_test.go +++ b/pkg/app/piped/executor/skipstage_test.go @@ -74,20 +74,14 @@ func TestHasOnlyPathsToSkip(t *testing.T) { changedFiles: []string{"dir1/file1", "dir1/sub/file2"}, skip: true, }, - { - name: "skip files not recursively", - skipPatterns: []string{"dir1/*"}, - changedFiles: []string{"dir1/sub/file2"}, - skip: false, - }, { name: "skip files with the extension recursively", - skipPatterns: []string{"dir1/**.yaml"}, - changedFiles: []string{"dir1/file1.yaml", "dir1/sub/file2.yaml"}, + skipPatterns: []string{"dir1/**/*.yaml"}, + changedFiles: []string{"dir1/file1.yaml", "dir1/sub1/file2.yaml", "dir1/sub1/sub2/file3.yaml"}, skip: true, }, { - name: "skip files with the extension not recursively", + name: "skip files not recursively", skipPatterns: []string{"*.yaml"}, changedFiles: []string{"dir1/file1.yaml"}, skip: false, From f3fa841f86c05d174fa2df73321c658daaa0f069 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 11:14:33 +0900 Subject: [PATCH 19/46] separate func Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage.go index cf98df98ad..95917d3a33 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage.go @@ -60,13 +60,16 @@ func skipByCommitMessagePrefixes(ctx context.Context, opt config.SkipOptions, re return false, err } - for _, prefix := range opt.CommitMessagePrefixes { - if strings.HasPrefix(commit.Message, prefix) { - return true, nil + return commitMessageHasAnyPrefix(commit.Message, opt.CommitMessagePrefixes), nil +} + +func commitMessageHasAnyPrefix(commitMessage string, prefixes []string) bool { + for _, prefix := range prefixes { + if strings.HasPrefix(commitMessage, prefix) { + return true } } - - return false, nil + return false } func skipByPathPattern(ctx context.Context, opt config.SkipOptions, repo git.Repo, runningRev, targetRev string) (skip bool, err error) { @@ -78,18 +81,11 @@ func skipByPathPattern(ctx context.Context, opt config.SkipOptions, repo git.Rep if err != nil { return false, err } - // TODO: Which should we choose when no file is changed? (e.g. when SYNC command is executed from Console) - // hasOnlyPathToSkip() returns true in this case. - // if len(changedFiles) == 0 { - // return false, err - // } - return hasOnlyPathsToSkip(opt.Paths, changedFiles) } // hasOnlyPathsToSkip returns true if and only if all changed files are included in `skipPatterns`. -// If any changed file is not included in `skipPatterns`, it returns false. -// If `changedFiles` is empty, it returns true. +// If any changed file does not match all `skipPatterns`, it returns false. func hasOnlyPathsToSkip(skipPatterns []string, changedFiles []string) (bool, error) { matcher, err := filematcher.NewPatternMatcher(skipPatterns) if err != nil { From 41e136b27fba8c64d9f9c9c5b15aaa78664ca6ec Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 11:14:50 +0900 Subject: [PATCH 20/46] add test for skipByCommitMessagePrefixes Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage_test.go | 33 ++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/pkg/app/piped/executor/skipstage_test.go b/pkg/app/piped/executor/skipstage_test.go index eb416b0966..6ee68c3370 100644 --- a/pkg/app/piped/executor/skipstage_test.go +++ b/pkg/app/piped/executor/skipstage_test.go @@ -21,7 +21,40 @@ import ( ) func TestSkipByCommitMessagePrefixes(t *testing.T) { + t.Parallel() + testcases := []struct { + name string + commitMessage string + prefixes []string + skip bool + }{ + { + name: "no prefixes", + commitMessage: "test message", + prefixes: []string{}, + skip: false, + }, + { + name: "no commit message", + commitMessage: "", + prefixes: []string{"to-skip"}, + skip: false, + }, + { + name: "prefix matches", + commitMessage: "to-skip: test message", + prefixes: []string{"to-skip"}, + skip: true, + }, + } + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + skip := commitMessageHasAnyPrefix(tc.commitMessage, tc.prefixes) + assert.Equal(t, tc.skip, skip) + }) + } } func TestHasOnlyPathsToSkip(t *testing.T) { From 794e2e5697f5b89733696a57d136d5e1b1344224 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 11:18:18 +0900 Subject: [PATCH 21/46] Moved package to new 'skipstage' Signed-off-by: t-kikuc --- pkg/app/piped/executor/analysis/analysis.go | 3 ++- pkg/app/piped/executor/{ => skipstage}/skipstage.go | 9 +++++---- pkg/app/piped/executor/{ => skipstage}/skipstage_test.go | 2 +- pkg/app/piped/executor/wait/wait.go | 3 ++- pkg/app/piped/executor/waitapproval/waitapproval.go | 3 ++- 5 files changed, 12 insertions(+), 8 deletions(-) rename pkg/app/piped/executor/{ => skipstage}/skipstage.go (89%) rename pkg/app/piped/executor/{ => skipstage}/skipstage_test.go (99%) diff --git a/pkg/app/piped/executor/analysis/analysis.go b/pkg/app/piped/executor/analysis/analysis.go index 4a5d1682e9..13826531b1 100644 --- a/pkg/app/piped/executor/analysis/analysis.go +++ b/pkg/app/piped/executor/analysis/analysis.go @@ -29,6 +29,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/app/piped/analysisprovider/metrics" metricsfactory "github.com/pipe-cd/pipecd/pkg/app/piped/analysisprovider/metrics/factory" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" + "github.com/pipe-cd/pipecd/pkg/app/piped/executor/skipstage" "github.com/pipe-cd/pipecd/pkg/config" "github.com/pipe-cd/pipecd/pkg/model" ) @@ -63,7 +64,7 @@ func Register(r registerer) { // Any of those fail then the stage ends with failure. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.AnalysisStageOptions.SkipOptions) + skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.AnalysisStageOptions.SkipOptions) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE diff --git a/pkg/app/piped/executor/skipstage.go b/pkg/app/piped/executor/skipstage/skipstage.go similarity index 89% rename from pkg/app/piped/executor/skipstage.go rename to pkg/app/piped/executor/skipstage/skipstage.go index 95917d3a33..f8ebdec9cf 100644 --- a/pkg/app/piped/executor/skipstage.go +++ b/pkg/app/piped/executor/skipstage/skipstage.go @@ -12,26 +12,27 @@ // See the License for the specific language governing permissions and // limitations under the License. -package executor +package skipstage import ( "context" "strings" + "github.com/pipe-cd/pipecd/pkg/app/piped/executor" "github.com/pipe-cd/pipecd/pkg/config" "github.com/pipe-cd/pipecd/pkg/filematcher" "github.com/pipe-cd/pipecd/pkg/git" ) // CheckSkipStage checks whether the stage should be skipped or not. -func CheckSkipStage(ctx context.Context, in Input, opt config.SkipOptions) (skip bool, err error) { +func CheckSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptions) (skip bool, err error) { if opt.Paths == nil && len(opt.CommitMessagePrefixes) == 0 { // When no condition is specified for skipping. return false, nil } - appRepo := in.Application.GitPath.Repo - clonedRepo, err := in.GitClient.Clone(ctx, appRepo.Id, appRepo.Remote, appRepo.Branch, "") + repoCfg := in.Application.GitPath.Repo + clonedRepo, err := in.GitClient.Clone(ctx, repoCfg.Id, repoCfg.Remote, repoCfg.Branch, "") if err != nil { return false, err } diff --git a/pkg/app/piped/executor/skipstage_test.go b/pkg/app/piped/executor/skipstage/skipstage_test.go similarity index 99% rename from pkg/app/piped/executor/skipstage_test.go rename to pkg/app/piped/executor/skipstage/skipstage_test.go index 6ee68c3370..bf118e60ae 100644 --- a/pkg/app/piped/executor/skipstage_test.go +++ b/pkg/app/piped/executor/skipstage/skipstage_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package executor +package skipstage import ( "testing" diff --git a/pkg/app/piped/executor/wait/wait.go b/pkg/app/piped/executor/wait/wait.go index ab62bbe7d5..716b0d5686 100644 --- a/pkg/app/piped/executor/wait/wait.go +++ b/pkg/app/piped/executor/wait/wait.go @@ -22,6 +22,7 @@ import ( "go.uber.org/zap" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" + "github.com/pipe-cd/pipecd/pkg/app/piped/executor/skipstage" "github.com/pipe-cd/pipecd/pkg/model" ) @@ -52,7 +53,7 @@ func Register(r registerer) { // Execute starts waiting for the specified duration. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitStageOptions.SkipOptions) + skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitStageOptions.SkipOptions) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE diff --git a/pkg/app/piped/executor/waitapproval/waitapproval.go b/pkg/app/piped/executor/waitapproval/waitapproval.go index 5b70bc3790..3a92b9c879 100644 --- a/pkg/app/piped/executor/waitapproval/waitapproval.go +++ b/pkg/app/piped/executor/waitapproval/waitapproval.go @@ -24,6 +24,7 @@ import ( "go.uber.org/zap" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" + "github.com/pipe-cd/pipecd/pkg/app/piped/executor/skipstage" "github.com/pipe-cd/pipecd/pkg/config" "github.com/pipe-cd/pipecd/pkg/model" ) @@ -53,7 +54,7 @@ func Register(r registerer) { // Execute starts waiting until an approval from one of the specified users. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := executor.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitApprovalStageOptions.SkipOptions) + skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitApprovalStageOptions.SkipOptions) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE From 06641de1f251df52695d12a80c476b09aa0ad011 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 11:29:49 +0900 Subject: [PATCH 22/46] Add SkipOptions to ScriptRunStageOptions Signed-off-by: t-kikuc --- pkg/config/application.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/config/application.go b/pkg/config/application.go index 33bc6f3778..b5cd93f349 100644 --- a/pkg/config/application.go +++ b/pkg/config/application.go @@ -503,10 +503,11 @@ func (a *AnalysisStageOptions) Validate() error { // ScriptRunStageOptions contains all configurable values for a SCRIPT_RUN stage. type ScriptRunStageOptions struct { - Env map[string]string `json:"env"` - Run string `json:"run"` - Timeout Duration `json:"timeout" default:"6h"` - OnRollback string `json:"onRollback"` + Env map[string]string `json:"env"` + Run string `json:"run"` + Timeout Duration `json:"timeout" default:"6h"` + OnRollback string `json:"onRollback"` + SkipOptions SkipOptions `json:"skipOn,omitempty"` } // Validate checks the required fields of ScriptRunStageOptions. From fbb350f8c05fe82d1e01f244b5259c5ad368c7c2 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 11:31:07 +0900 Subject: [PATCH 23/46] rename 'SkipOptions' to 'SkipOn' to match with yaml Signed-off-by: t-kikuc --- pkg/app/piped/executor/analysis/analysis.go | 2 +- pkg/app/piped/executor/wait/wait.go | 2 +- .../executor/waitapproval/waitapproval.go | 2 +- pkg/config/application.go | 18 +++++++++--------- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/app/piped/executor/analysis/analysis.go b/pkg/app/piped/executor/analysis/analysis.go index 13826531b1..e010798f4e 100644 --- a/pkg/app/piped/executor/analysis/analysis.go +++ b/pkg/app/piped/executor/analysis/analysis.go @@ -64,7 +64,7 @@ func Register(r registerer) { // Any of those fail then the stage ends with failure. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.AnalysisStageOptions.SkipOptions) + skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.AnalysisStageOptions.SkipOn) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE diff --git a/pkg/app/piped/executor/wait/wait.go b/pkg/app/piped/executor/wait/wait.go index 716b0d5686..8f3581ddad 100644 --- a/pkg/app/piped/executor/wait/wait.go +++ b/pkg/app/piped/executor/wait/wait.go @@ -53,7 +53,7 @@ func Register(r registerer) { // Execute starts waiting for the specified duration. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitStageOptions.SkipOptions) + skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitStageOptions.SkipOn) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE diff --git a/pkg/app/piped/executor/waitapproval/waitapproval.go b/pkg/app/piped/executor/waitapproval/waitapproval.go index 3a92b9c879..983b5b770c 100644 --- a/pkg/app/piped/executor/waitapproval/waitapproval.go +++ b/pkg/app/piped/executor/waitapproval/waitapproval.go @@ -54,7 +54,7 @@ func Register(r registerer) { // Execute starts waiting until an approval from one of the specified users. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { // Skip the stage if needed based on the skip config. - skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitApprovalStageOptions.SkipOptions) + skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitApprovalStageOptions.SkipOn) if err != nil { e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) return model.StageStatus_STAGE_FAILURE diff --git a/pkg/config/application.go b/pkg/config/application.go index b5cd93f349..d34599da83 100644 --- a/pkg/config/application.go +++ b/pkg/config/application.go @@ -412,8 +412,8 @@ type SkipOptions struct { // WaitStageOptions contains all configurable values for a WAIT stage. type WaitStageOptions struct { - Duration Duration `json:"duration"` - SkipOptions SkipOptions `json:"skipOn,omitempty"` + Duration Duration `json:"duration"` + SkipOn SkipOptions `json:"skipOn,omitempty"` } // WaitStageOptions contains all configurable values for a WAIT_APPROVAL stage. @@ -423,7 +423,7 @@ type WaitApprovalStageOptions struct { Timeout Duration `json:"timeout" default:"6h"` Approvers []string `json:"approvers"` MinApproverNum int `json:"minApproverNum" default:"1"` - SkipOptions SkipOptions `json:"skipOn,omitempty"` + SkipOn SkipOptions `json:"skipOn,omitempty"` } func (w *WaitApprovalStageOptions) Validate() error { @@ -456,7 +456,7 @@ type AnalysisStageOptions struct { Metrics []TemplatableAnalysisMetrics `json:"metrics,omitempty"` Logs []TemplatableAnalysisLog `json:"logs,omitempty"` HTTPS []TemplatableAnalysisHTTP `json:"https,omitempty"` - SkipOptions SkipOptions `json:"skipOn,omitempty"` + SkipOn SkipOptions `json:"skipOn,omitempty"` } func (a *AnalysisStageOptions) Validate() error { @@ -503,11 +503,11 @@ func (a *AnalysisStageOptions) Validate() error { // ScriptRunStageOptions contains all configurable values for a SCRIPT_RUN stage. type ScriptRunStageOptions struct { - Env map[string]string `json:"env"` - Run string `json:"run"` - Timeout Duration `json:"timeout" default:"6h"` - OnRollback string `json:"onRollback"` - SkipOptions SkipOptions `json:"skipOn,omitempty"` + Env map[string]string `json:"env"` + Run string `json:"run"` + Timeout Duration `json:"timeout" default:"6h"` + OnRollback string `json:"onRollback"` + SkipOn SkipOptions `json:"skipOn,omitempty"` } // Validate checks the required fields of ScriptRunStageOptions. From 817830a339ce724a888a8b8cc6c9ad99ef546cd4 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 11:47:37 +0900 Subject: [PATCH 24/46] Move skip logic to scheduler Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 32 +++++++++++++++++++ pkg/app/piped/executor/analysis/analysis.go | 11 ------- pkg/app/piped/executor/wait/wait.go | 11 ------- .../executor/waitapproval/waitapproval.go | 11 ------- 4 files changed, 32 insertions(+), 33 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 61daf1f12c..c9a6fb4555 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -29,6 +29,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/app/piped/deploysource" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" "github.com/pipe-cd/pipecd/pkg/app/piped/executor/registry" + "github.com/pipe-cd/pipecd/pkg/app/piped/executor/skipstage" "github.com/pipe-cd/pipecd/pkg/app/piped/logpersister" "github.com/pipe-cd/pipecd/pkg/app/piped/metadatastore" pln "github.com/pipe-cd/pipecd/pkg/app/piped/planner" @@ -537,6 +538,18 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage return model.StageStatus_STAGE_FAILURE } + // Skip the stage if needed based on the skip config. + skip, err := s.checkSkipStage(sig.Context(), input) + if err != nil { + lp.Errorf("failed to check whether skipping the stage", zap.Error(err)) + s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires) + return model.StageStatus_STAGE_FAILURE + } + if skip { + lp.Infof("The stage was successfully skipped due to the skip configuration of the stage.") + return model.StageStatus_STAGE_SKIPPED + } + // Start running executor. status := ex.Execute(sig) @@ -687,6 +700,25 @@ func (s *scheduler) reportDeploymentCompleted(ctx context.Context, status model. return err } +func (s *scheduler) checkSkipStage(ctx context.Context, in executor.Input) (skip bool, err error) { + stageConfig := in.StageConfig + var skipOptions config.SkipOptions + switch stageConfig.Name { + case model.StageAnalysis: + skipOptions = stageConfig.AnalysisStageOptions.SkipOn + case model.StageWait: + skipOptions = stageConfig.WaitStageOptions.SkipOn + case model.StageWaitApproval: + skipOptions = stageConfig.WaitApprovalStageOptions.SkipOn + case model.StageScriptRun: + skipOptions = stageConfig.ScriptRunStageOptions.SkipOn + default: + return false, nil + } + + return skipstage.CheckSkipStage(ctx, in, skipOptions) +} + func (s *scheduler) getMentionedAccounts(event model.NotificationEventType) ([]string, error) { n, ok := s.metadataStore.Shared().Get(model.MetadataKeyDeploymentNotification) if !ok { diff --git a/pkg/app/piped/executor/analysis/analysis.go b/pkg/app/piped/executor/analysis/analysis.go index e010798f4e..328ad5a249 100644 --- a/pkg/app/piped/executor/analysis/analysis.go +++ b/pkg/app/piped/executor/analysis/analysis.go @@ -29,7 +29,6 @@ import ( "github.com/pipe-cd/pipecd/pkg/app/piped/analysisprovider/metrics" metricsfactory "github.com/pipe-cd/pipecd/pkg/app/piped/analysisprovider/metrics/factory" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" - "github.com/pipe-cd/pipecd/pkg/app/piped/executor/skipstage" "github.com/pipe-cd/pipecd/pkg/config" "github.com/pipe-cd/pipecd/pkg/model" ) @@ -63,16 +62,6 @@ func Register(r registerer) { // Execute spawns and runs multiple analyzer that run a query at the regular time. // Any of those fail then the stage ends with failure. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { - // Skip the stage if needed based on the skip config. - skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.AnalysisStageOptions.SkipOn) - if err != nil { - e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) - return model.StageStatus_STAGE_FAILURE - } - if skip { - return model.StageStatus_STAGE_SKIPPED - } - e.startTime = time.Now() ctx := sig.Context() options := e.StageConfig.AnalysisStageOptions diff --git a/pkg/app/piped/executor/wait/wait.go b/pkg/app/piped/executor/wait/wait.go index 8f3581ddad..db472bcbd1 100644 --- a/pkg/app/piped/executor/wait/wait.go +++ b/pkg/app/piped/executor/wait/wait.go @@ -22,7 +22,6 @@ import ( "go.uber.org/zap" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" - "github.com/pipe-cd/pipecd/pkg/app/piped/executor/skipstage" "github.com/pipe-cd/pipecd/pkg/model" ) @@ -52,16 +51,6 @@ func Register(r registerer) { // Execute starts waiting for the specified duration. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { - // Skip the stage if needed based on the skip config. - skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitStageOptions.SkipOn) - if err != nil { - e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) - return model.StageStatus_STAGE_FAILURE - } - if skip { - return model.StageStatus_STAGE_SKIPPED - } - var ( originalStatus = e.Stage.Status duration = defaultDuration diff --git a/pkg/app/piped/executor/waitapproval/waitapproval.go b/pkg/app/piped/executor/waitapproval/waitapproval.go index 983b5b770c..bd69460903 100644 --- a/pkg/app/piped/executor/waitapproval/waitapproval.go +++ b/pkg/app/piped/executor/waitapproval/waitapproval.go @@ -24,7 +24,6 @@ import ( "go.uber.org/zap" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" - "github.com/pipe-cd/pipecd/pkg/app/piped/executor/skipstage" "github.com/pipe-cd/pipecd/pkg/config" "github.com/pipe-cd/pipecd/pkg/model" ) @@ -53,16 +52,6 @@ func Register(r registerer) { // Execute starts waiting until an approval from one of the specified users. func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { - // Skip the stage if needed based on the skip config. - skip, err := skipstage.CheckSkipStage(sig.Context(), e.Input, e.StageConfig.WaitApprovalStageOptions.SkipOn) - if err != nil { - e.Logger.Error("failed to check whether skipping the stage", zap.Error(err)) - return model.StageStatus_STAGE_FAILURE - } - if skip { - return model.StageStatus_STAGE_SKIPPED - } - var ( originalStatus = e.Stage.Status ctx = sig.Context() From 94618f9d84fb75594eee4857d6a0f731e3f99d09 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 11:52:12 +0900 Subject: [PATCH 25/46] Add missing reporting Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index c9a6fb4555..164048e999 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -547,6 +547,7 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage } if skip { lp.Infof("The stage was successfully skipped due to the skip configuration of the stage.") + s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_SKIPPED, ps.Requires) return model.StageStatus_STAGE_SKIPPED } From 5b3585e9538b42b4dc78572358d9868430f83f4b Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 12:34:58 +0900 Subject: [PATCH 26/46] fix nits Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage/skipstage.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/app/piped/executor/skipstage/skipstage.go b/pkg/app/piped/executor/skipstage/skipstage.go index f8ebdec9cf..d82fd33f8e 100644 --- a/pkg/app/piped/executor/skipstage/skipstage.go +++ b/pkg/app/piped/executor/skipstage/skipstage.go @@ -26,19 +26,19 @@ import ( // CheckSkipStage checks whether the stage should be skipped or not. func CheckSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptions) (skip bool, err error) { - if opt.Paths == nil && len(opt.CommitMessagePrefixes) == 0 { - // When no condition is specified for skipping. + if len(opt.Paths) == 0 && len(opt.CommitMessagePrefixes) == 0 { + // When no condition is specified. return false, nil } repoCfg := in.Application.GitPath.Repo - clonedRepo, err := in.GitClient.Clone(ctx, repoCfg.Id, repoCfg.Remote, repoCfg.Branch, "") + repo, err := in.GitClient.Clone(ctx, repoCfg.Id, repoCfg.Remote, repoCfg.Branch, "") if err != nil { return false, err } // Check by path pattern - skip, err = skipByPathPattern(ctx, opt, clonedRepo, in.RunningDSP.Revision(), in.TargetDSP.Revision()) + skip, err = skipByPathPattern(ctx, opt, repo, in.RunningDSP.Revision(), in.TargetDSP.Revision()) if err != nil { return false, err } @@ -46,8 +46,8 @@ func CheckSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptio return true, nil } - // Check by prefix of Git commit message - skip, err = skipByCommitMessagePrefixes(ctx, opt, clonedRepo, in.TargetDSP.Revision()) + // Check by prefix of commit message + skip, err = skipByCommitMessagePrefixes(ctx, opt, repo, in.TargetDSP.Revision()) return skip, err } From 5c46fe531c33874a5dda843815c43adb32e1c288 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 15:12:05 +0900 Subject: [PATCH 27/46] Fix check logic Signed-off-by: t-kikuc --- pkg/app/piped/executor/skipstage/skipstage.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/executor/skipstage/skipstage.go b/pkg/app/piped/executor/skipstage/skipstage.go index d82fd33f8e..0a04a05b47 100644 --- a/pkg/app/piped/executor/skipstage/skipstage.go +++ b/pkg/app/piped/executor/skipstage/skipstage.go @@ -52,7 +52,7 @@ func CheckSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptio } func skipByCommitMessagePrefixes(ctx context.Context, opt config.SkipOptions, repo git.Repo, targetRev string) (skip bool, err error) { - if len(opt.CommitMessagePrefixes) > 0 { + if len(opt.CommitMessagePrefixes) == 0 { return false, nil } @@ -74,7 +74,7 @@ func commitMessageHasAnyPrefix(commitMessage string, prefixes []string) bool { } func skipByPathPattern(ctx context.Context, opt config.SkipOptions, repo git.Repo, runningRev, targetRev string) (skip bool, err error) { - if opt.Paths == nil { + if len(opt.Paths) == 0 { return false, nil } From 2e4c9df5a6284789a1d8f8d85c1375ee6e110d77 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 15:13:47 +0900 Subject: [PATCH 28/46] Add missing error handling Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 164048e999..1ab68c41dc 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -542,12 +542,16 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage skip, err := s.checkSkipStage(sig.Context(), input) if err != nil { lp.Errorf("failed to check whether skipping the stage", zap.Error(err)) - s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires) + if err := s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires); err != nil { + s.logger.Error("failed to report stage status", zap.Error(err)) + } return model.StageStatus_STAGE_FAILURE } if skip { lp.Infof("The stage was successfully skipped due to the skip configuration of the stage.") - s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_SKIPPED, ps.Requires) + if err := s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_SKIPPED, ps.Requires); err != nil { + s.logger.Error("failed to report stage status", zap.Error(err)) + } return model.StageStatus_STAGE_SKIPPED } From a687cd73880d6a7872161624e902eadcf9a41238 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 15:45:55 +0900 Subject: [PATCH 29/46] Move skipstage to controller Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 2 +- pkg/app/piped/{executor => controller}/skipstage/skipstage.go | 0 .../piped/{executor => controller}/skipstage/skipstage_test.go | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename pkg/app/piped/{executor => controller}/skipstage/skipstage.go (100%) rename pkg/app/piped/{executor => controller}/skipstage/skipstage_test.go (100%) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 1ab68c41dc..719d33fcf1 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -26,10 +26,10 @@ import ( "go.uber.org/zap" "github.com/pipe-cd/pipecd/pkg/app/piped/controller/controllermetrics" + "github.com/pipe-cd/pipecd/pkg/app/piped/controller/skipstage" "github.com/pipe-cd/pipecd/pkg/app/piped/deploysource" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" "github.com/pipe-cd/pipecd/pkg/app/piped/executor/registry" - "github.com/pipe-cd/pipecd/pkg/app/piped/executor/skipstage" "github.com/pipe-cd/pipecd/pkg/app/piped/logpersister" "github.com/pipe-cd/pipecd/pkg/app/piped/metadatastore" pln "github.com/pipe-cd/pipecd/pkg/app/piped/planner" diff --git a/pkg/app/piped/executor/skipstage/skipstage.go b/pkg/app/piped/controller/skipstage/skipstage.go similarity index 100% rename from pkg/app/piped/executor/skipstage/skipstage.go rename to pkg/app/piped/controller/skipstage/skipstage.go diff --git a/pkg/app/piped/executor/skipstage/skipstage_test.go b/pkg/app/piped/controller/skipstage/skipstage_test.go similarity index 100% rename from pkg/app/piped/executor/skipstage/skipstage_test.go rename to pkg/app/piped/controller/skipstage/skipstage_test.go From 431682c70b31c38f18fc39603ea0769c330b144f Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 17:16:42 +0900 Subject: [PATCH 30/46] Update docs Signed-off-by: t-kikuc --- .../en/docs-dev/user-guide/configuration-reference.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/content/en/docs-dev/user-guide/configuration-reference.md b/docs/content/en/docs-dev/user-guide/configuration-reference.md index 56b8210b30..35804d86b5 100644 --- a/docs/content/en/docs-dev/user-guide/configuration-reference.md +++ b/docs/content/en/docs-dev/user-guide/configuration-reference.md @@ -578,6 +578,13 @@ Note: The available values are identical to those found in the aws-sdk-go-v2 Typ | Field | Type | Description | Required | |-|-|-|-| +## SkipOptions + +| Field | Type | Description | Required | +|-|-|-|-| +| commitMessagePrefixes | []string | List of prefixes. When the prefix of the commit's message matches any of them, the stage will be skipped. Empty means the stage will not be skipped by this condition. | No | +| paths | []string | List of directories or files. When all changes of the commit match them, the stage will be skipped. Empty means the stage will not be skipped by this condition. Regular expression can be used. | No | + ## StageOptions ### KubernetesPrimaryRolloutStageOptions @@ -685,12 +692,14 @@ Note: By default, the sum of traffic is rounded to 100. If both `primary` and `c |-|-|-|-| | duration | duration | Maximum time to perform the analysis. | Yes | | metrics | [][AnalysisMetrics](#analysismetrics) | Configuration for analysis by metrics. | No | +| skipOn | [SkipOptions](#skipoptions) | When to skip this stage. | No | ### WaitStageOptions | Field | Type | Description | Required | |-|-|-|-| | duration | duration | Time to wait. | Yes | +| skipOn | [SkipOptions](#skipoptions) | When to skip this stage. | No | ### WaitApprovalStageOptions @@ -699,6 +708,7 @@ Note: By default, the sum of traffic is rounded to 100. If both `primary` and `c | timeout | duration | The maximum length of time to wait before giving up. Default is 6h. | No | | approvers | []string | List of username who has permission to approve. | Yes | | minApproverNum | int | Number of minimum needed approvals to make this stage complete. Default is 1. | No | +| skipOn | [SkipOptions](#skipoptions) | When to skip this stage. | No | ### CustomSyncStageOptions (deprecated) | Field | Type | Description | Required | @@ -713,6 +723,7 @@ Note: By default, the sum of traffic is rounded to 100. If both `primary` and `c | run | string | Script run on this stage. | Yes | | env | map[string]string | Environment variables used with scripts. | No | | timeout | duration | The maximum time the stage can be taken to run. Default is `6h`| No | +| skipOn | [SkipOptions](#skipoptions) | When to skip this stage. | No | ## PostSync From 3f0c00796221ef33ad77e6c43f0d8d5e6a8bcd80 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 22 May 2024 22:34:52 +0900 Subject: [PATCH 31/46] gen mock by 'make gen/code' Signed-off-by: t-kikuc --- pkg/git/gittest/git.mock.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/git/gittest/git.mock.go b/pkg/git/gittest/git.mock.go index 62e5cc58be..9bb0588456 100644 --- a/pkg/git/gittest/git.mock.go +++ b/pkg/git/gittest/git.mock.go @@ -135,6 +135,21 @@ func (mr *MockRepoMockRecorder) GetClonedBranch() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetClonedBranch", reflect.TypeOf((*MockRepo)(nil).GetClonedBranch)) } +// GetCommitForRev mocks base method. +func (m *MockRepo) GetCommitForRev(arg0 context.Context, arg1 string) (git.Commit, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetCommitForRev", arg0, arg1) + ret0, _ := ret[0].(git.Commit) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetCommitForRev indicates an expected call of GetCommitForRev. +func (mr *MockRepoMockRecorder) GetCommitForRev(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCommitForRev", reflect.TypeOf((*MockRepo)(nil).GetCommitForRev), arg0, arg1) +} + // GetCommitHashForRev mocks base method. func (m *MockRepo) GetCommitHashForRev(arg0 context.Context, arg1 string) (string, error) { m.ctrl.T.Helper() From b465c662ffd878436735490cc3abdb88ff686fe6 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:00:07 +0900 Subject: [PATCH 32/46] nits: fix from 'fromCmd' to 'byCmd' Signed-off-by: t-kikuc --- pkg/app/piped/executor/analysis/analysis.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/executor/analysis/analysis.go b/pkg/app/piped/executor/analysis/analysis.go index 328ad5a249..775e4fb606 100644 --- a/pkg/app/piped/executor/analysis/analysis.go +++ b/pkg/app/piped/executor/analysis/analysis.go @@ -112,7 +112,7 @@ func (e *Executor) Execute(sig executor.StopSignal) model.StageStatus { for { select { case <-ticker.C: - if !e.checkSkippedFromCmd(ctx) { + if !e.checkSkippedByCmd(ctx) { continue } status = model.StageStatus_STAGE_SKIPPED @@ -339,7 +339,7 @@ func (e *Executor) buildAppArgs(customArgs map[string]string) argsTemplate { return args } -func (e *Executor) checkSkippedFromCmd(ctx context.Context) bool { +func (e *Executor) checkSkippedByCmd(ctx context.Context) bool { var skipCmd *model.ReportableCommand commands := e.CommandLister.ListCommands() From 674ea92ccc496abad7f32ed0d26684e25dcd4097 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:01:16 +0900 Subject: [PATCH 33/46] nits: rename func Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 719d33fcf1..3fc29093d7 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -539,7 +539,7 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage } // Skip the stage if needed based on the skip config. - skip, err := s.checkSkipStage(sig.Context(), input) + skip, err := s.shouldSkipStage(sig.Context(), input) if err != nil { lp.Errorf("failed to check whether skipping the stage", zap.Error(err)) if err := s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires); err != nil { @@ -705,7 +705,7 @@ func (s *scheduler) reportDeploymentCompleted(ctx context.Context, status model. return err } -func (s *scheduler) checkSkipStage(ctx context.Context, in executor.Input) (skip bool, err error) { +func (s *scheduler) shouldSkipStage(ctx context.Context, in executor.Input) (skip bool, err error) { stageConfig := in.StageConfig var skipOptions config.SkipOptions switch stageConfig.Name { From ea2858ad58be41ad39c5288f2a30682da1119ce8 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:04:00 +0900 Subject: [PATCH 34/46] nits: refine configuration-reference Signed-off-by: t-kikuc --- .../content/en/docs-dev/user-guide/configuration-reference.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/content/en/docs-dev/user-guide/configuration-reference.md b/docs/content/en/docs-dev/user-guide/configuration-reference.md index 35804d86b5..0d1c7f7122 100644 --- a/docs/content/en/docs-dev/user-guide/configuration-reference.md +++ b/docs/content/en/docs-dev/user-guide/configuration-reference.md @@ -582,8 +582,8 @@ Note: The available values are identical to those found in the aws-sdk-go-v2 Typ | Field | Type | Description | Required | |-|-|-|-| -| commitMessagePrefixes | []string | List of prefixes. When the prefix of the commit's message matches any of them, the stage will be skipped. Empty means the stage will not be skipped by this condition. | No | -| paths | []string | List of directories or files. When all changes of the commit match them, the stage will be skipped. Empty means the stage will not be skipped by this condition. Regular expression can be used. | No | +| commitMessagePrefixes | []string | List of commit message's prefixes. The stage will be skipped when the prefix of the commit's message matches any of them. Empty means the stage will not be skipped by this condition. | No | +| paths | []string | List of paths to directories or files. When all commit changes match them, the stage will be skipped. Empty means the stage will not be skipped by this condition. Regular expression can be used. | No | ## StageOptions From fb4d46b2220343a55f6ee0ffb5ca028941c61e07 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:08:12 +0900 Subject: [PATCH 35/46] fix error logging of zap.Error(err) Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 3fc29093d7..7daba620c6 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -541,7 +541,7 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage // Skip the stage if needed based on the skip config. skip, err := s.shouldSkipStage(sig.Context(), input) if err != nil { - lp.Errorf("failed to check whether skipping the stage", zap.Error(err)) + lp.Errorf("failed to check whether skipping the stage: %w", err.Error()) if err := s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires); err != nil { s.logger.Error("failed to report stage status", zap.Error(err)) } From c01b66771c9756bbdd44f7f4508d39028821d3a8 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:11:22 +0900 Subject: [PATCH 36/46] fix the flow in error handling Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 7daba620c6..64fbcf45cc 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -548,10 +548,11 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage return model.StageStatus_STAGE_FAILURE } if skip { - lp.Infof("The stage was successfully skipped due to the skip configuration of the stage.") if err := s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_SKIPPED, ps.Requires); err != nil { s.logger.Error("failed to report stage status", zap.Error(err)) + return model.StageStatus_STAGE_FAILURE } + lp.Infof("The stage was successfully skipped due to the skip configuration of the stage.") return model.StageStatus_STAGE_SKIPPED } From 6fe1325684df4b4db0263736a80d3ca083125455 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:12:12 +0900 Subject: [PATCH 37/46] Fix: check skipping before creating executor Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 64fbcf45cc..9a85b75c01 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -529,15 +529,6 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage Notifier: s.notifier, } - // Find the executor for this stage. - ex, ok := executorFactory(input) - if !ok { - err := fmt.Errorf("no registered executor for stage %s", ps.Name) - lp.Error(err.Error()) - s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires) - return model.StageStatus_STAGE_FAILURE - } - // Skip the stage if needed based on the skip config. skip, err := s.shouldSkipStage(sig.Context(), input) if err != nil { @@ -556,6 +547,15 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage return model.StageStatus_STAGE_SKIPPED } + // Find the executor for this stage. + ex, ok := executorFactory(input) + if !ok { + err := fmt.Errorf("no registered executor for stage %s", ps.Name) + lp.Error(err.Error()) + s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires) + return model.StageStatus_STAGE_FAILURE + } + // Start running executor. status := ex.Execute(sig) From 4e67ccb0acd00a7100a443b823ca3455409c1f6f Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:28:24 +0900 Subject: [PATCH 38/46] Remove unused func 'GetCommitHashForRev' Signed-off-by: t-kikuc --- pkg/git/gittest/git.mock.go | 15 --------------- pkg/git/repo.go | 11 ----------- pkg/git/repo_test.go | 37 +++++-------------------------------- 3 files changed, 5 insertions(+), 58 deletions(-) diff --git a/pkg/git/gittest/git.mock.go b/pkg/git/gittest/git.mock.go index 9bb0588456..fbb260bdc3 100644 --- a/pkg/git/gittest/git.mock.go +++ b/pkg/git/gittest/git.mock.go @@ -150,21 +150,6 @@ func (mr *MockRepoMockRecorder) GetCommitForRev(arg0, arg1 interface{}) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCommitForRev", reflect.TypeOf((*MockRepo)(nil).GetCommitForRev), arg0, arg1) } -// GetCommitHashForRev mocks base method. -func (m *MockRepo) GetCommitHashForRev(arg0 context.Context, arg1 string) (string, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetCommitHashForRev", arg0, arg1) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetCommitHashForRev indicates an expected call of GetCommitHashForRev. -func (mr *MockRepoMockRecorder) GetCommitHashForRev(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCommitHashForRev", reflect.TypeOf((*MockRepo)(nil).GetCommitHashForRev), arg0, arg1) -} - // GetLatestCommit mocks base method. func (m *MockRepo) GetLatestCommit(arg0 context.Context) (git.Commit, error) { m.ctrl.T.Helper() diff --git a/pkg/git/repo.go b/pkg/git/repo.go index dc1911e72a..115f84e0be 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -38,7 +38,6 @@ type Repo interface { ListCommits(ctx context.Context, visionRange string) ([]Commit, error) GetLatestCommit(ctx context.Context) (Commit, error) - GetCommitHashForRev(ctx context.Context, rev string) (string, error) GetCommitForRev(ctx context.Context, rev string) (Commit, error) ChangedFiles(ctx context.Context, from, to string) ([]string, error) Checkout(ctx context.Context, commitish string) error @@ -131,16 +130,6 @@ func (r *repo) GetLatestCommit(ctx context.Context) (Commit, error) { return commits[0], nil } -// GetCommitHashForRev returns the hash value of the commit for a given rev. -func (r *repo) GetCommitHashForRev(ctx context.Context, rev string) (string, error) { - out, err := r.runGitCommand(ctx, "rev-parse", rev) - if err != nil { - return "", formatCommandError(err, out) - } - - return strings.TrimSpace(string(out)), nil -} - // GetCommitFromRev returns the commit for the given rev. func (r *repo) GetCommitForRev(ctx context.Context, rev string) (Commit, error) { args := []string{ diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index e07b739aca..576f565a3d 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -28,33 +28,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestGetCommitHashForRev(t *testing.T) { - faker, err := newFaker() - require.NoError(t, err) - defer faker.clean() - - var ( - org = "test-repo-org" - repoName = "repo-get-commit-hash-for-rev" - ctx = context.Background() - ) - - err = faker.makeRepo(org, repoName) - require.NoError(t, err) - r := &repo{ - dir: faker.repoDir(org, repoName), - gitPath: faker.gitPath, - } - - commits, err := r.ListCommits(ctx, "") - require.NoError(t, err) - assert.Equal(t, 1, len(commits)) - - latestCommitHash, err := r.GetCommitHashForRev(ctx, "HEAD") - require.NoError(t, err) - assert.Equal(t, commits[0].Hash, latestCommitHash) -} - func TestChangedFiles(t *testing.T) { faker, err := newFaker() require.NoError(t, err) @@ -73,9 +46,9 @@ func TestChangedFiles(t *testing.T) { gitPath: faker.gitPath, } - previousCommitHash, err := r.GetCommitHashForRev(ctx, "HEAD") + previousCommit, err := r.GetCommitForRev(ctx, "HEAD") require.NoError(t, err) - require.NotEqual(t, "", previousCommitHash) + require.NotEqual(t, "", previousCommit.Hash) err = os.MkdirAll(filepath.Join(r.dir, "new-dir"), os.ModePerm) require.NoError(t, err) @@ -90,11 +63,11 @@ func TestChangedFiles(t *testing.T) { err = r.addCommit(ctx, "Added new file") require.NoError(t, err) - headCommitHash, err := r.GetCommitHashForRev(ctx, "HEAD") + headCommit, err := r.GetCommitForRev(ctx, "HEAD") require.NoError(t, err) - require.NotEqual(t, "", headCommitHash) + require.NotEqual(t, "", headCommit.Hash) - changedFiles, err := r.ChangedFiles(ctx, previousCommitHash, headCommitHash) + changedFiles, err := r.ChangedFiles(ctx, previousCommit.Hash, headCommit.Hash) sort.Strings(changedFiles) expectedChangedFiles := []string{ "new-dir/new-file.txt", From e2e35f46f4f0fe56cee9c4126b4e34e8f4cac64d Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:40:20 +0900 Subject: [PATCH 39/46] fix: remove unnecessary formatting Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 9a85b75c01..66dbea803b 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -543,7 +543,7 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage s.logger.Error("failed to report stage status", zap.Error(err)) return model.StageStatus_STAGE_FAILURE } - lp.Infof("The stage was successfully skipped due to the skip configuration of the stage.") + lp.Info("The stage was successfully skipped due to the skip configuration of the stage.") return model.StageStatus_STAGE_SKIPPED } From cab353926c7ff57a738d1a38b526200084f2aa49 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:53:40 +0900 Subject: [PATCH 40/46] refactor: quit separating 'skipstage' package Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 3 +-- pkg/app/piped/controller/{skipstage => }/skipstage.go | 2 +- pkg/app/piped/controller/{skipstage => }/skipstage_test.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) rename pkg/app/piped/controller/{skipstage => }/skipstage.go (99%) rename pkg/app/piped/controller/{skipstage => }/skipstage_test.go (99%) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 66dbea803b..a45421113d 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -26,7 +26,6 @@ import ( "go.uber.org/zap" "github.com/pipe-cd/pipecd/pkg/app/piped/controller/controllermetrics" - "github.com/pipe-cd/pipecd/pkg/app/piped/controller/skipstage" "github.com/pipe-cd/pipecd/pkg/app/piped/deploysource" "github.com/pipe-cd/pipecd/pkg/app/piped/executor" "github.com/pipe-cd/pipecd/pkg/app/piped/executor/registry" @@ -722,7 +721,7 @@ func (s *scheduler) shouldSkipStage(ctx context.Context, in executor.Input) (ski return false, nil } - return skipstage.CheckSkipStage(ctx, in, skipOptions) + return CheckSkipStage(ctx, in, skipOptions) } func (s *scheduler) getMentionedAccounts(event model.NotificationEventType) ([]string, error) { diff --git a/pkg/app/piped/controller/skipstage/skipstage.go b/pkg/app/piped/controller/skipstage.go similarity index 99% rename from pkg/app/piped/controller/skipstage/skipstage.go rename to pkg/app/piped/controller/skipstage.go index 0a04a05b47..92a67f438d 100644 --- a/pkg/app/piped/controller/skipstage/skipstage.go +++ b/pkg/app/piped/controller/skipstage.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package skipstage +package controller import ( "context" diff --git a/pkg/app/piped/controller/skipstage/skipstage_test.go b/pkg/app/piped/controller/skipstage_test.go similarity index 99% rename from pkg/app/piped/controller/skipstage/skipstage_test.go rename to pkg/app/piped/controller/skipstage_test.go index bf118e60ae..c54bf67c86 100644 --- a/pkg/app/piped/controller/skipstage/skipstage_test.go +++ b/pkg/app/piped/controller/skipstage_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package skipstage +package controller import ( "testing" From 7eda335753b450a3021b05fdc223ca61ff74fee4 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:58:04 +0900 Subject: [PATCH 41/46] fix: make 'checkSkipStage()' private after refactoring Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 2 +- pkg/app/piped/controller/skipstage.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index a45421113d..0d54fd75f0 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -721,7 +721,7 @@ func (s *scheduler) shouldSkipStage(ctx context.Context, in executor.Input) (ski return false, nil } - return CheckSkipStage(ctx, in, skipOptions) + return checkSkipStage(ctx, in, skipOptions) } func (s *scheduler) getMentionedAccounts(event model.NotificationEventType) ([]string, error) { diff --git a/pkg/app/piped/controller/skipstage.go b/pkg/app/piped/controller/skipstage.go index 92a67f438d..3c8f0c76df 100644 --- a/pkg/app/piped/controller/skipstage.go +++ b/pkg/app/piped/controller/skipstage.go @@ -24,8 +24,8 @@ import ( "github.com/pipe-cd/pipecd/pkg/git" ) -// CheckSkipStage checks whether the stage should be skipped or not. -func CheckSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptions) (skip bool, err error) { +// checkSkipStage checks whether the stage should be skipped or not. +func checkSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptions) (skip bool, err error) { if len(opt.Paths) == 0 && len(opt.CommitMessagePrefixes) == 0 { // When no condition is specified. return false, nil From b9e8a2ad3cbbd1832afc8e5c8524bda63a362df8 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 30 May 2024 18:59:16 +0900 Subject: [PATCH 42/46] Fix: rename test func Signed-off-by: t-kikuc --- pkg/app/piped/controller/skipstage_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/app/piped/controller/skipstage_test.go b/pkg/app/piped/controller/skipstage_test.go index c54bf67c86..589126c05e 100644 --- a/pkg/app/piped/controller/skipstage_test.go +++ b/pkg/app/piped/controller/skipstage_test.go @@ -20,7 +20,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestSkipByCommitMessagePrefixes(t *testing.T) { +func TestCommitMessageHasAnyPrefix(t *testing.T) { t.Parallel() testcases := []struct { name string From 8a96a8c09fb01c1230275795fa3537e38574b7d7 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 10 Jun 2024 09:39:31 +0900 Subject: [PATCH 43/46] Refactor: merge func 'checkSkipStage' into 'shouldSkipStage' Signed-off-by: t-kikuc --- pkg/app/piped/controller/scheduler.go | 19 ------------------- pkg/app/piped/controller/skipstage.go | 24 ++++++++++++++++++++---- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 0d54fd75f0..59716990ff 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -705,25 +705,6 @@ func (s *scheduler) reportDeploymentCompleted(ctx context.Context, status model. return err } -func (s *scheduler) shouldSkipStage(ctx context.Context, in executor.Input) (skip bool, err error) { - stageConfig := in.StageConfig - var skipOptions config.SkipOptions - switch stageConfig.Name { - case model.StageAnalysis: - skipOptions = stageConfig.AnalysisStageOptions.SkipOn - case model.StageWait: - skipOptions = stageConfig.WaitStageOptions.SkipOn - case model.StageWaitApproval: - skipOptions = stageConfig.WaitApprovalStageOptions.SkipOn - case model.StageScriptRun: - skipOptions = stageConfig.ScriptRunStageOptions.SkipOn - default: - return false, nil - } - - return checkSkipStage(ctx, in, skipOptions) -} - func (s *scheduler) getMentionedAccounts(event model.NotificationEventType) ([]string, error) { n, ok := s.metadataStore.Shared().Get(model.MetadataKeyDeploymentNotification) if !ok { diff --git a/pkg/app/piped/controller/skipstage.go b/pkg/app/piped/controller/skipstage.go index 3c8f0c76df..b7a42ba428 100644 --- a/pkg/app/piped/controller/skipstage.go +++ b/pkg/app/piped/controller/skipstage.go @@ -22,11 +22,27 @@ import ( "github.com/pipe-cd/pipecd/pkg/config" "github.com/pipe-cd/pipecd/pkg/filematcher" "github.com/pipe-cd/pipecd/pkg/git" + "github.com/pipe-cd/pipecd/pkg/model" ) // checkSkipStage checks whether the stage should be skipped or not. -func checkSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptions) (skip bool, err error) { - if len(opt.Paths) == 0 && len(opt.CommitMessagePrefixes) == 0 { +func (s *scheduler) shouldSkipStage(ctx context.Context, in executor.Input) (skip bool, err error) { + stageConfig := in.StageConfig + var skipOptions config.SkipOptions + switch stageConfig.Name { + case model.StageAnalysis: + skipOptions = stageConfig.AnalysisStageOptions.SkipOn + case model.StageWait: + skipOptions = stageConfig.WaitStageOptions.SkipOn + case model.StageWaitApproval: + skipOptions = stageConfig.WaitApprovalStageOptions.SkipOn + case model.StageScriptRun: + skipOptions = stageConfig.ScriptRunStageOptions.SkipOn + default: + return false, nil + } + + if len(skipOptions.Paths) == 0 && len(skipOptions.CommitMessagePrefixes) == 0 { // When no condition is specified. return false, nil } @@ -38,7 +54,7 @@ func checkSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptio } // Check by path pattern - skip, err = skipByPathPattern(ctx, opt, repo, in.RunningDSP.Revision(), in.TargetDSP.Revision()) + skip, err = skipByPathPattern(ctx, skipOptions, repo, in.RunningDSP.Revision(), in.TargetDSP.Revision()) if err != nil { return false, err } @@ -47,7 +63,7 @@ func checkSkipStage(ctx context.Context, in executor.Input, opt config.SkipOptio } // Check by prefix of commit message - skip, err = skipByCommitMessagePrefixes(ctx, opt, repo, in.TargetDSP.Revision()) + skip, err = skipByCommitMessagePrefixes(ctx, skipOptions, repo, in.TargetDSP.Revision()) return skip, err } From 27e4acd0af243070a48bb4ad097026a9052ea37d Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 10 Jun 2024 18:03:31 +0900 Subject: [PATCH 44/46] Merge funcs Signed-off-by: t-kikuc --- pkg/app/piped/controller/skipstage.go | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/controller/skipstage.go b/pkg/app/piped/controller/skipstage.go index b7a42ba428..94c5303093 100644 --- a/pkg/app/piped/controller/skipstage.go +++ b/pkg/app/piped/controller/skipstage.go @@ -67,6 +67,7 @@ func (s *scheduler) shouldSkipStage(ctx context.Context, in executor.Input) (ski return skip, err } +// skipByCommitMessagePrefixes returns true if the commit message has ANY one of the specified prefixes. func skipByCommitMessagePrefixes(ctx context.Context, opt config.SkipOptions, repo git.Repo, targetRev string) (skip bool, err error) { if len(opt.CommitMessagePrefixes) == 0 { return false, nil @@ -77,7 +78,12 @@ func skipByCommitMessagePrefixes(ctx context.Context, opt config.SkipOptions, re return false, err } - return commitMessageHasAnyPrefix(commit.Message, opt.CommitMessagePrefixes), nil + for _, prefix := range opt.CommitMessagePrefixes { + if strings.HasPrefix(commit.Message, prefix) { + return true, nil + } + } + return false, nil } func commitMessageHasAnyPrefix(commitMessage string, prefixes []string) bool { @@ -89,6 +95,8 @@ func commitMessageHasAnyPrefix(commitMessage string, prefixes []string) bool { return false } +// skipByPathPattern returns true if and only if ALL changed files are included in `opt.Paths`. +// If ANY changed file does not match all `skipPatterns`, it returns false. func skipByPathPattern(ctx context.Context, opt config.SkipOptions, repo git.Repo, runningRev, targetRev string) (skip bool, err error) { if len(opt.Paths) == 0 { return false, nil @@ -98,7 +106,19 @@ func skipByPathPattern(ctx context.Context, opt config.SkipOptions, repo git.Rep if err != nil { return false, err } - return hasOnlyPathsToSkip(opt.Paths, changedFiles) + + matcher, err := filematcher.NewPatternMatcher(opt.Paths) + if err != nil { + return false, err + } + + for _, changedFile := range changedFiles { + if !matcher.Matches(changedFile) { + return false, nil + } + } + + return true, nil } // hasOnlyPathsToSkip returns true if and only if all changed files are included in `skipPatterns`. From 38fae58358fc13feede6fe0cdad79d8e31592cfa Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 10 Jun 2024 18:03:44 +0900 Subject: [PATCH 45/46] Add TestSkipByCommitMessagePrefixes with mock Signed-off-by: t-kikuc --- pkg/app/piped/controller/skipstage_test.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/controller/skipstage_test.go b/pkg/app/piped/controller/skipstage_test.go index 589126c05e..c0187f3e2f 100644 --- a/pkg/app/piped/controller/skipstage_test.go +++ b/pkg/app/piped/controller/skipstage_test.go @@ -15,12 +15,17 @@ package controller import ( + "context" "testing" + "github.com/golang/mock/gomock" + "github.com/pipe-cd/pipecd/pkg/config" + "github.com/pipe-cd/pipecd/pkg/git" + "github.com/pipe-cd/pipecd/pkg/git/gittest" "github.com/stretchr/testify/assert" ) -func TestCommitMessageHasAnyPrefix(t *testing.T) { +func TestSkipByCommitMessagePrefixes(t *testing.T) { t.Parallel() testcases := []struct { name string @@ -51,8 +56,19 @@ func TestCommitMessageHasAnyPrefix(t *testing.T) { for _, tc := range testcases { tc := tc t.Run(tc.name, func(t *testing.T) { - skip := commitMessageHasAnyPrefix(tc.commitMessage, tc.prefixes) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + repoMock := gittest.NewMockRepo(ctrl) + repoMock.EXPECT().GetCommitForRev(gomock.Any(), "test-rev").Return(git.Commit{ + Message: tc.commitMessage, + }, nil).AnyTimes() + + opt := config.SkipOptions{ + CommitMessagePrefixes: tc.prefixes, + } + skip, err := skipByCommitMessagePrefixes(context.Background(), opt, repoMock, "test-rev") assert.Equal(t, tc.skip, skip) + assert.NoError(t, err) }) } } From 0b074fb1e64a14d6f59bbc18de2ae23ebb1d61e1 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Mon, 10 Jun 2024 18:09:26 +0900 Subject: [PATCH 46/46] Refactore: merge funcs and use mock Signed-off-by: t-kikuc --- pkg/app/piped/controller/skipstage.go | 26 ---------------------- pkg/app/piped/controller/skipstage_test.go | 16 +++++++++---- 2 files changed, 12 insertions(+), 30 deletions(-) diff --git a/pkg/app/piped/controller/skipstage.go b/pkg/app/piped/controller/skipstage.go index 94c5303093..ccbd5633e1 100644 --- a/pkg/app/piped/controller/skipstage.go +++ b/pkg/app/piped/controller/skipstage.go @@ -86,15 +86,6 @@ func skipByCommitMessagePrefixes(ctx context.Context, opt config.SkipOptions, re return false, nil } -func commitMessageHasAnyPrefix(commitMessage string, prefixes []string) bool { - for _, prefix := range prefixes { - if strings.HasPrefix(commitMessage, prefix) { - return true - } - } - return false -} - // skipByPathPattern returns true if and only if ALL changed files are included in `opt.Paths`. // If ANY changed file does not match all `skipPatterns`, it returns false. func skipByPathPattern(ctx context.Context, opt config.SkipOptions, repo git.Repo, runningRev, targetRev string) (skip bool, err error) { @@ -120,20 +111,3 @@ func skipByPathPattern(ctx context.Context, opt config.SkipOptions, repo git.Rep return true, nil } - -// hasOnlyPathsToSkip returns true if and only if all changed files are included in `skipPatterns`. -// If any changed file does not match all `skipPatterns`, it returns false. -func hasOnlyPathsToSkip(skipPatterns []string, changedFiles []string) (bool, error) { - matcher, err := filematcher.NewPatternMatcher(skipPatterns) - if err != nil { - return false, err - } - - for _, changedFile := range changedFiles { - if !matcher.Matches(changedFile) { - return false, nil - } - } - - return true, nil -} diff --git a/pkg/app/piped/controller/skipstage_test.go b/pkg/app/piped/controller/skipstage_test.go index c0187f3e2f..074ad9fed1 100644 --- a/pkg/app/piped/controller/skipstage_test.go +++ b/pkg/app/piped/controller/skipstage_test.go @@ -59,7 +59,7 @@ func TestSkipByCommitMessagePrefixes(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() repoMock := gittest.NewMockRepo(ctrl) - repoMock.EXPECT().GetCommitForRev(gomock.Any(), "test-rev").Return(git.Commit{ + repoMock.EXPECT().GetCommitForRev(gomock.Any(), gomock.Any()).Return(git.Commit{ Message: tc.commitMessage, }, nil).AnyTimes() @@ -73,7 +73,7 @@ func TestSkipByCommitMessagePrefixes(t *testing.T) { } } -func TestHasOnlyPathsToSkip(t *testing.T) { +func TestSkipByPathPattern(t *testing.T) { t.Parallel() testcases := []struct { name string @@ -97,7 +97,7 @@ func TestHasOnlyPathsToSkip(t *testing.T) { name: "no skip patterns and no changed files", skipPatterns: nil, changedFiles: nil, - skip: true, + skip: false, }, { name: "skip pattern matches all changed files", @@ -141,7 +141,15 @@ func TestHasOnlyPathsToSkip(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { // We do not use t.Parallel() here due to https://pkg.go.dev/github.com/pipe-cd/pipecd/pkg/filematcher#PatternMatcher.Matches. - actual, err := hasOnlyPathsToSkip(tc.skipPatterns, tc.changedFiles) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + repoMock := gittest.NewMockRepo(ctrl) + repoMock.EXPECT().ChangedFiles(gomock.Any(), gomock.Any(), gomock.Any()).Return(tc.changedFiles, nil).AnyTimes() + + opt := config.SkipOptions{ + Paths: tc.skipPatterns, + } + actual, err := skipByPathPattern(context.Background(), opt, repoMock, "running-rev", "target-rev") assert.NoError(t, err) assert.Equal(t, tc.skip, actual) })