[confmap] log a warning when using $VAR in config (WIP)

[tomasmota]

Description: As requested by @mx-psi , added a no-op log for when variables using the deprecated $VAR style are used. The logger should be replaced once it is clear how to pass it down (see #9443). Also, from my testing, the function passed to os.Expand is in fact only run when we have $VAR and not for ${env:VAR}, so I did not add additional checking.

Link to tracking Issue: #9162

Testing: I am not sure how to go about testing it, since we are not passing a logger in yet, there is no easy way to know what is being logged or what the map looks like. Some ideas on this would be appreciated

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.03%. Comparing base (7f13812) to head (169069f).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9547      +/-   ##
==========================================
+ Coverage   90.99%   91.03%   +0.03%     
==========================================
  Files         353      353              
  Lines       18706    18709       +3     
==========================================
+ Hits        17022    17031       +9     
+ Misses       1356     1349       -7     
- Partials      328      329       +1     

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

[mx-psi]

Thanks for opening the PR! I left some suggestions:

• String sets are typically represented in Go with the type `map[string]struct{}. I left some suggestions to do just that
• We need to warn only for $var, not for ${var}
• We need some new unit tests for this!

[tomasmota]

Thanks for opening the PR! I left some suggestions:

• String sets are typically represented in Go with the type `map[string]struct{}. I left some suggestions to do just that
• We need to warn only for $var, not for ${var}
• We need some new unit tests for this!

Thanks, regarding the unit tests, as stated in the PR, I would appreciate a suggestion with regards to how I could check for the warning. Not sure how do do it before the logger is being injected :/

[mx-psi]

For testing you can replace the logger with https://pkg.go.dev/go.uber.org/zap/zaptest/observer

[tomasmota]

For testing you can replace the logger with https://pkg.go.dev/go.uber.org/zap/zaptest/observer

That's what I wanted to do, but then I need to pass down the logger in ConverterSettings instead of hardcoding, so I can change it in the test, and from reading #9443 I got the impression that it is not yet totally clear how to pass it down. I would intuitively just add

type ConverterSettings struct{
    Logger *zap.Logger
}

Is this what you were expecting I do?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[mx-psi]

For testing you can replace the logger with pkg.go.dev/go.uber.org/zap/zaptest/observer

That's what I wanted to do, but then I need to pass down the logger in ConverterSettings instead of hardcoding, so I can change it in the test, and from reading #9443 I got the impression that it is not yet totally clear how to pass it down. I would intuitively just add

type ConverterSettings struct{
    Logger *zap.Logger
}

Is this what you were expecting I do?

@tomasmota Hey, apologies for the delay on replying, I went on vacation and had a huge backlog afterwards :) Yes, this is what I want to do, we can do it on this PR but IMO it's not necessary for testing right now

[mx-psi]

@tomasmota is this ready for another review?

[tomasmota]

@tomasmota is this ready for another review?

Yes, and sorry about the mess of commits, I messed it up and got too lazy to reset properly. guess you will just squash in the end anyways.

[mx-psi]

Code looks great! Would you be up for writing some tests? You can write a function for testing like this:

func NewTestConverter() (confmap.Converter, observer.ObservedLogs) {
   core, logs := observer.New(zapcore.InfoLevel)
   conv := converter{ loggedDeprecations: make(map[string]struct{}), logger: zap.New(core)}
   return conv, logs
}

And then call it with some example confmap.Conf.

If this is too much, I am also happy to take over, please let me know :)

[tomasmota]

I'll take a stab at it when I get the time :) thanks for the indications!

[tomasmota]

Code looks great! Would you be up for writing some tests? You can write a function for testing like this:

func NewTestConverter() (confmap.Converter, observer.ObservedLogs) {
   core, logs := observer.New(zapcore.InfoLevel)
   conv := converter{ loggedDeprecations: make(map[string]struct{}), logger: zap.New(core)}
   return conv, logs
}

And then call it with some example confmap.Conf.

If this is too much, I am also happy to take over, please let me know :)

I have added some tests, feel free to review

[mx-psi]

To test the QuoteMeta change I commented above, we should add a test like $HOSTONAME${HOST.NAME} to check that we are properly escaping characters

[tomasmota]

I have added QuoteMeta because it makes sense, but in practice I'm not really understanding how it is useful or needed, because our regexp will never match ${HOST.NAME}, and if you attempt to have $HOST.NAME you will just end up with 127.0.0.1.NAME according to my testing. Same if you put other special characters after $HOST.

With that in mind, I couldn't think of a good test case, because the $VAR way, which is the only one we match, does not play well with special characters anyways.

[mx-psi]

You are right, I mistakenly assumed that . would be a valid part of a naked environment variable name for os.Expand, but it does not seem like that is the case.

I guess a test that has both ${HOST.NAME} and $HOST_NAME should be a valid test, right? Taking a look at the implementation of os.Expand, it also seems like $? would be a valid thing to test, but I doubt anybody is using that in practice 😄

[tomasmota]

@mx-psi i think you can review, I addressed your comments directly.

[mx-psi]

Looks good to me, I will skip the changelog on this PR and we can add it once we actually do the logging.

I left a comment on a testcase for regexp.QuoteMeta that I think should work, let's give it a try and add it if it works

[mx-psi]

You are right, I mistakenly assumed that . would be a valid part of a naked environment variable name for os.Expand, but it does not seem like that is the case.

I guess a test that has both ${HOST.NAME} and $HOST_NAME should be a valid test, right? Taking a look at the implementation of os.Expand, it also seems like $? would be a valid thing to test, but I doubt anybody is using that in practice 😄

[tomasmota]

@mx-psi I added a test with what I understood you were asking for, not sure it was exactly that you meant 😅

[mx-psi]

Yeah, that's what I meant, thanks!

[tomasmota]

Great, really appreciate your help and patience!