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

Use custom server name #81

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use custom server name #81

wants to merge 3 commits into from

Conversation

milanito
Copy link

@milanito milanito commented Nov 29, 2017

Hello,

First, let me thank you for this project.

This PR has the same goal than #68 , meaning using a custom env variable for the server name (for the moment is it symfony.dev in the config file)

Description

Nginx is not supposed to read for env variables, to do so, you need the Lua support through the dedicated module. The recommanded way to use it is to use openresty, which I did in the nginx image

Usage

A new env variable has been added to the .env.dist file, SERVER_NAME. You should update this variable to change your server name

Changes

The nginx service now uses openresty.
Because Openresty does not come with default file, I had to :

  • Create dedicated config directories
  • Add fastcgi_params and mime.types files to the project

The env variables is retrieved using the set_by_lua_block directive, using this simple block :

return os.getenv("SERVER_NAME")

Related Issue

Fix #58

Misc

This PR has been tested with this demo app, which is in 3.1, and an internal application (not from auth0), which is in 3.3 .

The applications have been tested both on Mac OS X and Ubuntu.

Thank you for reading 😄

@PyRowMan
Copy link

Nice one here @milanito !

I'm working on an improvement to add more than one symfony apps.

I'll be waiting for your PR to be accepted to integrate your changes in my working copy.

@maxpou
Copy link
Owner

maxpou commented Jan 31, 2018

@PyRowMan does this PR works on your environment?

@PyRowMan
Copy link

@maxpou I did not integrated it yet for now

My docker project integration is currently in hold because our work machines are lacking some RAM; but it's planned to integrate and test it as soon as i receive the hardware :)

@maxpou
Copy link
Owner

maxpou commented Jan 31, 2018

cool, I'm looking forward then!

@milanito
Copy link
Author

milanito commented Feb 1, 2018

@PyRowMan you're welcome 😄

Tell me if you have any issue, but I think you could make it run on your env.

We have been deploying a staging env for our application using this PR code, for the moment we have had no issue, but this is an env we tailored to our needs

@PyRowMan
Copy link

PyRowMan commented Feb 1, 2018

@maxpou I am too, i'm really tired of working with that shitty wampserver uU

@milanito I haven't actually even installed the PR for now, but i have seen the commit files and there is no probleme i can foresee.

Nonetheless i have a problematic that i can't solve :

At first my idea for integrating more than one app into the project was to creating a new docker container for each symfony app.

But after trying that solution I had ran into some issues.

I then got back to the temporary easy standard "all symfony apps in the same container".

So i'm asking it now: what are you thinking is the best solution ?

Btw : One of the problems with the second solution is that you can't set the workdir to the symfony app directly... And i'm annoyed with it.

@milanito
Copy link
Author

milanito commented Feb 2, 2018

@PyRowMan I think the first solution is the one I'd use. But in this case, you might have to change several things :

  • You'd have to update your nginx configuration with a new configuration file for your new application
  • Update the nginx docker file with ADD your_new_conf.conf /etc/nginx/sites-available/ and RUN ln -s /etc/nginx/sites-available/your_new_conf.conf /etc/nginx/sites-enabled/your_new_conf
  • Update the docker compose file, by adding a new service for each application, in which the most important part is the volume one, in particular :- ${SYMFONY_APP_PATH_2}:/var/www/your_new_conf
  • Don't forget to update the volumes_from: entry for the nginx and elk volumes
  • Update the .env file with the value of SYMFONY_APP_PATH_2 (But I think it might work with the same value)

I think with all that it should work.

Anyway, I'm not sure it's the right place for this discussion 😄 maybe after this PR is merged, you could open an issue for that ?

@PyRowMan
Copy link

PyRowMan commented Feb 2, 2018

@milanito Yes, you are totally right ! I'll do that as soon as I have the time to write the right issue.

About that PR, I tested your branch yesterday, here is my review :

Note that I had activated the opcache before merging you PR.

I had less performances after merging your branch, something like 0.3 seconds more loading page on a standard symfony page.

Do you have any idea where that can come from ?

@milanito
Copy link
Author

milanito commented Feb 2, 2018

@PyRowMan uhm, to be honest no 😄

Could you provide maybe a benchmark comparing both env (before and after the PR) ? I though it'd might be openresty but it's supposed to be even better.

@PyRowMan
Copy link

PyRowMan commented Feb 4, 2018

@milanito Today my environnement don't want to play I think. I was at like 1.5 sec loading time yesterday; today i'm at 17 sec loading time.

But when i tested it there was no real difference between the two branches. (0.3 seconds like I said)

I'll make a Issue to integrate opcache and test the perfs.

Copy link

@PyRowMan PyRowMan left a comment

Choose a reason for hiding this comment

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

This branch work well on my machine, there is two merging conflict but you can accept the one from this branch :)

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

Successfully merging this pull request may close these issues.

3 participants