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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse environment variables on build configuration #1667

Merged
merged 2 commits into from Aug 9, 2018
Merged

Parse environment variables on build configuration #1667

merged 2 commits into from Aug 9, 2018

Conversation

maturanomx
Copy link
Contributor

@maturanomx maturanomx commented Jul 31, 2018

My PR is a: 馃悰 Bug fix

Main update on the: Framework


Currently the use of environment variables in the configuration files is allowed, however the values are not replaced building the administration panel. You can clearly see the error if you try to use an environment variable as the value of the host parameter in the server.json file:

serverconfig_withhttp

As can see, the value was used as an ordinary string; they were not replaced by the value of the environment variable.

This PR moves the code that makes that interpretation to strapi-utils trying to make it a shared functionality. In addition to been invoked originally in the same place, with this changes also it's called by webpack, getting a consistent result in the build.

You can see here the result of this change:

serverconfig_patch

Notes:

  • I keep the name of the function. Could have a better name maybe?
  • Is strapi-utils the correct place? 馃

close #1590

@derrickmehaffy
Copy link
Member

@maturano Is this for loading config values from environment variables for use in something like Docker?

@maturanomx
Copy link
Contributor Author

I was not thinking specifically in Docker, but I think could be also a use case.

I used environment variables on the server.json file and there is not been interpreted on the build job (as part of a CI pipeline). Not in all parts... this fix that.

On #1590 I documented some cases and their result. I'll put the result of this PR hoping makes clear why should be included.

@derrickmehaffy
Copy link
Member

@maturano good thinking 馃槈

@maturanomx
Copy link
Contributor Author

@derrickmehaffy I didn't see any new comments on the original issue. I updated the description of this PR hopping having better visibility.

@lauriejim lauriejim self-assigned this Aug 6, 2018
@lauriejim lauriejim requested a review from soupette August 6, 2018 10:27
@lauriejim lauriejim added pr: 馃拝 Enhancement source: core:strapi Source is core/strapi package labels Aug 6, 2018
Copy link
Member

@Aurelsicoko Aurelsicoko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! 馃憤

Move existing code to a shared library and invoke it on the build
process to parse environment variables on configuration files.

Looking for a consistent behavior.

close #1590
@maturanomx
Copy link
Contributor Author

Glad to help :)

I just made a rebase with master to ensure a clean merge 馃憤

@lauriejim lauriejim added this to In progress in 3.0.0-alpha.14 via automation Aug 9, 2018
@lauriejim lauriejim added this to the 3.0.0-alpha.13.2 milestone Aug 9, 2018
@lauriejim lauriejim merged commit d30b03f into strapi:master Aug 9, 2018
3.0.0-alpha.14 automation moved this from In progress to Done Aug 9, 2018
@maturanomx maturanomx deleted the bug/1590 branch August 22, 2018 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source: core:strapi Source is core/strapi package
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Enviroment variables not replaced on server.json config file
4 participants