Support skipping stages by path pattern or commit message prefix

[t-kikuc]

What this PR does / why we need it:

This PR enables skipping stages (Analysis,WaitApproval,Wait,ScriptRun) when the commit matches configured path patterns or commit message prefixes.

Which issue(s) this PR fixes:

Fixes #4899

Does this PR introduce a user-facing change?: N/A (only append)

• How are users affected by this change: N/A
• Is this breaking change: N/A
• How to migrate (if breaking change): N/A

How to use

Configuration: add with.skipOn section in stages to skip.

• paths: When ALL changes of the commit match them, the stage will be skipped.
• commitMessagePrefixes: When the prefix of the commit's message matches ANY of them, the stage will be skipped.

  pipeline:
    stages:
      - name: ECS_TRAFFIC_ROUTING
        with:
          canary: 30
      - name: WAIT_APPROVAL
        with:
          skipOn:
            commitMessagePrefixes:
              - [skip-approval] # When the commit message starts with '[skip-approval]', this stage will be skipped
      - name: WAIT
        with:
          duration: 60s
          skipOn:
            paths:
              - "**/*.yaml" # Any yaml file under any directory
      - name: ANALYSIS
        with:
          duration: 60s
          skipOn:
            paths:
              - "**/app.pipecd.yaml" # Any app.pipecd.yaml under any directory
              - "**/*.md" # Any markdown file under any directory
      - name: ECS_TRAFFIC_ROUTING
        with:
          primary: 100

When all skipOn conditions of each stage passed, the deployment will be:

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 34.44444% with 59 lines in your changes missing coverage. Please review.

Project coverage is 28.49%. Comparing base (8129078) to head (e498822).

Files	Patch %	Lines
pkg/app/piped/controller/skipstage.go	33.87%	38 Missing and 3 partials :warning:
pkg/app/piped/controller/scheduler.go	0.00%	15 Missing :warning:
pkg/app/piped/executor/analysis/analysis.go	0.00%	2 Missing :warning:
pkg/git/repo.go	90.90%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4922      +/-   ##
==========================================
- Coverage   29.34%   28.49%   -0.86%     
==========================================
  Files         323      327       +4     
  Lines       40984    42284    +1300     
==========================================
+ Hits        12027    12048      +21     
- Misses      27997    29271    +1274     
- Partials      960      965       +5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[t-kikuc]

I’ll remove the wip label after fixing the CI error.

[khanhtc1202]

/review

[github-actions[bot]]

PR Analysis

Main theme

"Introduce stage skip logic based on commit message prefixes and file paths"

PR summary

This PR introduces the functionality to skip pipeline stages based on specific conditions, such as commit message prefixes or changes in certain file paths. It aims to provide flexibility in pipeline execution by allowing certain stages to be conditional based on the context of the commits.

Type of PR

Enhancement

PR Feedback:

General suggestions

The additions in this PR introduce the ability to skip pipeline stages based on commit messages or file path patterns. It is important to ensure that the new skip logic is well-documented and that any changes do not introduce security issues or degrade performance. Additionally, it's crucial to review the logic for any possible edge cases that might affect the expected behavior of the pipeline execution.

Code feedback

• relevant file: pkg/app/piped/controller/scheduler.go suggestion: | When logging an error due to the failure of checkSkipStage, including the stage ID in the logged message could provide more context for debugging.

  lp.Errorf("failed to check whether skipping the stage ID %q", ps.Id, zap.Error(err))
  

  relevant line: + lp.Errorf("failed to check whether skipping the stage", zap.Error(err))

• relevant file: pkg/app/piped/executor/analysis/analysis.go suggestion: | To maintain method naming consistency, consider changing checkSkippedFromCmd to something like isSkippedByCommand to more accurately reflect it returns a boolean representing the skipped state.

  func (e *Executor) isSkippedByCommand(ctx context.Context) bool {
  

  relevant line: +func (e *Executor) checkSkippedFromCmd(ctx context.Context) bool {

• relevant file: pkg/git/repo.go suggestion: | The method GetCommitForRev should have a comment explaining what the function does, following the Go convention for documenting public methods. relevant line: +func (r *repo) GetCommitForRev(ctx context.Context, rev string) (Commit, error) {

Security concerns:

no

The added logic relates to operational conditions and does not directly interact with user input or sensitive data paths. There seems to be no addition of new user-controllable input vectors that could introduce security issues such as SQL injection, XSS, or CSRF.

[t-kikuc]

@khanhtc1202 Thank you so much for your great reviewing! I fixed all!

[t-kikuc]

"should we separate the skipstage as a new package/module or just place it in scheduler code"

I quit separating skipstage package because I also think we don't need to separate package here.

• cab3539

• 7eda335

[Warashi]

IMO, I agree with khanhtc1202. Because we have no need to hide functions from other controller codes.

[ffjlabo]

Sorry to be late. I agree with it. https://github.com/pipe-cd/pipecd/pull/4922#pullrequestreview-2087774132

should we separate the skipstage as a new package/module or just place it in scheduler code" is left. IMO, we should place it in the scheduler.go code. Happy to hear other thoughts here 👀

[t-kikuc]

@Warashi

Thank you, that's important.

a. Path Pattern:

This PR already checks ALL changes from the running commit to the latest commit as below.

changedFiles, err := repo.ChangedFiles(ctx, runningRev, targetRev)

b. Commit Message:

As you said, only the latest commit is checked.

Cases when the latest commit matches the condition but not all commits match it would be:

• Commit&merge frequently
• Not using a squash merge

cf. commitMatcher only checks the triggered commit. https://github.com/pipe-cd/pipecd/blob/812907842c79b6c26f48fc56cf533d9a47cc24a2/pkg/app/pipedv1/planner/kubernetes/kubernetes.go#L158

@peaceiris What do you think? Should ALL commits match the condition?

[peaceiris]

Only the latest commit is good enough for my use case 👍

[t-kikuc]

@peaceiris I got it, thank you so much!

[khanhtc1202]

I think the same way, we should only treat the last commit message 👍