[services][blob] Add AWS SDK config
ClosedPublic
Actions

Authored by bartek on Nov 18 2022, 9:03 AM.

Details

Reviewers

varun
• jon
tomek
marcin
ashoat

Commits

rCOMMb7272eb60aec: [services][blob] Add AWS SDK config

Summary

Added a function to retrieve SDK config, required for accessing
the S3 and DynamoDB SDKs. It is unused yet, but will be needed
later.

Conditionally, if COMM_SERVICES_SANDBOX environment variable is set,
it uses localstack, which reflects the
current C++ implementation.

Depends on D5677

Test Plan

This function is tested a few diffs later, when S3 / DynamoDB are used.

Diff Detail

Repository

rCOMM Comm

Branch

barthap/blob-rust

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Nov 18 2022, 9:03 AM

bartek held this revision as a draft.

Herald added subscribers: atul, ashoat. · View Herald TranscriptNov 18 2022, 9:03 AM

Harbormaster completed remote builds in B13564: Diff 18583.Nov 18 2022, 9:17 AM

bartek added a child revision: D5679: [services][blob] Introduce S3 primitives.Nov 18 2022, 9:41 AM

bartek published this revision for review.Nov 18 2022, 10:37 AM

varun requested changes to this revision.Nov 18 2022, 12:09 PM

varun added inline comments.

services/blob/src/tools.rs
4–12	Is there an advantage to the env var approach vs using `clap` (https://docs.rs/clap/latest/clap/) to provide options? the latter just seems cleaner to me from a usability perspective. we use clap in the Identity service already

This revision now requires changes to proceed.Nov 18 2022, 12:09 PM

bartek added inline comments.Nov 21 2022, 4:10 AM

services/blob/src/tools.rs
4–12	I looked at the `clap` usage in the Identity Service and I totally agree that it is a better solution than using env vars. The reason I'd stick with the env-vars, for now, is to keep it as a drop-in replacement for the old C++ implementation. This way, minimal effort will be required to switch them later. This said, I'd leave this env-based at this point, but I'm more than happy to implement a more flexible configuration for the service later. I have created a Linear task to keep track of this: https://linear.app/comm/issue/ENG-2310/make-blob-service-configurable

varun added inline comments.Nov 21 2022, 7:43 AM

services/blob/src/tools.rs
4–12	Thanks for the explanation! Good with this approach for now. Can you please Request Review again to move this back to my queue?

bartek requested review of this revision.Nov 21 2022, 7:44 AM

bartek added inline comments.Nov 21 2022, 7:47 AM

services/blob/src/tools.rs
4–12	Done ;)

Looks ok to me but @varun should review this. Also adding @ashoat because there are new dependencies.

varun accepted this revision.Nov 22 2022, 9:40 AM

ashoat accepted this revision.Nov 22 2022, 9:43 AM

This revision is now accepted and ready to land.Nov 22 2022, 9:43 AM

Rebase

Harbormaster completed remote builds in B13720: Diff 18800.Nov 23 2022, 11:56 PM

ashoat added inline comments.Nov 24 2022, 9:36 AM

services/blob/Cargo.toml
12–15 ↗	(On Diff #18800)	In D5676, @tomek called out that these should omit the patch versions

bartek added inline comments.Nov 29 2022, 12:09 AM

services/blob/Cargo.toml
12–15 ↗	(On Diff #18800)	I'd make an exception for AWS SDK because I find its version compatibility very unstable. I struggled to find a matching `aws-config` version to `aws-sdk-*`. Even on the official site they say that the SDK is an unstable developer preview.