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

[💡 FEATURE REQUEST]: Allow defaults in env var expansion of server.env config #1989

Open
devnev opened this issue Aug 16, 2024 · 2 comments
Assignees
Labels
C-feature-accepted Category: Feature discussed and accepted help-needed-easy Call for participation: Experience needed to fix: Easy / not much

Comments

@devnev
Copy link

devnev commented Aug 16, 2024

Plugin

Server

I have an idea!

The current handling of server.env configuration does a plain os.Expand(_, os.Getenv()). Unlike other parts of the roadrunner config, this doesn't allow for defaults in the form ${ENV:-default}.

It would be great to allow defaults, or even go one step further and use https://github.com/drone/envsubst both here and in the general configuration for some more advanced substitution options.

@devnev devnev added the C-feature-request Category: feature requested, but need to be discussed label Aug 16, 2024
@rustatian
Copy link
Member

Hey @devnev 👋
Yeah, you're right, I forgot about these envs to standardize them.
It would be nice if you'd be able to send us a PR. I wrote a custom env parser (std+some additions): link.
The steps for anyone wanted to contribute:

  1. Copy that file to the server plugin.
  2. Replace this and this env parsers from the default os.Expand to custom.
  3. Write tests for that 😃

@rustatian rustatian added C-feature-accepted Category: Feature discussed and accepted and removed C-feature-request Category: feature requested, but need to be discussed labels Aug 17, 2024
@rustatian rustatian added this to the v2024.2.1 milestone Aug 17, 2024
@rustatian rustatian added the help-needed-easy Call for participation: Experience needed to fix: Easy / not much label Aug 17, 2024
@devnev
Copy link
Author

devnev commented Aug 21, 2024

@rustatian I started implementing this in the server plugin, but was then wondering why config plugin wasn't already doing this. It turns out I was using a slice-of-maps in the config for server.env, which viper with WeaklyTypedInput=true will merge into an output map when required, but isn't included in the results of viper's AllKeys method when the config plugin does its env expansion. Thet led me down a rabbit hole of how viper decodes things, and I've raised roadrunner-server/config#60 that should make it consistent, but might also be enough of a breaking change to warrant more careful consideration.

@rustatian rustatian removed this from the v2024.2.1 milestone Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: Feature discussed and accepted help-needed-easy Call for participation: Experience needed to fix: Easy / not much
Projects
Status: 🔖 Ready
Development

No branches or pull requests

2 participants