Skip to content

Miscellaneous improvements to option.NewNamedMapOptions #40529

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

Merged
merged 4 commits into from
Jul 16, 2025

Conversation

giorio94
Copy link
Member

Remove a few pitfalls associated with option.NewNamedMapOptions, and simplify its API surface. Please refer to the individual commits for additional details.

giorio94 added 4 commits July 15, 2025 12:31
Remove a potential pitfall and ensure that NewMapOptions correctly
initializes the given map if nil, so that the values can be set
properly. Currently, instead, the map is initialized, but the
original pointer is not updated, which means that the caller has
no ways to see the configured values.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
The name parameter is only assigned, but never read, so we can drop it
to simplify the callers.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Rework the NewMapOptions validator to actually perform validation only,
rather than possibly mutating the values as well. Indeed, all validators
are always passing through the given value, except the BPFMapEventBuffers
one, but the returned values are never read in that case. Additionally,
let's make the set of validators variadic, so that we can drop the nil
parameter from all callers that don't set a validator.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Jul 15, 2025
@giorio94
Copy link
Member Author

/test

@github-actions github-actions bot added the cilium-cli This PR contains changes related with cilium-cli label Jul 15, 2025
@giorio94 giorio94 marked this pull request as ready for review July 15, 2025 12:05
@giorio94 giorio94 requested review from a team as code owners July 15, 2025 12:05
@giorio94 giorio94 requested review from thorn3r, asauber and tklauser July 15, 2025 12:05
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Nice!

@giorio94 giorio94 enabled auto-merge July 15, 2025 12:16
@giorio94 giorio94 added this pull request to the merge queue Jul 16, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 16, 2025
Merged via the queue into cilium:main with commit 88bbfd4 Jul 16, 2025
86 checks passed
@giorio94 giorio94 deleted the mio/option-named-map branch July 16, 2025 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cilium-cli This PR contains changes related with cilium-cli kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants