Skip to content

Conversation

@jkblume
Copy link
Contributor

@jkblume jkblume commented Dec 12, 2018

I wondered on the comment in the Dockerfile, which says to install the composer dependencies before building the image.

Maybe it is an option for you to use a multi-stage Dockerfile, like i proposed in my PR :) I would appreciate to hear your thoughts about that.

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #694 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #694   +/-   ##
=========================================
  Coverage     82.17%   82.17%           
  Complexity      935      935           
=========================================
  Files            44       44           
  Lines          2154     2154           
=========================================
  Hits           1770     1770           
  Misses          384      384

Copy link
Owner

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Nice!

Dockerfile Outdated
RUN composer install

# Running this container will start a language server that listens for TCP connections on port 2088
# Every connection will be run in a forked child process
Copy link
Owner

Choose a reason for hiding this comment

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

Can you keep this comment at the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes of course :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the comments back to the top. I also replaced the deprecated maintainer with the appropriate "maintainer" label.

@felixfbecker felixfbecker merged commit 7303143 into felixfbecker:master Dec 12, 2018
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.

2 participants