[DRAFT] [services] Tests - Add Dockerfile
AbandonedPublic
Actions

Authored by • karol on May 31 2022, 6:49 AM.

Details

Reviewers

tomek
varun
ashoat

Summary

Depends on D4161

Adding docker file for the test service based mostly on what's in the identity service.

Test Plan

None, really. You can try to fire this up if you want but I think it's better to use the docker-compose (next diff in this stack).

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.May 31 2022, 6:49 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptMay 31 2022, 6:49 AM

Harbormaster completed remote builds in B9541: Diff 13234.May 31 2022, 6:58 AM

• karol requested review of this revision.May 31 2022, 6:58 AM

• karol edited the summary of this revision. (Show Details)May 31 2022, 7:04 AM

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, varun, ashoat.

• karol added a parent revision: D4161: [services] Tests - Initialize project.

• karol added a child revision: D4163: [DRAFT] [services] Tests - Add to docker compose.

ashoat added inline comments.May 31 2022, 7:59 AM

services/commtest/Dockerfile
7–9	Thanks, this will be helpful for me in ENG-1076

Do we really need a Docker container for these tests? Why can't we just run the build and the tests locally, without a container?

services/commtest/Dockerfile
22	Why is this necessary? Can you add a comment? See below – I might be wrong, but I think for this `rm` stuff to improve build caching it has to be in the same `RUN` as the thing it is `rm`ing
29	Does this actually work when it's in a separate command? I think this just creates a new layer... If this artifact is created as a result of `cargo build`, then I think we should `rm` it in that same `RUN` command. Alternately, if it's coming in from `COPY services/commtest .`, then I think we should add it to the `.dockerignore`
31	Why do we need to run this twice?

This revision now requires changes to proceed.May 31 2022, 1:03 PM

Do we really need a Docker container for these tests? Why can't we just run the build and the tests locally, without a container?

The summary of D4161 explains the idea:

I think we can treat the test project as another service that will never leave the dev environment. This is because we do not want devs to download dependencies and build the project on their host machines, it's going to be built inside of the docker container.

But I'm not sure if containerizing really simplifies the setup.

services/commtest/Dockerfile
1	What is the reason behind choosing this version? It looks like `1.61` is the most recent now. Also, there are alternative images, e.g. `1.61.0-slim` - do we know what is the difference and if we can use lighter ones? (doesn't matter that much)

This is because we do not want devs to download dependencies and build the project on their host machines

I don't think this is the case... see ENG-1060 and ENG-1122. With Nix and potentially vcpkg, I think we should be able to avoid all of the bad parts here and get all of the performance / debugging benefits.

I agree we can omit the docker setup. This is probably going to be abandoned then. Thanks for the review of the Dockerfile itself even though we're resigning from it.

BTW, as for any doubts - most of the code here is copied from the identity, sorry I've not mentioned this.

• karol abandoned this revision.Jun 6 2022, 4:34 AM

• karol removed a parent revision: D4161: [services] Tests - Initialize project.Jun 13 2022, 4:40 AM