[services] Blob/Backup - Read env vars in runtime instead of compile-time flags
ClosedPublic
Actions

Authored by • karol on Apr 28 2022, 2:34 AM.

Details

Reviewers

tomek
• jim

Commits

rCOMM010fcaa64933: [services] Blob/Backup - Read env vars in runtime instead of compile-time flags

Summary

Depends on D3691

As suggested in https://phabricator.ashoat.com/D3498#98870 we want to avoid using compile-time flags and replace them with reading environment variables in the runtime.

Test Plan

cd services
yarn test-backup-service
yarn test-backup-service-dev-mode
yarn run-backup-service
yarn run-backup-service-dev-mode
yarn test-blob-service
yarn test-blob-service-dev-mode
yarn run-blob-service
yarn run-blob-service-dev-mode

They all should work as before.

Diff Detail

Repository

rCOMM Comm

Branch

dev

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Apr 28 2022, 2:34 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptApr 28 2022, 2:34 AM

• karol mentioned this in D3690: [services] Dev mode - Set up Backup service.Apr 28 2022, 2:35 AM

• karol mentioned this in D3691: [services] Dev mode - Set up Blob service.

Harbormaster completed remote builds in B8642: Diff 12033.Apr 28 2022, 2:38 AM

• karol requested review of this revision.Apr 28 2022, 2:38 AM

• karol edited the summary of this revision. (Show Details)Apr 28 2022, 2:39 AM

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

• karol added reviewers: tomek, • jim.

• karol added a parent revision: D3691: [services] Dev mode - Set up Blob service.

ashoat added inline comments.Apr 28 2022, 11:30 AM

services/backup/src/AwsTools.cpp
12–14 ↗	(On Diff #12033)	Here too
services/backup/src/Tools.cpp
29–31 ↗	(On Diff #12033)	This looks really messy... can we pull the ternary out of the conditional to make it cleaner?
services/blob/src/Tools.cpp
79–80 ↗	(On Diff #12033)	Here too

tomek requested changes to this revision.Apr 29 2022, 5:51 AM

tomek added inline comments.

services/backup/src/Tools.cpp
29–31 ↗	(On Diff #12033)	Yeah, this seems to be overly complicated. What do you think about something like this. Also, creating a function that returns a bool would be really useful.

This revision now requires changes to proceed.Apr 29 2022, 5:51 AM

• karol added inline comments.Apr 29 2022, 7:27 AM

services/backup/src/Tools.cpp
29–31 ↗	(On Diff #12033)	I think a helper function is a way to go here!

add helper function isDevMode()

Harbormaster completed remote builds in B8699: Diff 12118.Apr 29 2022, 7:53 AM

Please use the new function in decorateTableName functions before landing

services/backup/src/Tools.cpp
29–31 ↗	(On Diff #12118)
services/blob/src/Tools.cpp
78–80 ↗	(On Diff #12118)

This revision is now accepted and ready to land.Apr 29 2022, 7:59 AM

• karol added inline comments.May 4 2022, 1:42 AM

services/backup/src/Tools.cpp
29–31 ↗	(On Diff #12118)	`isDevMode` checks against `COMM_SERVICES_DEV_MODE`, here we have `COMM_TEST_SERVICES`. These are 2 different flags. We could create a similar function `isTesting` but `std::getenv("COMM_TEST_SERVICES")` is really used only in one place so there's no point I guess.
services/blob/src/Tools.cpp
78–80 ↗	(On Diff #12118)	as above

ashoat added inline comments.May 4 2022, 9:01 AM

services/backup/src/Tools.cpp
29–31 ↗	(On Diff #12118)	Please do not land this diff with this messy ternary inside a conditional. It is very difficult to read

remove ternary

Harbormaster completed remote builds in B8838: Diff 12322.May 6 2022, 1:46 AM

Closed by commit rCOMM010fcaa64933: [services] Blob/Backup - Read env vars in runtime instead of compile-time flags (authored by • karol). · Explain WhyMay 6 2022, 1:46 AM

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMM010fcaa64933: [services] Blob/Backup - Read env vars in runtime instead of compile-time flags.

tomek added inline comments.May 9 2022, 2:47 AM

services/backup/src/Tools.cpp
29–31 ↗	(On Diff #12118)	We've landed this, but one possible option here was to introduce a new function e.g. `hasEnvFlag` that would contain the `if` with two conditions and could be used here and in `isDevMode` function.