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

[mahadzaryab1]

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

[codecov]

Codecov Report

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

Project coverage is 91.57%. Comparing base (574e434) to head (dfdfcbb).

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.58%   91.57%   -0.02%     
==========================================
  Files         446      446              
  Lines       23741    23745       +4     
==========================================
+ Hits        21743    21744       +1     
- Misses       1623     1625       +2     
- Partials      375      376       +1     

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

[mahadzaryab1]

@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]

Let's wait to discuss this with @songy23

[yurishkuro]

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]

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.

[songy23]

OK with this change

[mx-psi]

I was okay merging this with the expectation that we do not consider this a breaking change, but rather a change in an unspecified part of confmap. If we think this is a breaking change, then I am against merging this, since confmap is 1.0

[songy23]

I'm a bit on the fence here.

• On one hand this does change the current output data type of confmap so it will break users who rely on the type or format.
• On the other hand, the current behavior is subtle - it is not documented or specified anywhere. And from the PR description it sounds like the current behavior should not have been there in the first place. I'm OK with changing this back to bug_fix if treating it as a breaking change will block it.

[evan-bradley]

I've finally spent some time digging into this, and I think we should consider merging this after we define what we want to be the correct behavior. In my opinion, the current behavior is undefined and that's a bug in confmap, even if changing the existing behavior will break users. Personally, I'd like to keep a 1:1 mapping between YAML <-> map[string]any <-> confmap.Conf <-> Config struct as much as is reasonable.

I'm also not sure if we should keep zeroSliceHookFunc: removing it from our list of decoder hooks only breaks a single test in cmd/mdatagen, and that test passes with the hook removed if the changes in this PR are applied. In general, we should likely try to keep a distinction between []any(nil) and any{}, whereas the hook currently maps []any(nil) to []any{} (and ToStringMap, without the changes in this PR, maps []any{} to []any(nil)). Additionally, if I understand the changes in #11734, I'm not 100% sure the hook does much anymore.

Edit: I see now that the hook will clear out a slice during unmarshaling. I will look into how we can confirm this with a test case.

[evan-bradley]

I created a PR to illustrate a little of what I have in mind here: #11882.

I ran the k8s attributes processor tests with this change and didn't see any failing tests from removing zeroSliceHookFunc.

[mahadzaryab1]

@evan-bradley thanks for the great explanation and the illustration in the PR. Let me know how you think we should proceed here. Do we want to replace my PR with the one you have open?