Skip to content

Conversation

@joshadambell
Copy link

Summary

Fixes a bug where repositoryMatch was ignored when combined with branch-level conditions like pathsExist, pathsDoNotExist, or branchMatch.

The Problem

SCM Provider filtering operates in two phases:

  1. Repo phase - filters repositories by name and labels
  2. Branch phase - filters by branch name and path existence

Previously, getApplicableFilters categorized filters based solely on their FilterType enum. A filter with both repositoryMatch (repo-level) AND pathsExist (branch-level) would only be placed in the branch filter group, causing the repositoryMatch condition to never be evaluated.

The Fix

  1. getApplicableFilters now inspects the actual conditions present in each filter rather than relying on the FilterType enum. Filters with mixed conditions are added to both filter groups.

  2. matchFilter now accepts a phase parameter to evaluate only the conditions relevant to the current filtering phase:

    • FilterTypeRepo phase → checks repositoryMatch and labelMatch
    • FilterTypeBranch phase → checks branchMatch, pathsExist, and pathsDoNotExist

Example

filters:
  - repositoryMatch: "^eks-"
    pathsExist:
      - "config/app.yaml"

Before: All repos with config/app.yaml matched (repositoryMatch ignored)
After: Only repos starting with eks- AND containing config/app.yaml match

⚠️ Breaking Change

This fix changes the behavior of filters that combine repo-level and branch-level conditions. Previously, repo-level conditions (repositoryMatch, labelMatch) were silently ignored when combined with branch-level conditions (branchMatch, pathsExist, pathsDoNotExist).

If you have filters like this:

filters:
  - repositoryMatch: "^eks-"
    pathsExist:
      - "config/app.yaml"

Before: Matched all repos containing config/app.yaml (repositoryMatch was ignored)
After: Correctly matches only repos starting with eks- that also contain config/app.yaml

Migration

  • If your existing filters rely on the old (buggy) behavior, you may see fewer repositories matched after this change
  • Review any ApplicationSets using mixed filter conditions to ensure the corrected AND behavior is desired
  • If you intentionally want OR behavior, use separate filter blocks (multiple filters are ORed together)

…ed correctl

Signed-off-by: Josh Bell <joshadambell@me.com>
@joshadambell joshadambell requested review from a team as code owners January 21, 2026 12:44
@bunnyshell
Copy link

bunnyshell bot commented Jan 21, 2026

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

@joshadambell
Copy link
Author

#14465

Signed-off-by: Josh Bell <joshadambell@me.com>
@joshadambell joshadambell force-pushed the bugfix/scmprovider-filtering-fix branch from 07a8907 to 6767697 Compare January 21, 2026 12:46
@ppapapetrou76
Copy link
Contributor

ppapapetrou76 commented Jan 21, 2026

@joshadambell , thanks for the contribution. It seems you are introducing a breaking change, and many things are moving.
I highly recommend joining tomorrow's contributor meeting and presenting your PR in details

WDYT?

@joshadambell
Copy link
Author

Scheduled to discuss this in the contributor meeting on 1/29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants