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

[confmap][fix] Correctly differentiate between nil and empty slices in ToStringMap #11755

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mahadzaryab1
Copy link
Contributor

@mahadzaryab1 mahadzaryab1 commented Nov 26, 2024

Description

  • Background
    • The confmap decoder hook has a zeroSliceHookFunc which treats empty slices provided in the config as empty slices rather than nil
    • However in confmap.ToStringMap, empty slices were getting rendered as nil which is different from the decoder hook as described above.
  • This PR fixes confmap.ToStringMap by adding a check in the sanitizer function that returns an empty slice when one is provided.
  • This PR is a prelude to [WIP][confmap] Enable decoding nil values in confmap #11734 to allow for the decoding of nil values in confmap to enable optional configs.

Link to tracking issue

Fixes #11749

Testing

  • Adjusted the existing unit test and added a new unit test

Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.52%. Comparing base (e91dafa) to head (5fb775c).

Files with missing lines Patch % Lines
confmap/confmap.go 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11755      +/-   ##
==========================================
- Coverage   91.53%   91.52%   -0.02%     
==========================================
  Files         443      443              
  Lines       23766    23770       +4     
==========================================
+ Hits        21754    21755       +1     
- Misses       1638     1640       +2     
- Partials      374      375       +1     

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

@mahadzaryab1 mahadzaryab1 marked this pull request as ready for review November 26, 2024 22:25
@mahadzaryab1 mahadzaryab1 requested a review from a team as a code owner November 26, 2024 22:25
Comment on lines +136 to +138
if m == nil {
return newSlice
}
Copy link
Contributor Author

@mahadzaryab1 mahadzaryab1 Nov 26, 2024

Choose a reason for hiding this comment

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

@mx-psi This block is currently not covered by tests because we're not decoding nil values in this PR (that will happen in #11734). We could (1) remove this if block here and add it in the following PR; (2) leave it in here but merge with missing code coverage; (3) add a unit test for this function directly. Let me know what you think.

@mx-psi
Copy link
Member

mx-psi commented Nov 29, 2024

Let's wait to discuss this with @songy23

@yurishkuro
Copy link
Member

The description is confusing to me

which differs from the behaviour of confmap's decoding workflow

What does this tell me?

What I would rather see is something like this:

  • the hook's description says
    • empty slice [] in yaml should override the default value assigned to config
    • but a nil slice in yaml should fall back to default value
  • but in practice the code was doing this: ...

@mahadzaryab1
Copy link
Contributor Author

The description is confusing to me

which differs from the behaviour of confmap's decoding workflow

What does this tell me?

What I would rather see is something like this:

  • the hook's description says

    • empty slice [] in yaml should override the default value assigned to config
    • but a nil slice in yaml should fall back to default value
  • but in practice the code was doing this: ...

@yurishkuro Thanks for the feedback! I updated the description. One thing to note is that the discrepancy is not within the hook but rather ToStringMap which is more of a helper function to create a map[string]any from a Conf. This PR matches the behaviour of ToStringMap with the behaviour of the hook and more specifically with that of zeroSliceHookFunc.

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.

Behaviour of ToStringMap Not Matching Confmap Decoding Workflow
3 participants