[services] Backup - rename docker to scripts
AbandonedPublic
Actions

Authored by • karol on May 4 2022, 6:18 AM.

Details

Reviewers

tomek
• jim
ashoat

Summary

Depends on D3903

When refactoring, @jimpo renamed scripts to docker. This solution is not the best as these scripts are copied twice:
https://github.com/CommE2E/comm/blob/c7fdc0b4154259b9a0f365ede281908f2479715a/services/backup/Dockerfile#L16
https://github.com/CommE2E/comm/blob/c7fdc0b4154259b9a0f365ede281908f2479715a/services/backup/Dockerfile#L17

I'm renaming the folder name to scripts again so it's copied only once.
We should use the name "scripts" inside of the docker container as from its perspective these are bash scripts that should be run.
I don't see why cannot we name this folder "scripts".

Aside from personal preferences, we should lean towards a solution that is more efficient in the end of the day.

Test Plan

No real changes are applied, just moving stuff around, so services should still build and that's the test plan

cd services

yarn run-blob-service
yarn run-backup-service

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.May 4 2022, 6:18 AM

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

• karol edited the summary of this revision. (Show Details)May 4 2022, 6:44 AM

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

• karol added a reviewer: tomek.

• karol added a parent revision: D3903: [services] Lib - Move cmake components.

• karol added a child revision: D3905: [services] Blob - rename docker to scripts.

• karol added a subscriber: • jim.

Harbormaster completed remote builds in B8761: Diff 12207.May 4 2022, 6:51 AM

• karol requested review of this revision.May 4 2022, 6:51 AM

• karol added inline comments.May 4 2022, 6:53 AM

services/backup/Dockerfile
22 ↗	(On Diff #12207)	unnecessary change

• karol planned changes to this revision.May 4 2022, 6:53 AM

update

Harbormaster completed remote builds in B8773: Diff 12220.May 4 2022, 7:43 AM

I agree the name scripts makes the most sense from inside the Docker container, but I like the name docker better for outside of the container. I think I made this point a while back when @karol-bisztyga was first introducing this code... this is the source of the folder name transferred, maybe?

I don't think it's crazy for the folder to have a different name inside vs. outside the container, and the extra line in the Dockerfile isn't a big deal in my opinion.

My main point is: for a folder that gets transferred into the Docker container, I would like a more specific name than "scripts". Maybe transferred or something. docker is fine in my book

This revision now requires changes to proceed.May 4 2022, 10:41 AM

(Adding @jimpo as blocking since this is reverting a change he made, but I doubt he feels very strongly)

ashoat mentioned this in D3905: [services] Blob - rename docker to scripts.May 4 2022, 11:07 AM

An extra line in the Dockerfile may be ok, but I don't think copying the same scripts twice is good and scales well and that's what happens here, this is the main problem and I pointed it out in the description.

This is what happens:
We copy the subfolder changing the name from docker to scripts

COPY services/backup/docker/ scripts/

We copy the entire parent folder (which includes the subfolder under the name docker)

COPY services/backup/ .

So effectively we have two folders in the docker container: scripts and docker with exactly the same contents inside.

If we wanted to change the name of the subfolder, we would have to copy every subdirectory manually or drag this subfolder outside of the parent dir or something else, there are a lot of ways of handling this. None of these ways is probably super clean and intuitive. I think having the same folder name on the host and on the docker container is the least harmful and the easiest solution.

• karol mentioned this in D3909: [services] Lib - Move Item.h.May 5 2022, 3:54 AM

Ah, interesting. So I intended for the scripts to not get copied twice by putting docker/ in the .dockerignore (see services/backup/.dockerignore). However, that is not actually working and preventing the files from getting copied because apparently the .dockerignore must be in the root of the context -- unlike gitignores, docker doesn't check every subdirectory for a .dockerignore.

This is something we should fix anyway, even if we decide to rename docker to scripts, because it's unnecessarily copying in the Dockerfile. I'll make a Linear issue for this.

Now on the topic of renaming the directory -- I agree with @ashoat that calling it scripts in the source code doesn't seem right as scripts indicates to me that you can run them on the host, like the services/scripts directory and docker is a loose convention I've seen for files that only make sense in the context of the container. That said I don't feel strongly and will approve this if you prefer scripts for whatever reason. But just to be clear, we can make sure scripts aren't copied twice (eg. with RUN mv docker/* scripts/ && rmdir docker), so this is really just an issue of naming preference.

Made issue https://linear.app/comm/issue/ENG-1089/dont-copy-dockerfile-into-service-images

Can we keep the name docker for this both inside and outside of the container? If not, can we find another name that can be consistent between inside and outside of the container, but is not generic like scripts where the reader would generally assume that they are scripts run from the host? I think we need a name that provides some clarity that the scripts inside the directory are only intended to be run inside the Docker container.

(Additionally, see my note here)

This revision now requires changes to proceed.May 5 2022, 9:13 PM

Ok so if we're going to keep the name docker and fix this some other way, then I think we can abandon this revision.