Skip to content

Conversation

@patrick-stephens
Copy link

@patrick-stephens patrick-stephens commented Dec 19, 2024

Resolves fluent/fluent-bit#9743 by making lookups case-insensitive.

Potentially also resolves fluent/fluent-bit#9537

Fixes up out-of-date workflow usage of old actions and versions too in #56 then merged in.

@patrick-stephens patrick-stephens enabled auto-merge (squash) December 19, 2024 12:44
@patrick-stephens patrick-stephens merged commit 20da4c6 into master Dec 19, 2024
21 of 23 checks passed
@patrick-stephens patrick-stephens deleted the 9743_case_sensitivity branch December 19, 2024 13:50
@braydonk
Copy link

I wasn't aware my approval would trigger the auto-merge, my mistake. Hopefully that's okay, it looks right to me.

@patrick-stephens
Copy link
Author

Yeah I'll add some required checks and codeowners to ensure we sort that but I'd checked the CI was ok before automerge enabling

@patrick-stephens
Copy link
Author

#57 adds the relevant changes and I've updated branch protection to ensure we have a required check now.

@edsiper
Copy link
Member

edsiper commented Dec 19, 2024

While this resolves the high-level issue in Fluent Bit, I would not agree to get this merged into the underlying library, which pretty much needs to be agnostic for the user case of the "end-user" (Fluent Bit in this case), e.g: kvlist purpose is not only to serve the Fluent Bit config reader, many other components and peer libraries, this change adds a restriction/limitation: if we wanted to extend the lookup with a hashing mechanism case-insensitive becomes a restriction (unless the original key is stored in lowercase so the hash will match). Please do not rush the merge of PRs that are in active discussion, as well the history of the PR (commits) has a couple of duplicates, we want to avoid that.

I agree with the solution proposed in #58 which is sustainable.

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.

yaml: case-sensitivity of name field Config parsing error: "section 'filter' is missing the 'name' property"

4 participants