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:

[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 for the late review 🙏 Added some comments.

[ffjlabo]

[imo] How about adding the func StageConfig.GetSkipConfig() in the config package?

[t-kikuc]

I think the current one is enough because

• StageConfig.GetSkipConfig() will be used only in skipstage package
• adding the func to each stage model seems redundant

WDYT?

[ffjlabo]

OK, I got it 👍

[ffjlabo]

[imo] It might not be a func, just put it in the skipByCommitMessagePrefixes ?

[t-kikuc]

You mean we need to mock repo.ChangedFiles in skipByCommitMessagePrefixes?

[ffjlabo]

I feel commitMessageHasAnyPrefix doesn't have much logic, so IMO, it would be nice to gather the logic to the skipByCommitMessagePrefixes.

You mean we need to mock repo.ChangedFiles in skipByCommitMessagePrefixes?

When testing, we might need to mock as you said.

[t-kikuc]

@ffjlabo thanks, I merged the func and used mock in tests.
TBH, I missed git.mock.go https://github.com/pipe-cd/pipecd/blob/master/pkg/git/gittest/git.mock.go

[ffjlabo]

[imo] It might not be a func, just put it in the skipByPathPattern?

[t-kikuc]

@ffjlabo thanks, I merged the func and used mock in tests.

[ffjlabo]

[imo] It might not be a func, just put it in the shouldSkipStage ?

[t-kikuc]

Thanks, I did it here 8a96a8c

[ffjlabo]

Sorry to be late. I agree with it.
#4922 (review)

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 👀

[Warashi]

If I push several commits at once, this PR's code seems to process only the last commit.
I think all pushed commits should be handled the same way as the last commit. What do you think?

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

pipecd/pkg/app/pipedv1/planner/kubernetes/kubernetes.go

Line 158 in 8129078

if pipelineRegex.MatchString(in.Trigger.Commit.Message) {

@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 👍

[khanhtc1202]

LGTM, can't wait for this in action 👍

[Warashi]

Thank you. There is a consensus to see only the last commit message, so LGTM.