Vector: Run `make generate` inside Docker image

[binarylogic]

This ensures that the environment is constant and working.

[binarylogic]

@a-rodin do you think it makes sense to use docker-run.sh directly in the Makefile? This would ensure commands like make generate work properly for new contributors. The issue with this is recursion. Once you're within the Docker context it's also nice to run make .... As I see it, there are a few ways we could resolve this:

1. Maintain two separate Makefiles (Makefile and MakefileDocker). Where Makefile is designed to be used by an operator and MakefileDocker is designed to be used within the context of a Dockerfile.
2. Have docker-run.sh detect if it's in the context of a Docker file and simplify execute the passed command if so.
3. Couple scripts with Docker images, treating Dockerfiles as the scripts. For example, script/generate.rb could be moved into a generate/Dockerfile.
4. Make the use of Docker optional, as it is now, and inform users that they can run commands with scripts/docker-run.sh checker make generate.

Regarding options 1, 2, and 3, it seems abnormal for a make target to leverage Docker, but it simplifies make target execution for human operators. It also makes it difficult to run make targets in environments where the Docker image must be specified separately (CircleCI).

What do you think?

[a-rodin]

I would lean towards not making it a requirement to use Docker to run the generation, as some people might not want to do it, for example because of security concerns, as Docker requires root access.

Initially I had in mind option 4, which is to recommend using scripts/docker-run.sh. However, I think there is also a more ergonomic way to accomplish the same goal, which would not require calls to Docker in Makefile targets and not require to run a separate shell script. This option is to use docker-compose.

We can have a docker-compose.yml file with some defined services, each of which would run different kind of generation or check. Then make generate can be executed like this:

docker-compose run generate

or make check-fmt can be executed like this:

docker-compose run check-fmt

Even building Vector can be made possible to be executed by running

docker-compose run build

I think this approach is almost as ergonomic as plain usage of make <target>, and it also looks more transparent in its intent to run something inside Docker than a shell script.

I seems to me that the only caveat about this approach is the fact that we already have docker-compose.yml file, which currently has different purpose. I think we can either have two docker-compose.yml files, for example renaming the current one to docker-compose.test.yml and naming the new one plain docker-compose.yml, or merge them.

I'm not sure which one of these approaches is better. The approach with having just a single docker-compose.yml file is a bit messier (as the file can grow really large), but it would make it possible to also run the integration tests entirely inside Docker, thus not requiring new contributors to have Rust compiler installed on the host machine at all. As a consequence, it might also be possible to implement https://github.com/timberio/vector/issues/1616 and https://github.com/timberio/vector/issues/1615 as part of these integration tests running inside Docker. On the other hand, with the approach using separate docker-compose.yml files it might be easier to reason about these files.

Furthermore, as something related, we can eventually implement most of the jobs we currently run in CI on Linux in the same way, and replace calls to them in .circleci/config.yml to executions of the corresponding docker-compose run <...> commands. It would make it easier to run any failing CI check locally.

[binarylogic]

I like this proposal. What if we moved the current docker-compose.yml to tests/docker-compose.yml since it is test specific?

[LucioFranco]

@a-rodin I think it may make sense to put these behind make generate-docker and then place the actual docker-compose.yml file in the scripts directory because then make can just do docker-compose -f scripts/docker-compose.yml run <cmd>. The reason I kept the docker-compose file for tests in the main directory was to make it super to just run

docker-compose start 
cargo test --features docker

which is generally my preferred workflow since I can spin up just the containers I need without a super long command.

Also at this point installing ruby isn't that bad maybe it makes sense to not do this all with docker?

[binarylogic]

Also at this point installing ruby isn't that bad maybe it makes sense to not do this all with docker?

I disagree, it's very obvious that new contributors have trouble with this. This seems like the perfect use case for Docker.

@a-rodin I think it may make sense to put these behind make generate-docker

I'm not really a fan of that, it feels messy and somewhat confusing. I would like one authoritative command to generate documentation, if possible.

---

If we're going to use docker-compose as a Docker driven Makefile then it makes sense to have it at the root of the repository with the Makefile. I vote that we move the current docker-compose.yml file to the test directory or maintain one file.

[binarylogic]

Also, our Makefile currently interacts with Docker via make test, so I don't think the solution in https://github.com/timberio/vector/pull/1623 is that much different. Why don't we try that first and then we can adjust as necessary?

[a-rodin]

Also, our Makefile currently interacts with Docker via make test, so I don't think the solution in #1623 is that much different. Why don't we try that first and then we can adjust as necessary?

This sounds good to me. One caveat: I think we need to ensure that --user=$UID is added to and --privileged flag is removed from the arguments of docker run command. One way to achieve this is to modify ./scripts/docker-run.sh script. Another way is to write complete invocation of docker run ... directly to Makefile with --user=$UID option. Without --user=$UID there would be issue https://github.com/timberio/vector/issues/1621.

[binarylogic]

Accidentally closed this. Will close it in a follow-up PR.