Skip to content
This repository has been archived by the owner on Apr 3, 2022. It is now read-only.

[fix] Issues with composer and enhanced makefile #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jorgemuza
Copy link

-Fix small typos in README file
-Enhanced docker file to copy composer on app container
-Enhanced makefile to remove orphans and volumes

-Fix small typos in README file
-Enhanced docker file to copy composer on app container
-Enhanced makefile to remove orphans and volumes
@@ -18,6 +18,7 @@ FROM php:7.4-fpm-alpine

WORKDIR /var/www/html

COPY --from=vendor /usr/bin/composer /usr/bin/composer
Copy link
Owner

@munza munza Jan 8, 2022

Choose a reason for hiding this comment

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

@jorgemuza Thanks for the PR 😄

The composer executable was intentionally not copied in the image. The image is intended to be used as a production image and don't require composer. We can use make composer to start an sh session with composer command available.

If there is a need for composer command to be available in prod environment then it can be added additionally and not copied by default.

I would suggest to either not copy it in the image or at least comment it out and add a note above so it can be used if necessary. Eg.

# Notes about not having it in the image...
# COPY --from=vendor /usr/bin/composer /usr/bin/composer

@munza
Copy link
Owner

munza commented Jan 8, 2022

@jorgemuza

Loved the changes in the Makefile 👍
And fixing those typos 💯

Comment on lines +22 to +25
@docker-compose down -v

clean: ## Clean all Docker services
@docker rmi -f lumen-api-starter-app
Copy link
Owner

@munza munza Jan 8, 2022

Choose a reason for hiding this comment

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

Removing volumes with -v is risky. Developer may not want to delete the volumes when they want to remove only the instances with down command.

I will suggest the following instead -

down: ## Down all Docker services
    @docker-compose down

remove: ## Remove all Docker instances, images, volumes and networks
     @docker-compose down -v
     @docker rmi -f lumen-api-starter-app

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

Successfully merging this pull request may close these issues.

2 participants