Skip to content
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.

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

test(domain-filter): simple filters on domain exclusion #5064

Merged

Conversation

ivankatliarchuk
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk commented Feb 5, 2025

Description

Relates #3718

The work relates to a bug in domain filter logic. I've added a test that focuses only on MATCH behaviour for a domain filters with include and exclude values, and validate against provided domains. This does not fix anything. The filters itself looks like works correctly.

There is a slight design problem. We instantiate filters in main.go

if cfg.RegexDomainFilter.String() != "" {
, this could not be tested, hence the bug could be there. I keep digging.

Probably an option to consider is to create a composition filter or something. Move if/else filter creation logic away from main.go file. Any preferences, ideas?

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 5, 2025
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 5, 2025
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 5, 2025
@ivankatliarchuk
Copy link
Contributor Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 5, 2025
@ivankatliarchuk ivankatliarchuk changed the title issue(filter-tags): issue-3718 domain exclusion filters issue(filter-tags): issue-3718 domain exclusion filters tests only Feb 5, 2025
@ivankatliarchuk ivankatliarchuk changed the title issue(filter-tags): issue-3718 domain exclusion filters tests only chore(domain-filter): issue-3718 domain exclusion filters tests only Feb 5, 2025
@mloiseleur
Copy link
Contributor

mloiseleur commented Feb 8, 2025

There are already tests on this with TestDomainFilterWithExclusions on exclusion L408, so I am unsure of what we should do with this PR. Is this still a draft ?

@ivankatliarchuk
Copy link
Contributor Author

These tests are different. There is some sort of deserialisation, fixtures coupled with two tests, and handled with block like

if len(tt.exclusions) > 0 {
			t.Logf("NewDomainFilter() doesn't support exclusions - skipping test %+v", tt)
			continue
		}

I was actually not able to make it green with such a simple case, probably designed with a slightly different model, that I can't get my head around.

So the plan was to create a simple tests. I think long term this whole domain filter need to be revisited and simplified.

If you think is not required, I'll close this pull request. We could w8 for community member to review they may came out with better plan.

@mloiseleur
Copy link
Contributor

mloiseleur commented Feb 9, 2025

Then let's rename it with a clear different name from the already existing one and we should be good to go.
See my suggestion with a proposition for this name.

Co-authored-by: Michel Loiseleur <[email protected]>
@mloiseleur
Copy link
Contributor

/retitle test(domain-filer): simple filters on domain exclusion

@k8s-ci-robot k8s-ci-robot changed the title chore(domain-filter): issue-3718 domain exclusion filters tests only test(domain-filer): simple filters on domain exclusion Feb 10, 2025
@mloiseleur
Copy link
Contributor

/retitle test(domain-filter): simple filters on domain exclusion

@k8s-ci-robot k8s-ci-robot changed the title test(domain-filer): simple filters on domain exclusion test(domain-filter): simple filters on domain exclusion Feb 10, 2025
@mloiseleur
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mloiseleur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2025
@k8s-ci-robot k8s-ci-robot merged commit 70cbcd1 into kubernetes-sigs:master Feb 10, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants