Hi @jeffmendoza. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Are there going to be more dashboard files added in the future? Currently I'm hesitant to think 3 files warrants a directory copy. The alternative I'm considering is along the lines of:

Dockerfile.grafana:

- COPY dashboards /var/lib/grafana/dashboards/
+ # possibly need to do: RUN mkdir -p /var/lib/grafana/dashboards/
+ COPY istio-dashboard.json /var/lib/grafana/dashboards/
+ COPY mixer-dashboard.json /var/lib/grafana/dashboards/
+ COPY pilot-dashboard.json /var/lib/grafana/dashboards/

in tools/istio-docker.mk add these to GRAFANA_FILES:

addons/grafana/dashboards/istio-dashboard.json
addons/grafana/dashboards/mixer-dashboard.json
addons/grafana/dashboards/pilot-dashboard.json

an optional simplification is to remote the $(GRAFANA_FILES) dependency:

- docker.grafana: addons/grafana/Dockerfile$$(suffix $$@) $(GRAFANA_FILES)
+ docker.grafana: addons/grafana/Dockerfile$$(suffix $$@)

as the dependency is already added/handled by the preceeding "$(foreach FILE,$(GRAFANA_FILES) " line.

/ok-to-test

@jeffmendoza would you mind posting some screenshots of the updates to make it slightly easier to appreciate what the update looks like?

@mattdelco What is the cost of adding a directory copy? There will be more dashboards, and I'd like it to be easier for them to be added. New contributors could be intimidated by the Makefile system.

The cost of the directory copy is mostly about loss of consistency and increased special casing in the Makefile. If someone also later adds other files (e.g., a readme.md for devs) then those might inadvertently get added to the docker image. With individual files someone would have to update the Makefile but that'd only be to add a file to GRAFANA_FILES.

The runtime cost is harder to quantify (e.g., 1 call to "cp -r" vs 3 calls to "cp"). Citing individual files will probably copy less during an incremental rebuild, but the build time will probably be dominated by "docker build" copying the entire $(ISTIO_DOCKER) directory to a temp location to perform a hermetic build. In any case I doubt anyone would notice the perf difference for such a small file count, so I was mostly asking to help gauge if the inconsistency was a worthwhile tradeoff.

@jeffmendoza PR needs rebase

@mattdelco
I'd like a new developer to be able to run docker build -t my-grafana -f Dockerfile.grafana . inside that directory. Using GRAFANA_FILES and the changes you mentioned in the Dockerfile would make that not work, unless the dashboard.json files were moved up a dir. I think that would get a bit messy.

Generally it's been fragile for there to be multiple ways to build things (it's not uncommon for one way to break in ways that the others don't), so I'd actually find it appealing to use an approach that dissuades doing stuff outside of make.

I understand that using native go commands to build components is a goal of the project. I believe docker build is similar. There is not a Makefile in each directory to just simply type make. A developer unfamiliar with the project and our particular make target naming scheme would simply try to build the docker image as I described.

conclusion ?

@mattdelco Let me know what you want. I'd like to leave it as-is, to allow easy dashboard adding without editing Makefiles. If the directory copy is not ok, I'll move the dashboards up a dir, and make it a flat directory structure.

I can live with things either way--just don't create a doc that suggests that people run "docker build" directly. The last I'd heard was that go devs could run "go build" to satisfy the particular needs of themselves, tools, or IDEs, but anything else should be handled via make.

For a fun different perspective, we do use Dockerfile to build in some places, for instance https://github.com/istio/fortio/blob/master/Dockerfile
is what docker swarm auto builds and auto pushes to docker for git tagged releases
and even run tests etc
https://github.com/istio/fortio/blob/master/docker-compose.test.yml
but that setup predates istio's switch from bazel and fortio does have Makefile too to drive most steps and fortio's build is a lot simpler/straightforward than istio's (and I guess inside istio/istio we probably should try to be consistent)

but shall we / l g t m as is or not ?

Multi-stage builds for Docker images are really nice.

just don't create a doc that suggests that people run "docker build" directly

Sgtm

Multi-stage builds for Docker images are really nice.

thanks ! there is even a third stage as the binary is later extracted in realease/ to make the linux binary tgz release

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly

The full list of commands accepted by this bot can be found here.

/test all [submit-queue is verifying that this PR is safe to merge]

Automatic merge from submit-queue.

@jeffmendoza: The following test failed, say /retest to rerun them all: