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

Bugfix namespaced filters #1143

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

MarkusFreitag
Copy link
Contributor

What this PR does / why we need it:

Currently there is a recursion bug for namespaced filters. It is caused by the loop within the merge closure, which itself is called for each item. This behaviour let to having the hashed namespace been added multiple times.

Which issue(s) this PR fixes:

Fixes the above explained bug.

Does this PR introduced a user-facing change?

None

The recursion bug was caused by the loop within the merge closure,
which itself is called for each item. This behaviour let to having the
hashed namespace been added multiple times.

Signed-off-by: Markus Freitag <[email protected]>
@benjaminhuo benjaminhuo requested a review from adiforluls April 22, 2024 14:10
@benjaminhuo
Copy link
Member

cc @wanjunlei

@adiforluls
Copy link
Member

I'll take a look tomorrow

@MarkusFreitag
Copy link
Contributor Author

A bit more context to the individual commits:
The first one fixes the described bug, by replacing the for loop with a switch/case type assertion. I've also added some unittests to verify that.
The interface, introduced within the second one, aims to increase read- and maintainability for the future, as the switch/case would just grow bigger. In case you don't like this solution, it could be dropped. In this case the addition from commit three should still be added as namespaced rewrite-tag filters have not worked before.

@adiforluls
Copy link
Member

LGTM, I can approve but there's a pipeline failure.
cc: @benjaminhuo

@benjaminhuo
Copy link
Member

LGTM, I can approve but there's a pipeline failure. cc: @benjaminhuo

nevermind the pipeline error, it's trying to push image to ghcr

@benjaminhuo benjaminhuo merged commit 975fea6 into fluent:master Apr 25, 2024
9 of 10 checks passed
@benjaminhuo
Copy link
Member

Thank you @MarkusFreitag !

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.

3 participants