Skip to content

Conversation

Fortiz2305
Copy link
Collaborator

  • Add docker-compose with external dependencies (mongodb for now)
  • Add Makefile to execute basic operations

- Add docker-compose file to be able to launch a separate database service
@Fortiz2305 Fortiz2305 force-pushed the add_dependencies_docker branch from 0a3da09 to f2d217d Compare June 11, 2020 17:20
@Fortiz2305 Fortiz2305 changed the title Add external dependencies with docker-compose Add docker-compose with external dependencies Jun 11, 2020
Copy link
Member

@JavierCane JavierCane left a comment

Choose a reason for hiding this comment

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

🙌 Good work with very great ideas in the Makefile!

I've left some suggestions that could even be wrong because I probably missed something,feel free to reject any of them 😊

Comment on lines +3 to +4
IMAGE_NAME := codelytv/typescript-ddd-skeleton
SERVICE_NAME := app
Copy link
Member

Choose a reason for hiding this comment

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

Love it. 🎩

Comment on lines +6 to +17
# Test if the dependencies we need to run this Makefile are installed
DOCKER := $(shell command -v docker)
DOCKER_COMPOSE := $(shell command -v docker-compose)
deps:
ifndef DOCKER
@echo "Docker is not available. Please install docker"
@exit 1
endif
ifndef DOCKER_COMPOSE
@echo "docker-compose is not available. Please install docker-compose"
@exit 1
endif
Copy link
Member

Choose a reason for hiding this comment

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

Very good one! 🙌


# Start mongodb container in background
start_database:
docker-compose up -d mongo
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding the Node dependencies management to the Makefile in order to make it easier to handle them from within the Node Docker container?

You have an example in the PHP repository.

In that case, we even create a temporal Docker container to run Composer (the PHP dependencies manager). This way we don't need to install Composer in the PHP container, but because Node integrates it all in the same service, we could simplify it just directly interacting with the Node Container.

The goal behind this would be to ensure we install/update dependencies using the very same Node version that executes our code in an easy way (just a make) command 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! We will take a look and include that possibility here.

Copy link
Collaborator Author

@Fortiz2305 Fortiz2305 Jul 1, 2020

Choose a reason for hiding this comment

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

Not sure if I am missing something, but I think we are getting the requirement of ensuring that we install the dependencies using the same Node version every time we launch a make command. We are getting that by setting the build step as a dependency for all the rules.

Example:

# Build image
build:
	docker build  -t $(IMAGE_NAME):dev .

# Run tests
test: build
	docker-compose run --rm $(SERVICE_NAME) bash -c 'npm run build && npm run test'

Setting the build as a dependency of the test step we have a tradeoff: This adds some time in order to build the image, but it ensures that the dependencies are correct (it uses the docker cache and it will rebuild the image in case a dependency changes). Currently, it is not taking much time once the docker image was built for the first time so we will leave as it is for now. If this is slower in the future, we can create an install rule in the Makefile as you said.

Does it make sense for you?

Fortiz2305 and others added 2 commits June 19, 2020 16:04
Co-authored-by: Javier Ferrer <javier.mailserio@gmail.com>
Co-authored-by: Javier Ferrer <javier.mailserio@gmail.com>
@Fortiz2305 Fortiz2305 merged commit 48da110 into master Jul 3, 2020
@Fortiz2305 Fortiz2305 deleted the add_dependencies_docker branch July 3, 2020 14:57
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