-
Notifications
You must be signed in to change notification settings - Fork 73
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
Laravel 4.2 sample app with both apache and nginx/fpm docker files #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
one question, would it be worthwile to change the docker images to something more official than FROM alfa30/laravel:4.2
?
I’m worried that those could in theory disappear or get overwritten by something worse
This is a very good idea. The others required a little bit more work, so I took the shortcut 😄 but I want to do it because it makes a lot of sense. |
@pawelchcki I based the images on It would be great to have the final round of CR and merge it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job @labbati! I have a few questions/tweaks, but everything else is looking good! :)
|
||
Start up an agent: you have to set the env `DATADOG_API_KEY` with your own DD api key. | ||
|
||
$ docker-composer up -d agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I make this type-o so often that I tweeted about it. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
$ docker-compose build laravel42_php56_apache | ||
|
||
# Php 5.6 - Nginx/fpm - Laravel 4.2 | ||
$ docker-compose build laravel42_php56_nginx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the nginx config in the docker-compose.yml
file. Should we remove the references to nginx? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, to be removed, was there before. Good catch
*/ | ||
|
||
Route::get('/', ['uses' => 'HomeController@showWelcome', 'as' => 'simple_route_name']); | ||
Route::get('/distributed-tracing', 'HomeController@distributedTracing'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get an Apache-404 page when I try to visit this URL. I'm wondering if we're missing the Apache config for mod_rewrite? We should probably mention this route in the README, or was this just a left over test? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably I left mod_rewrite config out when moving from the preconfigured image to the one based on php
base images. Thanks for noticing. Looking at it
Add a docs line to hint the user at how the app can now been accessed Co-Authored-By: labbati <luca.abbati@datadoghq.com>
The very first commit is the Laravel application as created by the composer command in the getting started guide of the framework.
The real changes are here: dd05647...949eea7