Skip to content

Conversation

@joejstuart
Copy link
Contributor

@joejstuart joejstuart commented Jul 16, 2025

feat: implement pluggable rule filtering system with pipeline intentions

  • Add extensible filtering framework with RuleFilter interface
  • Implement PipelineIntentionFilter for filtering rules by pipeline intentions
  • Implement IncludeListFilter for filtering by collections, packages, and rules
  • Add pipeline_intention metadata field to rule annotations
  • Fix filtering leak by ensuring AllNamespaces=false prevents bypassing filters
  • Add comprehensive test coverage for all filtering components
  • Maintain backward compatibility with existing filtering logic

The new system allows for flexible rule filtering based on pipeline intentions
and include lists, while preventing any evaluation of excluded namespaces
regardless of filtering results.

Assisted by: Cursor (powered by Claude 4 Sonnet)

@joejstuart joejstuart force-pushed the EC-1339-3 branch 3 times, most recently from dfc4eea to 7734a9e Compare July 17, 2025 23:48
@joejstuart joejstuart changed the title Filter rego packages based on pipeline intention implement pluggable rule filtering system with pipeline intentions Jul 17, 2025
@joejstuart
Copy link
Contributor Author

I have a fix coming for the acceptance test errors.

@joejstuart joejstuart force-pushed the EC-1339-3 branch 2 times, most recently from 1186cc8 to 79084bf Compare July 22, 2025 14:48
} else if len(c.namespace) == 0 {
// When no namespaces are specified and filtering results in empty list,
// use an empty namespace list to prevent any evaluation
namespaceToUse = []string{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it likely to happen for real? How will the user deal with it? Would it be better to throw an error? Or do we have some handling for this later that presents the situation to the user?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just wondering, not a blocker to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we set it to nil during evaluation now. It still could use improvement. Here is an update 37a790f

@simonbaird
Copy link
Member

High level suggestions/comments/thoughts:

  • I think I like the filtering mechanism, and it feels like it's useful no matter what. However I wonder if we could add it with just the include filtering in a nice tidy PR. Then introduce the pipeline_intention filtering support separately.
  • The more I think about it, the more I wonder why we need this new special "pipeline_intention" filter. What if we put all the pipeline intention rules in a collection called "redhat-release", then just included them by collection? It might require some extra flags to do --include-only "@redhat-release" on the command line, and have that override or merge with the include from the ECP.

@joejstuart
Copy link
Contributor Author

High level suggestions/comments/thoughts:

  • I think I like the filtering mechanism, and it feels like it's useful no matter what. However I wonder if we could add it with just the include filtering in a nice tidy PR. Then introduce the pipeline_intention filtering support separately.
  • The more I think about it, the more I wonder why we need this new special "pipeline_intention" filter. What if we put all the pipeline intention rules in a collection called "redhat-release", then just included them by collection? It might require some extra flags to do --include-only "@redhat-release" on the command line, and have that override or merge with the include from the ECP.

We can look at including it as a collection, but it takes on multiple values (release, production, staging). I noticed it's being used in ITS's with pipeline_intention=staging. We'd probably want to investigate more if we change how this works so we don't break anyone.

@simonbaird
Copy link
Member

simonbaird commented Jul 23, 2025

We can look at including it as a collection, but it takes on multiple values

Yeah good point. It think let's go ahead with this design and maybe consider other options long term.

@simonbaird
Copy link
Member

I had one or two minor suggestions, but overall lgtm.

@joejstuart joejstuart force-pushed the EC-1339-3 branch 3 times, most recently from 3c071b5 to 903c2e3 Compare July 24, 2025 02:31
- Add extensible filtering framework with RuleFilter interface
- Implement PipelineIntentionFilter for filtering rules by pipeline intentions
- Implement IncludeListFilter for filtering by collections, packages, and rules
- Add pipeline_intention metadata field to rule annotations
- Fix filtering leak by ensuring AllNamespaces=false prevents bypassing filters
- Add comprehensive test coverage for all filtering components
- Maintain backward compatibility with existing filtering logic

The new system allows for flexible rule filtering based on pipeline intentions
and include lists, while preventing any evaluation of excluded namespaces
regardless of filtering results.

Assisted by: Cursor (powered by Claude 4 Sonnet)

https://issues.redhat.com/browse/EC-1401
@joejstuart joejstuart merged commit 6297bc7 into conforma:main Jul 24, 2025
11 checks passed
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