Uploaded image for project: 'OpenShift Pipelines'
  1. OpenShift Pipelines
  2. SRVKP-9977

Improve skip-push-event-for-pr-commits to only skip actual duplicate runs

XMLWordPrintable

    • Icon: Epic Epic
    • Resolution: Won't Do
    • Icon: Major Major
    • None
    • None
    • Pipelines as Code
    • improve-skip-push-event-for-pr-commits-to-only-skip-actual-duplicate-runs
    • False
    • Hide

      None

      Show
      None
    • False
    • To Do

      Story (Required)

      As a DevOps engineer trying to configure CI/CD pipelines that trigger only on push events I want the skip-push-event-for-pr-commits feature
      to only skip push events when an actual duplicate PipelineRun would occur so that my pipelines are not incorrectly blocked when there are
      open pull requests that would not create duplicate runs.

      Background (Required)

      The skip-push-event-for-pr-commits feature was introduced to prevent duplicate PipelineRuns when a commit appears in both a push event
      and a pull request event. However, the current implementation has a logic flaw that causes false positives.

      Current Behavior:

      The feature checks if a commit SHA exists in ANY open pull request and skips ALL push events for that commit, regardless of whether:

      • The pipeline is configured to trigger on PR events
      • The push event's branch matches the PR's head or base branch
      • An actual duplicate run would occur

      Customer Impact:

      Ford Motor Company (Case #04264459) is experiencing production issues where:
      Developer 1 opens PR from qa → main (remains open for approval)

      Developer 2 pushes to qa branch (different feature)

      Developer 2's push event is SKIPPED because the commit is now "part of the open PR"

      No pipeline runs, even though NO PR event trigger is configured

      This affects multiple development teams working in parallel and has become a widespread issue across their production clusters.

      Related Work:

      • SRVKP-9111: Fixed tag push events being skipped (resolved in 1.21)
      • SRVKP-9351: RFE to not auto-enable features on upgrade

      Out of scope

      • Changing the default value of skip-push-event-for-pr-commits (covered by SRVKP-9351)
      • Supporting other Git providers beyond GitHub in this RFE (can be addressed separately)
      • Advanced duplicate detection based on PipelineRun history (too complex due to retention policies)

      Approach (Required)

      Option 1: Branch-Aware Skip Logic (Recommended)

      Enhance the duplicate detection logic in pkg/provider/github/parse_payload.go (lines 337-345) to be branch-aware:

      // Current problematic logic:
      if v.pacInfo.SkipPushEventForPRCommits && len(prs) > 0 && !isGitTagEvent {
          isPartOfPR, prNumber := v.isCommitPartOfPullRequest(sha, org, repoName, prs)
          if isPartOfPR {
              // SKIPS ALL PUSH EVENTS - TOO AGGRESSIVE
              return nil, nil
          }
      }
      
      // Proposed improved logic:
      if v.pacInfo.SkipPushEventForPRCommits && len(prs) > 0 && !isGitTagEvent {
          // Extract the branch name from the push event
          pushBranch := extractBranchFromRef(gitEvent.GetRef()) // e.g., "refs/heads/qa" -> "qa"
      
      // Check if this push would duplicate a PR event
      isDuplicate, prNumber := v.wouldCreateDuplicateRun(sha, pushBranch, org, repoName, prs)
      
      if isDuplicate {
          v.Logger.Infof("Skipping push event for commit %s on branch %s as it would duplicate PR #%d", sha, pushBranch, prNumber)
          return nil, nil
      }
      }
      

      The new wouldCreateDuplicateRun function should only return true when:
      The commit is part of an open PR, AND

      The push event's branch matches EITHER:

      • The PR's head branch (the branch being merged FROM), OR
      • The PR's base branch (the branch being merged TO)

      Why this works:

      In Ford's scenario:

      • PR: qa → main (open)
      • Push: to qa branch
      • Push branch (qa) == PR head branch (qa) → Would skip (correct, as PR event would also trigger)
      • Push: to dev branch (different developer)
      • Push branch (dev) != PR head branch (qa) AND != PR base branch (main) → Would NOT skip (correct, no duplicate)

      Alternative Option 2: CEL Expression Analysis

      Query the repository's PipelineRun CEL expressions to determine if PR events are configured. Only skip push events if:
      Commit is in open PR, AND

      Repository has PipelineRuns with event == "pull_request" configured

      This is more accurate but requires additional API calls and CEL parsing complexity.

      Dependencies

      • None - this is a standalone enhancement to existing functionality

      Acceptance Criteria (Mandatory)

      AC1: Branch Matching Logic

      • GIVEN a commit that is part of an open PR from branch A to branch B
      • WHEN a push event occurs on branch A (PR head branch)
      • THEN the push event SHOULD be skipped

      AC2: Non-Matching Branch Logic

      • GIVEN a commit that is part of an open PR from branch A to branch B
      • WHEN a push event occurs on branch C (unrelated branch)
      • THEN the push event SHOULD NOT be skipped

      AC3: Ford's Scenario

      • GIVEN:
      • PR open: qa → main
      • Pipeline configured with: event == "push" && target_branch == "qa"
      • NO PR event configuration
      • WHEN a developer pushes to qa branch
      • THEN pipeline run SHOULD trigger (currently broken)

      AC4: Tag Events Remain Unaffected

      • GIVEN the fix for SRVKP-9111 is in place
      • WHEN a tag is pushed for a commit in an open PR
      • THEN the push event SHOULD NOT be skipped (already working in 1.21)

      AC5: Backward Compatibility

      • GIVEN repositories where push and PR events genuinely would duplicate
      • WHEN a commit is pushed to a PR's head branch
      • THEN the push event SHOULD be skipped (existing behavior preserved)

      Edge Cases to Consider:

      • Multiple open PRs containing the same commit
      • PRs targeting different base branches
      • Force pushes that change commit SHAs
      • Merged PRs vs open PRs

      INVEST Checklist

      • Dependencies identified: None
      • Blockers noted and expected delivery timelines set: No blockers
      • Design is implementable: Yes, requires changes to pkg/provider/github/parse_payload.go
      • Acceptance criteria agreed upon: Pending customer validation
      • Story estimated: Pending engineering review

      Done Checklist

      • Code is completed, reviewed, documented and checked in
      • Unit tests added to pkg/provider/github/parse_payload_test.go covering all acceptance criteria
      • Integration tests added to test/github_pullrequest_test.go for Ford's specific scenario
      • Documentation updated in docs/content/docs/install/settings.md to clarify the behavior
      • Customer case #04264459 validated with fix
      • Acceptance criteria are met

      Additional Context

      Customer Quote from Case:

      According to the feature description, this option should prevent duplicate PipelineRuns. However, it should not incorrectly assume that a
      run is a duplicate when it is not.

      Log Evidence:

      "msg":"Skipping push event for commit 925dcca61fcc6b33a325de16b9be54c7a5773d66 as it belongs to pull request #33"
      

      File References:

      • Implementation: pkg/provider/github/parse_payload.go:337-345
      • Configuration: pkg/params/settings/config.go:76
      • Tests: pkg/provider/github/parse_payload_test.go
        
        

              Unassigned Unassigned
              cboudjna@redhat.com Chmouel Boudjnah
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

                Created:
                Updated:
                Resolved: