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

Feature/92 Provide Makefile #94

Merged
merged 8 commits into from Oct 15, 2020
Merged

Feature/92 Provide Makefile #94

merged 8 commits into from Oct 15, 2020

Conversation

XaviTorello
Copy link

TL;DR

With this setup, we just create an specific layer with testing requirements also applied ready to work tagged locally in anem-per-feina:tests. Also it provides a make with most common development actions integrated

Fix #92 Create Makefile

Description

It provides a Makefile devised to simplify development actions:

$ make help
usage: make [target]

options:
  start                           Start all or c=<name> containers in FOREGROUND
  serve                           Start all or c=<name> containers in BACKGROUND
  test                            Execute tests
  stop                            Stop all or c=<name> containers
  restart                         Restart all or c=<name> containers
  status                          Show status of containers
  ps                              Alias of status
  clean                           Clean all containers (keeping volumes)
  purge                           Purge all containers and volumes
  build                           (re)Build all images or c=<name> containers

other:
  help                            Show this help.
  mk-upgrade                      Check for updates of mk-lib
  mk-version                      Show current version of mk-lib

The idea is that now our composition will build two local images:

  • anem-per-feina:local: with current Dockerfile definition (just requirements.txt file)
  • anem-per-feina:tests: extending local with testing requirements (processing requirements-test.txt)

This is handled with a new service named test:

  • devised just to handle local testing image creation (on build or up)
  • has no entrypoint, so it just dies when composition starts
  • it's used to dispatch temporal runs of our testing scenario via make test
    • it patchs test service entrypoint to invoke our test task and dies (will be deleted once ends with --rm)

Considerations

One option was to also process requirements-test.txt inside the base image, but this increases +53% image size:

$ docker images anem-per-feina
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
anem-per-feina      tests               d27527209a7c        14 minutes ago      317MB
anem-per-feina      local               55cd78c97c96        15 minutes ago      207MB

Remember that this base Dockerfile will be the one used for everything (CI, CD, ...)

Another option was to integrate the requirements-test.txt processing inside the app entrypoint, or inside an specific task to be invoked, but it's very slow and complicates the development experience.

We could also duplicate the docker-file with an specific testing definition, but maybe can introduce some non needed complexity. This way can be studied in short if we need it :)

Used https://github.com/krom/docker-compose-makefile to simplify Makefile definition (just 40kB)

Code formating

Done

This will prepare a testing image ready to be dispatched (with specific requirements applied)
Devised to customize our base image with testing requirements.

We splitted this in two images because testing increases +120MB layers size
So far just with dummy `manage.py test` execution
Implemented targets:
```
$ make help
usage: make [target]

options:
  start                           Start all or c=<name> containers in FOREGROUND
  serve                           Start all or c=<name> containers in BACKGROUND
  test                            Execute tests
  stop                            Stop all or c=<name> containers
  restart                         Restart all or c=<name> containers
  status                          Show status of containers
  ps                              Alias of status
  clean                           Clean all containers (keeping volumes)
  purge                           Purge all containers and volumes
  build                           (re)Build all images or c=<name> containers

other:
  help                            Show this help.
  mk-upgrade                      Check for updates of mk-lib
  mk-version                      Show current version of mk-lib
```
@XaviTorello XaviTorello added enhancement New feature or request backend Dev team arch & CI arch team labels Oct 15, 2020
@XaviTorello XaviTorello requested review from jbagot and a team October 15, 2020 09:23
@XaviTorello XaviTorello changed the title Feature/92 provide makefile Feature/92 Provide Makefile Oct 15, 2020
serve: ## Start all or c=<name> containers in BACKGROUND
@$(DOCKER_COMPOSE) -f $(DOCKER_COMPOSE_FILE) up $(c) -d

test: ## Execute tests
Copy link

Choose a reason for hiding this comment

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

Would be nice to have another option to execute an specific test or all tests from one folder as you can do it in pytest. What do you think?
I've seen that you used the django command to run the tests, but it will trigger the pytests? Can you add a parameter to add the specific test path or folder path?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, this is just an starting point :)

Tomorrow I'll explore options to inject strings or define specific string modifiers (filters).

Regarding the executed tests, it's not hardcoded, it's configurable on the invoked task so can be ammended without effort.

Currently there are no tests, pytest has not been configured and maybe it's better to deal with this in another PR

@@ -0,0 +1,95 @@
# Docker compose makefile
Copy link

Choose a reason for hiding this comment

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

I didn't know about this package, I'll check it later, but it adds a lot of files I don't know what is the added value

Copy link
Author

Choose a reason for hiding this comment

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

It speeds up targets generation, provides auto help and more enhancements.

Btw, this is not mandatory :)

@XaviTorello
Copy link
Author

The proposed solution makes sense @jbagot? Or you want to explore another solution?

I would liked to reach more feedback about the abstract solution (not about the detaila about the tests params) :)

Copy link

@jbagot jbagot left a comment

Choose a reason for hiding this comment

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

It's fine, thanks @XaviTorello

@jbagot jbagot merged commit d6e6909 into master Oct 15, 2020
@jbagot jbagot deleted the feature/92_provide_makefile branch October 15, 2020 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch & CI arch team backend Dev team enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Makefile
2 participants