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

[ghost]

docker build and docker tag deduplicated
build dir as order only prerequisite,
docker rmi deduplicated

Resolves: #194, #193

[kbeker]

Please if you adding PR write which issue you resolve.
e.g.

Resolves #issue_number

Also please keep up with ours branch naming style :) : Code-Poets/coding-style#13

[cameel]

Resolves: #194, #193

This does not work like this. Please see Closing issues using keywords.

To close multiple issues, preface each issue reference with one of the above keywords. You must use the keyword before each issue you reference for the keyword to work.

For example, This closes #34, closes #23, and closes example_user/example_repo#42 would close issues #34 and #23 in the same repository, and issue #42 in the "example_user/example_repo" repository.

[cameel]

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.

[cameel]

This pull request still needs some work:

• Two separate issues worth of changes are stuffed into a single commit.
• I think putting whole bash commands into variables is not the best way to go. I suggested two alternate approaches in my comment.

[cameel]

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

[cameel]

Please keep whitespace consistent.

[cameel]

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.

[cameel]

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)

[ghost]

superseded by #211