NOISSUE - Add MF_DOCKER_IMAGE_NAME_PREFIX to Makefile #1173

sprql · 2020-05-12T14:22:16Z

Allow custom docker image names

manuio · 2020-05-12T17:30:18Z

Makefile

@@ -7,6 +7,7 @@ SERVICES = users things http coap lora influxdb-writer influxdb-reader mongodb-w
 	bootstrap opcua authn twins mqtt provision
 DOCKERS = $(addprefix docker_,$(SERVICES))
 DOCKERS_DEV = $(addprefix docker_dev_,$(SERVICES))
+DOCKER_IMAGE_NAME_PREFIX ?= ""


I would use mainflux as prefix here and I would remove it where it's hard-coded.

I've thought about it but I'm confused about what to do with /:
should be / part of DOCKER_IMAGE_NAME_PREFIX or not.

I prefer to keep it outside but let's see what others think about

@manuio I think mainflux should be default, and then <docker_registry_url>/mainflux can be used for other cases.

I preferred previous implementation, looks cleaner and more configurable.

@drasko I'm no sure to understand. I proposed DOCKER_IMAGE_NAME_PREFIX ?= "mainflux"
and $(DOCKER_IMAGE_NAME_PREFIX)/$(svc) to simplify an eventual renaming and because it's hard-coded for many commands.

Ah, cool - then we are on the same page.

Good, I made MF_DOCKER_IMAGE_NAME_PREFIX ?= mainflux/

We proposed mainflux, not mainflux/ ;)

ok, updated:

MF_DOCKER_IMAGE_NAME_PREFIX ?= mainflux

drasko · 2020-05-27T20:46:28Z

I do not see a great value of this PR in the core (mostly because of additional code complexity for no big gains).

@sprql what is the particular use-case that you have to name images differently?

I think that this might be some particular use-case, which can probably be solved with a simple custom script (or even shell one-liner) that renames Mainflux images, but I would like to first hear from you the requirement.

sprql · 2020-06-01T13:31:35Z

@drasko It's very handy for custom builds, for example when using different docker registry.
Yes, it is possible to create custom shell scripts, but isn't it easier to solve this in the first place, in Makefile?

I think it's better to introduce MF_DOCKER_IMAGE_NAME_PREFIX env var, so it will be easy to export it in a shell.

drasko · 2020-06-01T14:19:15Z

...for example when using different docker registry.

Can you please elaborate on this, because I think that even in the private registries (non-DockerHub) you can have mainflux/<service_name>. Or am I wrong?

sprql · 2020-06-01T15:40:48Z

@drasko, as I know, to push an image to non-DockerHub registry I should specify host and port in the Docker image name. So to push mainflux/service:v1 to registry.example.com I should tag the image with registry.example.com/mainflux/service:v1 and then push it as registry.example.com/mainflux/service:v1

https://docs.docker.com/engine/reference/commandline/push/

Maybe there is another approach which I don't know…

drasko · 2020-06-01T16:07:15Z

I see... I thought that you can give a param to docker push in some other way, but apparently not.

OK, this is good enough reason to add this prefix to the core. Please update the branch and I will do the review.

drasko · 2020-06-01T16:13:23Z

Makefile

@@ -7,6 +7,7 @@ SERVICES = users things http coap lora influxdb-writer influxdb-reader mongodb-w
 	bootstrap opcua authn twins mqtt provision
 DOCKERS = $(addprefix docker_,$(SERVICES))
 DOCKERS_DEV = $(addprefix docker_dev_,$(SERVICES))
+DOCKER_IMAGE_NAME_PREFIX ?= ""


@manuio I think mainflux should be default, and then <docker_registry_url>/mainflux can be used for other cases.

I preferred previous implementation, looks cleaner and more configurable.

Makefile

drasko · 2020-06-02T20:22:08Z

Makefile

@@ -1,6 +1,7 @@
 # Copyright (c) Mainflux
 # SPDX-License-Identifier: Apache-2.0

+MF_DOCKER_IMAGE_NAME_PREFIX ?= mainflux/


Why not just mainflux? I would prefer that service is always prefixed with some name, then followed by /, so you can have $(MF_DOCKER_IMAGE_NAME_PREFIX)/$(svc)

It is more readable then concatenated like this.

I thought it would be more flexible

drasko

LGTM

manuio

LGTM

manuio · 2020-06-03T08:58:03Z

@sprql update your branch please

manuio · 2020-06-03T15:17:00Z

@sprql Please, can you update again your branch? We had an issue with the CI...

Signed-off-by: Alexander Obukhov <[email protected]>

codecov-commenter · 2020-06-03T18:11:28Z

Codecov Report

Merging #1173 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
+ Coverage   77.23%   77.25%   +0.02%     
==========================================
  Files         102      102              
  Lines        6816     6816              
==========================================
+ Hits         5264     5266       +2     
+ Misses       1182     1180       -2     
  Partials      370      370

Impacted Files	Coverage Δ
things/service.go	`86.33% <0.00%> (+1.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7c3cfc...87df445. Read the comment docs.

drasko · 2020-06-03T18:15:20Z

Makefile


 	# Remove unused images
-	docker images "mainflux\/*" -f dangling=true -q | xargs -r docker rmi
+	docker images "$(MF_DOCKER_IMAGE_NAME_PREFIX)\/*" -f dangling=true -q | xargs -r docker rmi


Are the quotes really needed in "$(MF_DOCKER_IMAGE_NAME_PREFIX)\/*" or can be removed?

I think it depends on that is sh on your system

manuio

LGTM

drasko

LGTM

Signed-off-by: Alexander Obukhov <[email protected]>

sprql requested a review from a team as a code owner May 12, 2020 14:22

manuio suggested changes May 12, 2020

View reviewed changes

drasko requested changes Jun 1, 2020

View reviewed changes

manuio reviewed Jun 1, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

sprql force-pushed the feature/docker-tag-prefix branch from 81be906 to 6651e2c Compare June 2, 2020 13:55

sprql changed the title ~~NOISSUE - Add DOCKER_IMAGE_NAME_PREFIX to Makefile~~ NOISSUE - Add MF_DOCKER_IMAGE_NAME_PREFIX to Makefile Jun 2, 2020

drasko requested changes Jun 2, 2020

View reviewed changes

sprql force-pushed the feature/docker-tag-prefix branch from 6651e2c to d5df272 Compare June 3, 2020 08:38

drasko previously approved these changes Jun 3, 2020

View reviewed changes

manuio previously approved these changes Jun 3, 2020

View reviewed changes

sprql dismissed stale reviews from manuio and drasko via 117c7ea June 3, 2020 09:27

sprql force-pushed the feature/docker-tag-prefix branch from d5df272 to 117c7ea Compare June 3, 2020 09:27

NOISSUE - Add MF_DOCKER_IMAGE_NAME_PREFIX to Makefile

87df445

Signed-off-by: Alexander Obukhov <[email protected]>

sprql force-pushed the feature/docker-tag-prefix branch from 117c7ea to 87df445 Compare June 3, 2020 18:05

drasko reviewed Jun 3, 2020

View reviewed changes

manuio approved these changes Jun 3, 2020

View reviewed changes

drasko approved these changes Jun 3, 2020

View reviewed changes

drasko merged commit 8b004b3 into absmach:master Jun 3, 2020

manuio pushed a commit that referenced this pull request Oct 12, 2020

NOISSUE - Add MF_DOCKER_IMAGE_NAME_PREFIX to Makefile (#1173)

b060831

Signed-off-by: Alexander Obukhov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NOISSUE - Add MF_DOCKER_IMAGE_NAME_PREFIX to Makefile #1173

NOISSUE - Add MF_DOCKER_IMAGE_NAME_PREFIX to Makefile #1173

sprql commented May 12, 2020

manuio May 12, 2020 •

edited

Loading

sprql May 12, 2020

manuio May 12, 2020

drasko Jun 1, 2020

manuio Jun 1, 2020

drasko Jun 1, 2020

sprql Jun 2, 2020

drasko Jun 2, 2020

sprql Jun 3, 2020 •

edited

Loading

drasko commented May 27, 2020

sprql commented Jun 1, 2020

drasko commented Jun 1, 2020

sprql commented Jun 1, 2020

drasko commented Jun 1, 2020

drasko Jun 1, 2020

drasko Jun 2, 2020

sprql Jun 3, 2020

drasko left a comment

manuio left a comment

manuio commented Jun 3, 2020

manuio commented Jun 3, 2020

codecov-commenter commented Jun 3, 2020

drasko Jun 3, 2020

sprql Jun 3, 2020

manuio left a comment

drasko left a comment

NOISSUE - Add MF_DOCKER_IMAGE_NAME_PREFIX to Makefile #1173

NOISSUE - Add MF_DOCKER_IMAGE_NAME_PREFIX to Makefile #1173

Conversation

sprql commented May 12, 2020

manuio May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sprql Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

drasko commented May 27, 2020

sprql commented Jun 1, 2020

drasko commented Jun 1, 2020

sprql commented Jun 1, 2020

drasko commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

manuio left a comment

Choose a reason for hiding this comment

manuio commented Jun 3, 2020

manuio commented Jun 3, 2020

codecov-commenter commented Jun 3, 2020

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

manuio May 12, 2020 •

edited

Loading

sprql Jun 3, 2020 •

edited

Loading