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

[shopsys] Microservices webserver using nginx + php-fpm #494

Merged
merged 7 commits into from
Sep 25, 2018

Conversation

MattCzerner
Copy link
Contributor

Q A
Description, reason for the PR Microservices were started by symfony integrated server, this is not a good practice for production environment. This PR adds nginx and starts php-fpm during dockerfile build.
New feature No
BC breaks No
Fixes issues
Standards and tests pass Yes
Have you read and signed our License Agreement for contributions? Yes

Copy link
Member

@TomasLudvik TomasLudvik left a comment

Choose a reason for hiding this comment

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

Hello @MattCzerner ,

please squash first and second commit and move third commit into separate PR, thank you.

STOPSIGNAL SIGTERM

# allow "www-data" user running "php-fpm" and "nginx" as root (needed during the execution of "docker-after-start")
RUN apk add --no-cache sudo
Copy link
Member

Choose a reason for hiding this comment

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

Please join this apk and apk git with apk for libconv ... you then do not need run again apk --update and it is better because you are not creating necessary layers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, btw i deleted install of git, seems like i had some misconfiguration on my local computer

STOPSIGNAL SIGTERM

# allow "www-data" user running "php-fpm" and "nginx" as root (needed during the execution of "docker-after-start")
RUN apk add --no-cache sudo
Copy link
Member

Choose a reason for hiding this comment

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

Same as with previous comment

@MattCzerner MattCzerner force-pushed the ph-mc-microservices-webserver branch 2 times, most recently from f3927ce to 940c9cc Compare September 21, 2018 10:56
@@ -18,8 +18,7 @@ This leanest Symfony version is highly optimized and it is suitable for this typ
The microservice is responsible for feeding the Elasticsearch by product data so [product search microservice](https://github.com/shopsys/microservice-product-search) can work properly.

## Installation
Basically, in the Docker container of the microservice, all the dependencies have to be installed using `composer install` and the server has to be started by `php bin/console server:run *:8000`.
All this is automatically done when the container starts.
Microservice is installed during `docker-compose up -d` in your `project-base`, during build of image all dependencies gets installed and nginx with php-fpm si configured to allow traffic into microservice.
Copy link
Contributor

Choose a reason for hiding this comment

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

The installation of a microservice should be independent of the rest of the framework imo, I would just mention starting the container, without mentioning project-base or docker-compose.

Also, later in this section you left the mention about the native installation of microservices which still recommends using the symfony webserver - but you have uninstalled it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added commit that fixes this. Can you please look at it? Last commit will be squashed with README.md commit and i will use commit message "Updated documentation to match new state of microservices"

@MattCzerner MattCzerner force-pushed the ph-mc-microservices-webserver branch 3 times, most recently from 1093289 to a006a89 Compare September 21, 2018 14:53
Copy link
Contributor

@PetrHeinz PetrHeinz left a comment

Choose a reason for hiding this comment

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

There were inconsistencies and copy-paste mistakes in the native installation guide. In the Dockerfile sudeoers note, php-fpm wasn't mentioned. I've fixed both in a couple of commits made in the GitHub web UI. Please review them.

Also, there is still a missing artifact after the installed webserver in the second microservice, which was not removed.

@@ -2,6 +2,5 @@

return [
Symfony\Bundle\FrameworkBundle\FrameworkBundle::class => ['all' => true],
Symfony\Bundle\WebServerBundle\WebServerBundle::class => ['dev' => true],
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you haven't properly uninstalled the webserver - you still have /.web-server-pid included in the .gitignore... Why don't you use Flex?

Mathew Czerner added 4 commits September 24, 2018 13:35
- change port from 8000 to 80 in kubernetes services
- installation is inspired by official nginx dockerimage
- microservices are not dependent on symfony webserver now
@MattCzerner MattCzerner force-pushed the ph-mc-microservices-webserver branch from 527e7bb to 5c7d47f Compare September 24, 2018 11:45
Copy link
Contributor

@PetrHeinz PetrHeinz left a comment

Choose a reason for hiding this comment

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

There are just two minor problems with formatting, please fix them, then it should be OK.

\
# forward request and error logs to docker log collector
&& ln -sf /dev/stdout /var/log/nginx/access.log \
&& ln -sf /dev/stderr /var/log/nginx/error.log
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be spaces instead of tabs (it's correct in the second Dockerfile).

UPGRADE.md Outdated
@@ -18,6 +18,11 @@ There is a list of all the repositories maintained by monorepo, changes in log b
* [shopsys/microservice-product-search-export]

## [From 7.0.0-beta1 to Unreleased]
[shopsys/project-base]
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be a heading: ### [shopsys/project-base].

@PetrHeinz PetrHeinz changed the title Microservices webserver using nginx + php-fpm [shopsys] Microservices webserver using nginx + php-fpm Sep 25, 2018
Copy link
Contributor

@PetrHeinz PetrHeinz left a comment

Choose a reason for hiding this comment

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

approved and tested

@MattCzerner MattCzerner merged commit adefcda into master Sep 25, 2018
@MattCzerner MattCzerner deleted the ph-mc-microservices-webserver branch September 25, 2018 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants