Skip to content

fix(filter): filter group logic #102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 14, 2025
Merged

fix(filter): filter group logic #102

merged 1 commit into from
Mar 14, 2025

Conversation

apgrucza
Copy link
Contributor

@apgrucza apgrucza commented Mar 3, 2025 •
edited
Loading

This fixes a few issues with filter groups:

  • The group attribute was being ignored in the YAML config. We now set it in UnmarshalYAML and apply the default there instead of in GetGroup.
  • The filter logic wasn't working as documented. To keep a resource, all of the filter groups need to match, not one of them.
  • Global filters were being ignored when filter groups are enabled.

I've added/updated tests accordingly. Some of the tests were incorrect, which is why they were previously passing. I've fixed these too.

Fixes #103

@apgrucza apgrucza marked this pull request as draft March 3, 2025 05:09
@apgrucza apgrucza force-pushed the fix/filter-group-logic branch from 3fe2ef0 to 178b2bf Compare March 3, 2025 05:56
@apgrucza apgrucza marked this pull request as ready for review March 3, 2025 06:00
@apgrucza apgrucza marked this pull request as draft March 3, 2025 08:21
@apgrucza apgrucza force-pushed the fix/filter-group-logic branch from 178b2bf to 8480a8f Compare March 3, 2025 09:25
@apgrucza apgrucza marked this pull request as ready for review March 3, 2025 09:25
@apgrucza apgrucza force-pushed the fix/filter-group-logic branch from 8480a8f to 183fc86 Compare March 3, 2025 09:27
@ekristen
Copy link
Owner

ekristen commented Mar 3, 2025

Interesting. Thanks for this. To my knowledge it was working at least at some point. I'll need to spend a little time looking at current vs this PR to understand where things went wrong. I'll try and spend some time today reviewing, definitely this week.

@apgrucza
Copy link
Contributor Author

Any progress on this?

Since the group attribute is currently ignored, all filters currently end up in same (default) group. We can therefore surmise that nobody is using the filter groups feature, so it should be safe to merge this pull request as-is.

@codecov Codecov
Copy link

codecov bot commented Mar 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.94%. Comparing base (ce326f3) to head (183fc86).
Report is 6 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   98.92%   98.94%   +0.01%     
==========================================
  Files          19       19              
  Lines        1680     1704      +24     
==========================================
+ Hits         1662     1686      +24     
  Misses         13       13              
  Partials        5        5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ekristen ekristen merged commit 9080e32 into ekristen:main Mar 14, 2025
6 checks passed
@ekristen-dev
Copy link

ekristen-dev bot commented Mar 14, 2025

🎉 This pull request is included in version 0.24.4 🎉

The release is available on GitHub release 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filter groups do not work
2 participants
@apgrucza @ekristen