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

Dockerisation: Added dockerfile and updated Readme #316

Merged
merged 6 commits into from
Sep 10, 2018

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Sep 3, 2018

Adds a basic dockerfile to the Laravel 5.6 example, allow it to be run in a local container.

This not only will allow us to get the examples running inside a container faster, but also is a stepping stone toward end-to-end/maze tests.

An updated readme for the example has also been provided.

Tests

The docker container was manually run, and confirmed to be delivering notifications to Bugsnag.

Review

The example needs to be tested so that the example can be built and run out of the box.

@Cawllec Cawllec requested review from mikeewhite and a team September 3, 2018 14:14
@Cawllec Cawllec changed the base branch from master to next September 3, 2018 14:15
Copy link

@martin308 martin308 left a comment

Choose a reason for hiding this comment

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

I added some things we should change in the dockerfile. I ran through the readme and it looks good 👍

RUN apt-get update && \
apt-get install -y --no-install-recommends git zip unzip wget

RUN mkdir /app

Choose a reason for hiding this comment

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

using the WORKDIR command below will create the required directory

RUN mkdir /app
WORKDIR /app

COPY . /app/

Choose a reason for hiding this comment

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

everything is relative from the WORKDIR once it is set so we can use relative paths here


COPY . /app/

RUN /app/install_composer.sh

Choose a reason for hiding this comment

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

same here with a relative path


RUN /app/install_composer.sh

RUN cd /app/ && \

Choose a reason for hiding this comment

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

you shouldn't need to change directory here either due to using WORKDIR

php composer-setup.php
RESULT=$?
rm composer-setup.php
exit $RESULT

Choose a reason for hiding this comment

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

would it be better to just use the composer image directly so we don't need this script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The composer image only runs composer, and isn't designed for running PHP servers unfortunately.

FROM php:latest

RUN apt-get update && \
apt-get install -y --no-install-recommends git zip unzip wget

Choose a reason for hiding this comment

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

it'll make it easier to maintain this dockerfile if we organize these like this

RUN apt-get update && \
  apt-get install -y --no-install-recommends \
  git \
  unzip \
  wget \
  zip

@Cawllec
Copy link
Contributor Author

Cawllec commented Sep 5, 2018

Thanks for the feedback Martin. I've made some updates based on your (and @mikeewhite's in person) feedback, if you could give it another quick look over, that would be great.

Copy link

@martin308 martin308 left a comment

Choose a reason for hiding this comment

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

You can certainly use the composer image to run php applications, I used it at Laracon to do that. This works though so let's go with it 😃

@snmaynard
Copy link
Contributor

You cant run the official composer image. It is just a wrapper for the cli. composer/docker#44 (comment) as an example. You might be able to use it as a base and then go from there, but I think this is better.

Copy link

@mikeewhite mikeewhite left a comment

Choose a reason for hiding this comment

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

LGTM

@Cawllec Cawllec merged commit e6088ef into next Sep 10, 2018
@Cawllec Cawllec deleted the cawllec/dockerisation branch September 10, 2018 12:51
Cawllec added a commit that referenced this pull request Nov 2, 2018
* Dockerisation: Added dockerfile and updated Readme (#316)

* Dockerisation: Added dockerfile and updated Readme

* Dockerisation: Added composer installation into docker build

* Dockerisation: More example routes/navigation

* Dockerisation:  Updated wording/flow based on feedback

* Dockerisation: Streamlined dockerfile based on feedback

* Dockerisation: Added keygen

* Add correct handled/unhandled state to notifications (#325)

* handledState: Add Laravel Un/handled state checking

* Apply fixes from StyleCI (#303)

* handledState: Add Laravel Un/handled state checking
- Add laravel test fixture for maze

* Apply fixes from StyleCI (#304)

* handledState: Add Laravel Un/handled state checking
- Add copy of testing.env to .env during build

* feature: Update maze tests

* feature: Add un/handled middleware to client

* Apply fixes from StyleCI (#309)

* feature: Removed superfluous unit test for unhandledMiddleware

* handledState: Removed maze-tests to separate branch

* maze-tests: Initial maze fixture commit

* Apply fixes from StyleCI (#319)

* Handled state: Removed gemfile

* Maze-tests: Rebuilt laravel fixture

* Maze-tests: Added gemfile and git exceptions

* Maze-tests: Added supporting maze steps and environment

* Maze-tests: Add unhandled maze feature

* Apply fixes from StyleCI (#320)

* handledState: Ensure callable  is received and called

* HandledState: fixed desired class names

* Maze-tests: Moved shared server steps into maze-runner

* Maze tests: Pushed current changes

* Install bugsnag dep from local archive

* Maze tests:  Added dependencies to package composer.json in fixture

* HandledState: Made necessary change to middleware order

* MazeTest: Required new steps from maze, qualified gitignore

* MazeTests: Added further steps to unhandled feature

* Maze-tests: Added auto copying of bugsnag-laravel requirements into fixture

* Handled State: Ensure / are escaped correctly, add dependency on PHP changes

* Handled state: Add basic handled-state tests

* Apply fixes from StyleCI (#324)

* HandledState:  Added middleware methodology comment block

* HandledState: Added scenarios for checking session counts

* Apply fixes from StyleCI (#326)

* v2.15.0-alpha-1

* HandledState: Added handled and unhandled tests for several different places in Laravel apps

* Apply fixes from StyleCI (#327)

* HandledState: Fixed fixture route typo

* v2.15.0

* DockerDelay: Added delay before pings after docker-composer up (#329)

* Revert "DockerDelay: Added delay before pings after docker-composer up (#329)" (#330)

This reverts commit 35f319b.

* v2.15.0: Bumped changelog date
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.

4 participants