feature: apply env expansion consistently by moving it into a decode hook #60
Conversation
WalkthroughThe recent changes streamline configuration handling within the codebase by removing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigManager
participant Viper
User->>ConfigManager: Request Configuration
ConfigManager->>Viper: Retrieve Configuration
Viper-->>ConfigManager: Return Config Data
ConfigManager-->>User: Provide Config
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
Files selected for processing (3)
- expand.go (2 hunks)
- include.go (2 hunks)
- plugin.go (4 hunks)
Files skipped from review due to trivial changes (1)
- expand.go
Additional comments not posted (6)
include.go (2)
33-37: Improved error handling inhandleInclude.The initialization of
ifilesas an empty slice and the use ofUnmarshalKeywith error handling enhance the robustness of the function. This prevents potential nil pointer dereference issues.
61-63: Enhanced error handling inhandleEnvFile.The initialization of
envFileas an empty string and the use ofUnmarshalKeywith error handling improve the function's reliability by ensuringenvFileis always initialized.plugin.go (4)
83-83: Simplified environment variable handling inInit.The removal of automatic environment variable injection and the use of
handleEnvFilefor environment file handling simplify the logic and improve error handling.
135-137: Custom decoding options inUnmarshalKey.The inclusion of
p.unmarshalOpts()for custom decoding options enhances the flexibility and robustness of configuration handling.
144-146: Custom decoding options inUnmarshal.The inclusion of
p.unmarshalOpts()for custom decoding options enhances the flexibility and robustness of configuration handling.
151-161: Improved type conversion and environment variable parsing inunmarshalOpts.The use of decode hooks for type conversion and environment variable parsing improves the handling of complex data types and aligns with the PR objectives.
|
This also affects the semantics of the |
|
Hey @devnev 👋 You may introduce new methods, but yeah, we should not introduce breaking changes.
I already thought about that, but didn't want to introduce a BC 😞 However, |
rustatian
changed the title
|
It'll take me a while to circle back to this. Making it behave in a compatible fashion while also handling calls to Overwrite could be tricky. |
|
This is the case 😃 You don't need to be compatible. You may create an OverwriteWithEnv or whatever you choose the name. Put the new logic into that new method and that's it 😃 |
|
Hey @devnev 👋 Do you have plans to continue working on this? |
|
@rustatian I got really stuck on disentangling the various configs and overrides, and now I have other priorieties although I'd like to get back to this eventually. Maybe its better to close for now, to avoid any confusion, and I can re-open if and when I get back to it? |
|
@devnev np at all 😃 I'm not going to close this PR, just take your time and when you would have more time feel free to return to it 😃 Thank you for your work 👍 |
Reason for This PR
Addresses roadrunner-server/roadrunner#1989 and likely other sources of confusion where certain strings in the config file might not have been getting the expand-env-with-default treatment.
Description of Changes
The code before this change iterates through configuration values with viper's AllKeys() method, and tries to apply the env expansion in cases it understands. However, AllKeys() doesn't descend into sequences/slices, which the previous code tried to handle by checking if the slice contained strings, but its also possible to have slices of more complex values, e.g. subobjects, which would be completely ignored by this case.
By moving the logic for expanding environment variables into the unmarshal decoder, it is applied to any string encountered during unmarshaling (at least I think thats the case based on my reading of how viper does things).
This will cause problems if other plugins apply their own form of env expansion, e.g. the server plugin doing its own os.Expand() on environment variable values. So this is probably a breaking change, and at the very least requires the server plugin to be updated accordingly. It also changes the semantics of the plugins Get() method, which will return values where the expansion has not been applied, and it changes Overwrite() because the values set using Overwrite will have env expansion applied to them.
An alternative might be to still iterate through viper.AllKeys(), but for each key use UnmarshalKey with the decode option to consistently apply env substitution, and and call viper.Set() with the result to write it back.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes
Refactor