-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: fallback on environnement variables for strings parsing #3420
base: master
Are you sure you want to change the base?
Conversation
I like the idea, I'm curious how @antonmedv sees it :-) |
What if we prefer all such vars with ENV_? |
You mean prefix? Instead of "auto discovery"? |
Actually I was thinking about how Symfony manage env vars, but phpdeployer is run on its own, so there is no need to take this consideration into account. I have then done a push which use getenv method, like I've done here : #3421. |
I guess you want to reduce the risk of regression? Actually, I've done another push to execute the fallback in "auto discovery" mode (like named it @Schrank) only if nothing else has worked (which is finally a true fallback compare to my first proposition). In that way there is no regression possible as if the portion of code was missing, the script would lead to a blocking exception. |
Little up @Schrank @antonmedv :) |
Hi @Schrank @antonmedv, any news for this PR? |
if ($this->parent) { | ||
return $this->parent->has($name); | ||
} | ||
|
||
if (array_key_exists($name, \getenv())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t like what it’s for all the configurations. Let’s make it optional only in yaml via explicit exporfs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by via explicit exporfs
?
Actually, it would also be usefull for PHP configs too as in that way you don't need to edit your script to modulate its behavior and thus you can share your config across your projects. The way it is implemented is transparent for actual users who would upgrade to this version (because it is a fallback), but we can add a parameter to explicitly enable this feature to be sure to avoid any side effect (which can't occur because if you pass an unrecognised option, the deployment will fail by raising a fatal exception see
deployer/src/Configuration/Configuration.php
Line 111 in adbb4b7
throw new ConfigurationException("Config option \"$name\" does not exist."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antonmedv @Schrank any update on this please ?
…last chance method to avoid any risk of regression
As I prefer yaml files for configuration, I have a made a little modification to make phpdeployer able to fallback on environnement variables when trying to parse a string, in order to get a clean yaml configuration file which can be standard across project, and gitlab-ci (and I think github actions too) compatible.
Given this yaml file and the corresponding environnement variables set into the CI/CD pipeline, everything should be fine 👍
What do you think about?