Depends on D3905
Adding scripts that should be run inside of the docker container to the services "lib".
Copied scripts:
- build_server.sh
- build.sh
- run_tests.sh
Modified scripts:
- run_server.sh
- generate.sh
Details
Details
- Reviewers
tomek ashoat - Commits
- rCOMM929d775a5bad: [services] Lib - Add scripts to lib
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/lib/scripts/build.sh | ||
|---|---|---|
| 1 ↗ | (On Diff #12209) | Based on D3784 (cc @jonringer-comm) I think this should be #!/usr/bin/env bash (please apply everywhere) |
| 14 ↗ | (On Diff #12209) | I guess the way this script works for multiple places is that the working directory is different depending on who calls it? |
| 20 ↗ | (On Diff #12209) | Should this script maybe be called build_cmake.sh? |
| services/lib/scripts/build_server.sh | ||
| 23 ↗ | (On Diff #12209) | Should this script maybe be called build_service_dependencies.sh? |
| services/lib/scripts/generate.sh | ||
| 11 ↗ | (On Diff #12209) | Should this script maybe be called proto_codegen.sh? |
| services/lib/scripts/run_server.sh | ||
| 15 ↗ | (On Diff #12209) | Should this script maybe be called run_service.sh? ("server" seems a bit more general, since we also have keyserver) |
Comment Actions
Should this be merged with D3907 and D3908 in order to make it easier to review? I just wanted to make one simple change at a time but I can see you'd have to manually check the changes - I marked which files have been changed in the description.
Yeah I think it would be good if you squashed them – that way I could see what has changed in the files (if anything)
Comment Actions
I see a lot of suggestions for changing naming here. This diff is about moving stuff around, I think we should address naming separately.
| services/lib/scripts/build.sh | ||
|---|---|---|
| 1 ↗ | (On Diff #12209) | This totally sounds like stuff for a separate task https://linear.app/comm/issue/ENG-1087/change-sourcing-bash-in-scripts-for-services |
| 14 ↗ | (On Diff #12209) | Then it should only be called from the same place as build_server.sh in which the path cmake/build is recreated |
| 20 ↗ | (On Diff #12209) | I don't mind but it doesn't make sense to me honestly. We're not building cmake, we're using cmake to build the service. Just build seems to be enough. |
| services/lib/scripts/build_server.sh | ||
| 23 ↗ | (On Diff #12209) | Which script? build_server.sh or build.sh? You marked the last line of the script with a name suggestion like above but at the same time the last line is another script's name. This is confusing. To reduce cycles, I'm going to consider both cases:
And you named only one action from all of them. Besides, we don't even build dependencies here, do we?
|
| services/lib/scripts/generate.sh | ||
| 11 ↗ | (On Diff #12209) | Why exactly? What's wrong with the current name? |
| services/lib/scripts/run_server.sh | ||
| 15 ↗ | (On Diff #12209) | We can rename it |
Comment Actions
This diff is about moving stuff around, I think we should address naming separately.
Why? You're making your stack longer and longer. It makes your life harder (more diffs to rebase, more excuses not to rebase, more tasks to track) and it makes my life harder (having to track feedback across diffs, more tasks to track).
I see no advantage to making this change in a separate diff. In general, you have a pattern of "re-request review" on 80% changes I request... it causes a bunch of churn, makes feedback hard to track, causes you to have dozens of tasks, causes you to have huge stacks...
Going forward I would love if we could flip this pattern, and address 80% changes immediately in the next revision. (If there are changes that are requested that apply elsewhere in the codebase, that is a good time to create a separate task... but only for the parts outside of the diff.)
| services/lib/scripts/build.sh | ||
|---|---|---|
| 1 ↗ | (On Diff #12209) | I agree a task should be created for addressing this throughout the codebase. That said, can you please update this diff to address the feedback you were given? You can address just this part in this diff, and address the rest in the follow-up task. This is the pattern I am familiar with in my previous work and it's how the rest of the team usually works |
| 14 ↗ | (On Diff #12209) | I'm confused. I was asking you a question to confirm if my understanding is correct. You seemed to interpret it as a request for changes?
|
| 20 ↗ | (On Diff #12209) | Strong disagree, the current name is way too ambiguous. You have build.sh and build_server.sh... does build.sh not build the server? The quickest way to get an accept here would be to simply apply the naming suggestions I gave, but I'm also open to other suggestions |
| services/lib/scripts/build_server.sh | ||
| 23 ↗ | (On Diff #12209) |
Sorry, I was referring to build_server.sh. You make a good point – it's weird to call this build_service_dependencies.sh when it also builds the main binary. How about build_service.sh then? |
| services/lib/scripts/generate.sh | ||
| 11 ↗ | (On Diff #12209) | Is it not clear? generate.sh is incredibly generic. What are we generating? proto_codegen.sh is precise and tells us exactly what it's doing. |
Comment Actions
Because of the diff philosophy? A diff should do one thing at a time, right? So why would we all of a sudden handle two things(move, change naming) instead of one (move)?
You're making your stack longer and longer. It makes your life harder (more diffs to rebase, more excuses not to rebase, more tasks to track) and it makes my life harder (having to track feedback across diffs, more tasks to track).
The stack is not that long. I keep appending to keep the chronology but please, note that I'm landing a lot of diffs on the way and at one time there are not that many open ones.
I see no advantage to making this change in a separate diff.
As above, this is yet one more thing to do in a single diff. Isn't it against the diff rules?
In general, you have a pattern of "re-request review" on 80% changes I request... it causes a bunch of churn, makes feedback hard to track, causes you to have dozens of tasks, causes you to have huge stacks...
Going forward I would love if we could flip this pattern, and address 80% changes immediately in the next revision. (If there are changes that are requested that apply elsewhere in the codebase, that is a good time to create a separate task... but only for the parts outside of the diff.)
Where does this number come from? Sorry, I'm not following. When I see my mistake, I follow the review and fix it right away. But if I don't feel like the reviewer considered everything or something, I start the discussion. If you want me to blindly apply all changes from the review, then ok, but it would be weird. But just let me know if that's the case. I re-request the review to add it to your queue. I thought that's the correct workflow.