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

[DI] Allow ENV vars to be not defined when compiling the container #25010

Closed
goetas opened this Issue Nov 17, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@goetas
Contributor

goetas commented Nov 17, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? maybe
RFC? yes
Symfony version 3.3, 3.4, 4.0

I have a CI build & run pipeline.

  • The cache is warmed up in the build process (using docker, withing the docker image build)
  • Environment variables are used to configure the application

The current implementation of the DI component requires to have all the ENV variables used in the config files as "defined" in the warmup environment, and this is really annoying...

I'm forced by this the to re-define all of them (with some default values) inside a file like parameters.yml just to be able to warm up the cache

There are too many env files to maintain:

  1. Env variables are used "here and there" in the config files.
  2. a list of env variables is available inside .env.dist
  3. the customized env variables for development are stored inside the .env file
  4. parameters.yml to workaround the warmup issue

Can the variable existence check be omitted? Default to null?

It is a common thing that in the "build" environment env variables are different from the "run" envs and having to define them just to have a default is annoying.

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Nov 19, 2017

Contributor

This will be a problem for the Doctrine bundle as it expects to have a server-version 🤔
And sometimes your warmers need some configuration as well.

You should be able to use the .env.dist file if you need something default.

Contributor

sstok commented Nov 19, 2017

This will be a problem for the Doctrine bundle as it expects to have a server-version 🤔
And sometimes your warmers need some configuration as well.

You should be able to use the .env.dist file if you need something default.

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Nov 19, 2017

Member

This is tricky. Yes, we do want the user to be able to build their container without being in an environment that has all the env vars. But my current opinion is that you should simply not have any services used during the cache warmup process that require env variables (this logically makes sense). If you somehow are trying to use a service that needs an env var at warmup, that’s a bug.

What env vars do you see being required?

About the Doctrine situation, there is a recipe PR (by me) that may fix this requirement.

Member

weaverryan commented Nov 19, 2017

This is tricky. Yes, we do want the user to be able to build their container without being in an environment that has all the env vars. But my current opinion is that you should simply not have any services used during the cache warmup process that require env variables (this logically makes sense). If you somehow are trying to use a service that needs an env var at warmup, that’s a bug.

What env vars do you see being required?

About the Doctrine situation, there is a recipe PR (by me) that may fix this requirement.

@goetas

This comment has been minimized.

Show comment
Hide comment
@goetas

goetas Nov 19, 2017

Contributor

This will be a problem for the Doctrine bundle as it expects to have a server-version 🤔
And sometimes your warmers need some configuration as well.

The doctrine warmup needs to know only the server version, all the other parameters as username, host or password are useless.
Probably I'm arriving a bit late but I have to go trough the issues/PR history and see what are the advantages over DATABASE_URL vs DB_* parameters. I see only disadvantages as urlencoding issues with passwords/usernames ans coupling.

In this case my env file could be just

SERVER_VERSION=5.7

While now i'm forced to have

DATABASE_URL=mysql://notused:notused@notused:notused/notused?charset=utf8mb4&serverVersion=5.7

Another example:
before:

DB_PASSWORD=s458(&=65?w

after:

DATABASE_URL=mysql://notused:s458%28%26%3D65%3Fw@notused:notused/notused?charset=utf8mb4&serverVersion=5.7

s458%28%26%3D65%3Fw is the urlencoded s458(&=65?w... much les unclear.

You should be able to use the .env.dist file if you need something default.

This could be a solution, but the env.dist file was not meant to be used in the codebase, is just to help devs to setup their own variables.

@weaverryan

But my current opinion is that you should simply not have any services used during the cache warmup process that require env variables (this logically makes sense). If you somehow are trying to use a service that needs an env var at warmup, that’s a bug.

I disagree. Env vars are just configurations and the cache warmpup might need configurations too.
The serverVersion probably should not be in the environment variables as most probably the database-version is not something that changes so often and cold be hardcoded in the config.yml file (but because of DATABASE_URL, now has to be a single gigant obscure "URL")

If a warmer needs configurations why not allow them to have it.

EDIT: Ok, I see that the old configurations in the doctrine-bundle are still available so the server_version can be specified next to the database-url. This solves the doctrine warmup but the issue with the env vars defaults is still there

Contributor

goetas commented Nov 19, 2017

This will be a problem for the Doctrine bundle as it expects to have a server-version 🤔
And sometimes your warmers need some configuration as well.

The doctrine warmup needs to know only the server version, all the other parameters as username, host or password are useless.
Probably I'm arriving a bit late but I have to go trough the issues/PR history and see what are the advantages over DATABASE_URL vs DB_* parameters. I see only disadvantages as urlencoding issues with passwords/usernames ans coupling.

In this case my env file could be just

SERVER_VERSION=5.7

While now i'm forced to have

DATABASE_URL=mysql://notused:notused@notused:notused/notused?charset=utf8mb4&serverVersion=5.7

Another example:
before:

DB_PASSWORD=s458(&=65?w

after:

DATABASE_URL=mysql://notused:s458%28%26%3D65%3Fw@notused:notused/notused?charset=utf8mb4&serverVersion=5.7

s458%28%26%3D65%3Fw is the urlencoded s458(&=65?w... much les unclear.

You should be able to use the .env.dist file if you need something default.

This could be a solution, but the env.dist file was not meant to be used in the codebase, is just to help devs to setup their own variables.

@weaverryan

But my current opinion is that you should simply not have any services used during the cache warmup process that require env variables (this logically makes sense). If you somehow are trying to use a service that needs an env var at warmup, that’s a bug.

I disagree. Env vars are just configurations and the cache warmpup might need configurations too.
The serverVersion probably should not be in the environment variables as most probably the database-version is not something that changes so often and cold be hardcoded in the config.yml file (but because of DATABASE_URL, now has to be a single gigant obscure "URL")

If a warmer needs configurations why not allow them to have it.

EDIT: Ok, I see that the old configurations in the doctrine-bundle are still available so the server_version can be specified next to the database-url. This solves the doctrine warmup but the issue with the env vars defaults is still there

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 19, 2017

Member

not have any services used during the cache warmup process that require env variables

actually I agree with this statement: reading the env at build time means hardcoding a value that should be dynamic in the first place. Reading an env var as part of the cache warmup process defeats the usefulness of using en vars, so that a simple parameter should be used instead.

env vars should be used only for infrastructure related configuration, ones that change between the build machine and the prod or dev machines.

for app-related configuration, regular parameters should be used instead.

Member

nicolas-grekas commented Nov 19, 2017

not have any services used during the cache warmup process that require env variables

actually I agree with this statement: reading the env at build time means hardcoding a value that should be dynamic in the first place. Reading an env var as part of the cache warmup process defeats the usefulness of using en vars, so that a simple parameter should be used instead.

env vars should be used only for infrastructure related configuration, ones that change between the build machine and the prod or dev machines.

for app-related configuration, regular parameters should be used instead.

@goetas

This comment has been minimized.

Show comment
Hide comment
@goetas

goetas Nov 19, 2017

Contributor

@nicolas-grekas ok, I got your point.

But then the question is: How to allow to run cache:warmup in the build env without having to declare all the env vars that are need just at run time? (this is the current behavior and the reason behind this issue)

My proposal was to just set them to null if they are not declared in the build environment.

Contributor

goetas commented Nov 19, 2017

@nicolas-grekas ok, I got your point.

But then the question is: How to allow to run cache:warmup in the build env without having to declare all the env vars that are need just at run time? (this is the current behavior and the reason behind this issue)

My proposal was to just set them to null if they are not declared in the build environment.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 19, 2017

Member

for DATABASE_URL, @weaverryan has a solution, which is setting the serverVersion using a regular parameter, then ensuring that this is enough for Doctrine to not need a connection, even uninitialized (could be the case, dunno).
What kind of services are you using at warmup time that are configured with env vars? Shouldn't they be configured using parameters instead? You should challenge the need for each env there.

Member

nicolas-grekas commented Nov 19, 2017

for DATABASE_URL, @weaverryan has a solution, which is setting the serverVersion using a regular parameter, then ensuring that this is enough for Doctrine to not need a connection, even uninitialized (could be the case, dunno).
What kind of services are you using at warmup time that are configured with env vars? Shouldn't they be configured using parameters instead? You should challenge the need for each env there.

@goetas

This comment has been minimized.

Show comment
Hide comment
@goetas

goetas Nov 19, 2017

Contributor

for DATABASE_URL, @weaverryan has a solution, which is setting the serverVersion using a regular parameter, then ensuring that this is enough for Doctrine to not need a connection, even uninitialized (could be the case, dunno).

Yes i kwew it and i've commented it here #24927 (comment)

What kind of services are you using at warmup time that are configured with env vars? Shouldn't they be configured using parameters instead? You should challenge the need for each env there.

actually no services are needed, it is just that the container won't compile if the used env variables are not defined.

Contributor

goetas commented Nov 19, 2017

for DATABASE_URL, @weaverryan has a solution, which is setting the serverVersion using a regular parameter, then ensuring that this is enough for Doctrine to not need a connection, even uninitialized (could be the case, dunno).

Yes i kwew it and i've commented it here #24927 (comment)

What kind of services are you using at warmup time that are configured with env vars? Shouldn't they be configured using parameters instead? You should challenge the need for each env there.

actually no services are needed, it is just that the container won't compile if the used env variables are not defined.

@goetas

This comment has been minimized.

Show comment
Hide comment
@goetas

goetas Nov 20, 2017

Contributor

I've did a second check on my app and I have to admit that you were right.
If a service uses an env var in the build step but the variable is not available, makes sense to have the error.

Thanks for the support and patience, I think my issue was "invalid", Maybe a bit influenced by the "DATABASE_URL" unexpected behaviour but after setting "serverVersion" things are going well!

Closing the issue

Contributor

goetas commented Nov 20, 2017

I've did a second check on my app and I have to admit that you were right.
If a service uses an env var in the build step but the variable is not available, makes sense to have the error.

Thanks for the support and patience, I think my issue was "invalid", Maybe a bit influenced by the "DATABASE_URL" unexpected behaviour but after setting "serverVersion" things are going well!

Closing the issue

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