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

[ENV vars logic] Move env specific var definitions in the parameters.dist.yml #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[ENV vars logic] Move env specific var definitions in the parameters.dist.yml #306

wants to merge 1 commit into from

Conversation

Plopix
Copy link
Contributor

@Plopix Plopix commented Jul 12, 2018

That is the first thing I do on every project.
I don't get why we have env(VAR): definitions are mixed in the default_parameters.yml and parameters.dist.yml - it does not simplify at all.

One mentioned reason on Slack was:

the reason was to reduce the amount of variables you will be asked for on install, only expose the must haves
To get the site up and running, you need the database at least. Everything else can be configured later, on demand

To me that is not a good reason, it is kind of "quick and dirty" because:

  • deciding where to store/define ENV vars should be logical and consistent
  • it should make sense for a new-comers and/or a reader who arrives on a existing the project
  • now most of the time we are automating the installation and then we run composer with --no-interaction
  • it is not that long to hit enter 12 times more when installing and it could be educative to see those variables.

Even with the given reasons in Slack, I find hard to justify that the DB configuration is in the parameters.dist.yml and the MAILER is in default_parameters.yml?

That is not logical to me.

Then I propose:

  • the default_parameters.yml to be the file where we define the parameters that ezplatform needs to be set up, and those paremeters are getting their values from the ENV.
  • this default ENV values are defined in the parameters.dist.yml (where they belong according to me)

This PR includes also the MAILER_PORT which is also useful.

Let me know! ping @lserwatka @andrerom @bdunogier @SylvainGuittard @emodric @gggeek .

Others? any feedback?

@emodric
Copy link
Contributor

emodric commented Jul 13, 2018

To get the site up and running, you need the database at least. Everything else can be configured later, on demand

I stand by my sentence :) I instantiate eZ Platform installs many times per day for various testing purposes, so I actually like that only minimum required config is in parameters.yml.dist

@alongosz
Copy link
Member

I'm gonna agree with @emodric on this :)
But maybe we could just adjust the first line # comment in parameters.yml.dist to tell Developers where to look for the full list of parameters.

@gggeek
Copy link
Contributor

gggeek commented Jul 13, 2018

I am not sure if I follow fully the reasoning of @Plopix - esp. because so far I have never relied on using env vars in SF config.

Otoh I can give my own feedback in terms of parameters.dist.yml, and it is: we are now using it for very, very few variables.
In fact in my latest projects I only use it to have values for those settings that are security-sensitive and thus should not be stored in git - all the way down to only storing secret.

The rationale is: for developer's laptops, there is no need to press enter many times, nor to be creative and screw up things just by picking a non-standard value. We use Docker anyway, for the sole purpose of making sure that dev envs are homogeneous - and similar to prod envs.
For Test/Prod environments, we rely on different ways of having the environment-related configuration injected, and none of those involves parameters.yml.dist. When it's a parameters file, it is named something else and deployed to the server by the cloud admins, generally via ansible/puppet/chef.
Another despicable side effect of using parameters.dist.yml is that devs tend to put every parameter in there, even those which are not dependent on the environment but still make sense to be defined as parameter as they make the config more sane.

PS: after speaking out my mind, I just re-read the PR, and now it seems that it in fact goes in the opposite direction of what I just said... ;-)

@gggeek
Copy link
Contributor

gggeek commented Jul 13, 2018

pps: agree with @alongosz: can add a few more #comments to the existing config files.

Fe:

config. yml:
# Put parameters here that don't need to change on each machine where the app is deployed => each environment rather than 'machine' ? or would devs confuse that with the sf 'env ' (which is often the same but not always)

default_parameters.yml: add a comment stating that this file is where the per-env parameters should be kept, and that the others are in config. yml

@Plopix
Copy link
Contributor Author

Plopix commented Jul 13, 2018

hum sounds I am the only one in favor of my PR

But I am still not convinced, let me try again ;-)

Few feedbacks first:

But maybe we could just adjust the first line # comment in parameters.yml.dist to tell Developers where to look for the full list of parameters.

That would go for my arguments. If we need a comment to explain where to look we are not in good shape.
I mean there is "best practices and things people expect". (here I am NOT saying I know what people expect ;) that the idea to discuss it in this PR

because so far I have never relied on using env vars in SF config.

well that the way to go now most of the time, with containers or platform.sh

PS: after speaking out my mind, I just re-read the PR, and now it seems that it in fact goes in the opposite direction of what I just said... ;-)

@gggeek no, not really, it does not. Actually if I understood well, just are just saying that your are using another file to store the env/security parameters. (instead of the parameters.yml)
But the parameter.yml could be the file that includes those security/env related and deployed by admins, right? That makes a lot of sense when you don't want to manage value by ENV.

Why would we use another file? just because of its name parameter and that confuses developers?

default_parameters.yml: add a comment stating that this file is where the per-env parameters should be kept, and that the others are in config. yml

Maybe the misunderstanding is here, there is not .dist.yml for the default_parameters then it is probably not in this one we should put "values"
=> that is exactly what I propose:
- in this file you put the "definition"
- in the parameters.yml the default value for the machine/env/host

By the example:

In the current state, if I want to change the MAILER_HOST default value ? what should I do?

Put the value in the ENV only
=> great but not that convenient because not everybody will have all the needed variable in the ENV

Then we need the default value, of this required parameter. What if this value have to change from developer to others (machine related). There is not .dist so they need to change in the "default_parameters.yml`
=> And now we have an issue...

You will probably argue that we can add the same line env(xxx): something in the parameters.yml to override it.
=> fine but we have 2 configurations... it is confusing and more complex to apprehend than have it ready to be changed in the parameters.yml

One more argument:

default_parameters.yml => Put parameters here that don't need to change on each machine where the app is deployed

well that is not true today. locale_fallback is application related. search_engine is probably application related. admin_group_name is as well.

And then one more question:

Why the DB configuration should have a different treatment?
(except the install multiple time per day argument?)

@anaelChardan
Copy link

Hi! IMHO I would agree with @Plopix to be sure to separate what is application related and what is not, this is also how we work in my company.

Still, I have another question, why eZ does not use http://symfony.com/doc/current/components/dotenv.html to have default environment variables in a file that is well understood by a lot of cloud providers?

@nicolas-grekas
Copy link

Makes sense to me 👍

@Plopix
Copy link
Contributor Author

Plopix commented Sep 24, 2018

there are still a lot of in the default_parameters.yml

    env(MAILER_HOST):       127.0.0.1
    env(MAILER_USER):       ~
    env(MAILER_PASSWORD):   ~
    # env(MAILER_PORT) is missing
    env(SEARCH_ENGINE): legacy
    env(SOLR_DSN): http://localhost:8983/solr
    env(SOLR_CORE): collection1
    env(LOG_PATH): "%kernel.logs_dir%/%kernel.environment%.log"
    env(CACHE_POOL): "cache.app"
    env(CACHE_DSN): localhost
    env(CACHE_NAMESPACE): ez
    env(HTTPCACHE_PURGE_SERVER): "http://localhost:80"
    env(HTTPCACHE_DEFAULT_TTL): 86400
    env(SESSION_SAVE_PATH): '%kernel.project_dir%/var/sessions/%kernel.environment%'
    env(RECOMMENDATIONS_CUSTOMER_ID):   ~
    env(RECOMMENDATIONS_LICENSE_KEY):   ~
    env(PUBLIC_SERVER_URI):             ~

would be great to have that organized in 2.3 ;-)

@SylvainGuittard
Copy link
Contributor

I agree with @Plopix on this. We should simplify where it's possible to simplify and also avoid confusion for beginners.
+1

@andrerom
Copy link
Contributor

Ok, @Plopix can you rebase and we can aim to have this in after 2.3 release?

@lserwatka
Copy link
Member

Actually, we plan to make another RC on Friday, so we could include in 2.3. We will have 1 week till final build.

@andrerom
Copy link
Contributor

andrerom commented Sep 30, 2018

That date has passed so this will be in 2.4, and this kind of changes are better done before beta phase starts anyway. You want us to rebase this @Plopix ?

@Plopix
Copy link
Contributor Author

Plopix commented Sep 30, 2018

hey @andrerom. It was a crazy week I did not have the time. I will today (that the beginning of a long sunday;) ). Agreed with 2.4, let's not do that in a rush ;)

@Plopix
Copy link
Contributor Author

Plopix commented Sep 30, 2018

finally I've just done it now @andrerom and somehow those changes are really simple finally ;) and I think they are safe. So if you are up for 2.3 why not :p

@andrerom
Copy link
Contributor

andrerom commented Oct 1, 2018

@SylvainGuittard Side, but make sure to prioritize improvements to installer soon (once we do symfony flex imho) as this will increase parameters from 7 to 24 that incenteev/composer-parameter-handler will prompt for on install.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants