Skip to content

Conversation

@cbuto
Copy link
Contributor

@cbuto cbuto commented Jan 28, 2022

A follow up on #522 to print warnings if deprecated options are being set.

Admittedly, I haven't added unit tests around this yet as that required some refactors around how the configuration code works right now (a lot of the configuration relies on variables that aren't easily overridden). I can come back and handle that if this approach seems good!

It seemed the best way to approach this was to create the logger earlier on, though I didn't pull Debug and LogJSON out of the ServerOptions struct just in case for some reason these are being used by someone (seems unlikely).

@cbuto cbuto force-pushed the log-deprecation-warning branch from 87d590f to f438a44 Compare January 28, 2022 15:58
ChartPostFormFieldName: conf.GetString("chartpostformfieldname"),
ProvPostFormFieldName: conf.GetString("provpostformfieldname"),
ContextPath: conf.GetString("contextpath"),
LogJSON: conf.GetBool("logjson"),
Copy link
Contributor

Choose a reason for hiding this comment

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

If LogJSON and Debug fields are no longer effective can you add a comment in ServerOptions prefixed with // Deprecated: <reason>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitely, added in d84441b!

@jdolitsky jdolitsky added this to the v0.14.0 milestone Jan 28, 2022
cbuto added 2 commits January 28, 2022 11:24
Signed-off-by: Casey Buto <cbuto@d2iq.com>
Signed-off-by: Casey Buto <cbuto@d2iq.com>
@cbuto cbuto force-pushed the log-deprecation-warning branch from 7481306 to d84441b Compare January 28, 2022 16:25
Copy link
Contributor

@jdolitsky jdolitsky left a comment

Choose a reason for hiding this comment

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

excellent, thanks @cbuto !

@jdolitsky jdolitsky merged commit c76aed0 into helm:main Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants