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

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

Details

Reviewers

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.

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

bartek held this revision as a draft.

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 ↗	(On Diff #18583)	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

services/blob/src/tools.rs
4–12 ↗	(On Diff #18583)	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

services/blob/src/tools.rs
4–12 ↗	(On Diff #18583)	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

services/blob/src/tools.rs
4–12 ↗	(On Diff #18583)	Done ;)

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

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

Rebase

services/blob/Cargo.toml
12–15	In D5676, @tomek called out that these should omit the patch versions

services/blob/Cargo.toml
12–15	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.