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.

Sign up for GitHub

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

Jump to bottom

build dir as order only prerequisite, docker rmi deduplicated, docker… #199

Closed
wants to merge 1 commit into from
Closed

build dir as order only prerequisite, docker rmi deduplicated, docker… #199

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 23 additions & 30 deletions containers/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ VERIFIER_SOURCE := \
build/verifier/Dockerfile \
$(VERIFIER_SOURCE_FROM_CONCENT_API)

DOCKER_BUILD := \
docker build --tag $(IMAGE_PREFIX)$@:$(IMAGE_VERSION) build/$@/

DOCKER_TAG := \
docker tag $(IMAGE_PREFIX)$@:$(IMAGE_VERSION) $(IMAGE_PREFIX)$@:latest

ifndef IMAGE_VERSION
IMAGE_VERSION := $(shell git describe --always --abbrev=16)
Expand All @@ -59,42 +64,30 @@ $(info IMAGE_PREFIX=$(IMAGE_PREFIX))

all: nginx-proxy nginx-storage concent-api postgresql verifier

nginx-proxy: build/ $(NGINX_PROXY_SOURCE) build/nginx-proxy/concent-api-assets.tar build/nginx-proxy/concent-api-docs.tar
docker build --tag $(IMAGE_PREFIX)nginx-proxy:$(IMAGE_VERSION) build/nginx-proxy/
docker tag $(IMAGE_PREFIX)nginx-proxy:$(IMAGE_VERSION) $(IMAGE_PREFIX)nginx-proxy:latest

nginx-storage: build/ $(NGINX_STORAGE_SOURCE)
docker build --tag $(IMAGE_PREFIX)nginx-storage:$(IMAGE_VERSION) build/nginx-storage/
docker tag $(IMAGE_PREFIX)nginx-storage:$(IMAGE_VERSION) $(IMAGE_PREFIX)nginx-storage:latest

postgresql: build/ $(POSTGRESQL_SOURCE)
docker build --tag $(IMAGE_PREFIX)postgresql:$(IMAGE_VERSION) build/postgresql/
docker tag $(IMAGE_PREFIX)postgresql:$(IMAGE_VERSION) $(IMAGE_PREFIX)postgresql:latest

concent-api: build/ $(CONCENT_API_SOURCE) build/concent-api/requirements.lock build/concent-api/testing.pref build/repositories/concent/
docker build --tag $(IMAGE_PREFIX)concent-api:$(IMAGE_VERSION) build/concent-api/
docker tag $(IMAGE_PREFIX)concent-api:$(IMAGE_VERSION) $(IMAGE_PREFIX)concent-api:latest

verifier: build/ $(VERIFIER_SOURCE) build/repositories/golem/ build/verifier/golem/scripts/ build/verifier/golem/imgverifier-requirements.txt
docker build --tag $(IMAGE_PREFIX)verifier:$(IMAGE_VERSION) build/verifier/
docker tag $(IMAGE_PREFIX)verifier:$(IMAGE_VERSION) $(IMAGE_PREFIX)verifier:latest
nginx-proxy: $(NGINX_PROXY_SOURCE) build/nginx-proxy/concent-api-assets.tar build/nginx-proxy/concent-api-docs.tar | build/
$(DOCKER_BUILD)
$(DOCKER_TAG)
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding | build/ is a change that has nothing to do with other changes in this commit. So it should be in a separate commit.

Actually, it would be best to make a separate pull request. Keeping things separate makes it easier to review them. That's why there were two separate issues an not one.


docker-clean: nginx-proxy-clean concent-api-clean postgresql-clean verifier-clean
nginx-storage: $(NGINX_STORAGE_SOURCE) | build/
$(DOCKER_BUILD)
$(DOCKER_TAG)
Copy link
Contributor

@cameel cameel Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This does reduce duplication a bit but has two downsides:

  • You still have to repeat those three lines for every image.
  • The variables used like this look quite strange and opaque. This works but putting whole bash lines into variables feels kinda like you're trying really hard to create a function.

I can suggest two alternate approaches.

Bash script

Instead of defining the variables you could simply make a bash script that takes image name, version and everything else as arguments.

Static pattern rules

I'd actually prefer to reorganize the makefile and use Static Pattern Rules. That's how we dealt with it in one of the earlier projects.

Define a list of target names and then automatically declare the targets with a static pattern rule. Prerequisites can be added using a special intermediate target.

Here's an incomplete example:

IMAGE_TARGETS := \
    nginx-proxy  \
    concent-api  \
    postgresql

nginx-proxy!dependencies:                    \
    $(NGINX_PROXY_SOURCE)                    \
    build/nginx-proxy/concent-api-assets.tar \
    build/nginx-proxy/concent-api-docs.tar

concent-api!dependencies:               \
    $(CONCENT_API_SOURCE)               \
    build/concent-api/requirements.lock \
    build/concent-api/testing.pref      \
    build/repositories/concent/

postgresql!dependencies: \
    $(POSTGRESQL_SOURCE)

IMAGE_DEPENDENCY_TARGETS := $(addsuffix !dependencies, $(IMAGE_TARGETS))

all: images

$(IMAGE_TARGETS): %: %!dependencies build/
    docker build --tag $(IMAGE_PREFIX)$@:$(IMAGE_VERSION) build/$@/
    docker tag $(IMAGE_PREFIX)$@:$(IMAGE_VERSION) $(IMAGE_PREFIX)$@:latest

.PHONY:                         \
    all                         \
    $(IMAGE_TARGETS)            \
    $(IMAGE_DEPENDENCY_TARGETS)


nginx-proxy-clean:
docker rmi $(IMAGE_PREFIX)nginx-proxy:$(IMAGE_VERSION) --force
postgresql: $(POSTGRESQL_SOURCE) | build/
$(DOCKER_BUILD)
$(DOCKER_TAG)

nginx-storage-clean:
docker rmi $(IMAGE_PREFIX)nginx-storage:$(IMAGE_VERSION) --force
concent-api: $(CONCENT_API_SOURCE) build/concent-api/requirements.lock build/concent-api/testing.pref build/repositories/concent/ | build/
$(DOCKER_BUILD)
$(DOCKER_TAG)

postgresql-clean:
docker rmi $(IMAGE_PREFIX)postgresql:$(IMAGE_VERSION) --force
verifier: $(VERIFIER_SOURCE) build/repositories/golem/ build/verifier/golem/scripts/ build/verifier/golem/imgverifier-requirements.txt | build/
$(DOCKER_BUILD)
$(DOCKER_TAG)

concent-api-clean:
docker rmi $(IMAGE_PREFIX)concent-api:$(IMAGE_VERSION) --force
docker-clean: nginx-proxy-rmi nginx-storage-rmi postgresql-rmi concent-api-rmi verifier-rmi
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep whitespace consistent.


verifier-clean:
docker rmi $(IMAGE_PREFIX)verifier:$(IMAGE_VERSION) --force
%-rmi:
docker rmi $(IMAGE_PREFIX)$*:$(IMAGE_VERSION) --force
Copy link
Contributor

Choose a reason for hiding this comment

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

Any target that does not really create a file with the corresponding name must be a Phony target.

Copy link
Contributor

Choose a reason for hiding this comment

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

The change to the clean targets should be in a separate commit from the change to the image targets. They're independent of each other (one does not really require the other to work) so they can be separated easily. And keeping them in the same commit makes review harder - just look how changes from one are mixed with changes from the other in the diff.


build/virtualenv/: build/repositories/concent/concent_api/requirements.lock build/package-builder/build-virtualenv.sh
build/package-builder/build-virtualenv.sh "$<" "build"
Expand Down