Create a new lib where all the common database code for Rust services can be placed.
Depends on D6853
Details
Details
Compile and run blob and backup services. Still have to check how it affects Docker builds.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
This seems like it would've been a great time to introduce a shared library instead of copy-pasting. I understand there is a monthly goal at play here, but how long does it really take to set up a shared library? In JavaScript it can be done in ~15min, but I'm not as familiar with Rust
Comment Actions
I'm postponing it for now as creating a shared Rust lib and figuring out the common interface might take some time.
Additionally,
but how long does it really take to set up a shared library?
Is a good question for which I don't have an answer yet. I don't think that creating a new lib for js and extracting the common code to it is only 15 mins, but even if, then it should be faster than doing so for a language that is compiled.
It seems like I should make this refactoring a high priority one, as it was asked multiple times, so I'll focus on it now.
| services/blob/src/database.rs | ||
|---|---|---|
| 298–305 ↗ | (On Diff #23138) | Haven't extracted it, as it contains service-specific code. We need to figure out how to handle that. |
Comment Actions
- CI is failing because blob and backup (and feature_flag) Dockerfiles need rust_lib to be copied.
- Identity service also uses these functions.
| services/rust-lib/Cargo.toml | ||
|---|---|---|
| 2 ↗ | (On Diff #23138) | I don't think this name is accurate. It's meaningless.
We could also follow the native_rust_library pattern and name it services_rust_library but I personally don't like them either - I consider them too vague. |
Comment Actions
- Identity service also uses these functions.
Thanks for letting me know! I've quickly tried to use the new lib for identity, but it can't be built due to an error
error: failed to select a version for `xmlparser`.
... required by package `aws-smithy-xml v0.51.0`
... which satisfies dependency `aws-smithy-xml = "^0.51.0"` of package `aws-sdk-sts v0.21.0`
... which satisfies dependency `aws-sdk-sts = "^0.21.0"` of package `aws-config v0.51.0`
... which satisfies dependency `aws-config = "^0.51.0"` of package `comm-services-lib v0.1.0 (/Users/tomaszpalys/IdeaProjects/comm/services/comm-services-lib)`
... which satisfies path dependency `comm-services-lib` of package `identity v0.1.0 (/Users/tomaszpalys/IdeaProjects/comm/services/identity)`
versions that meet the requirements `^0.13.5` are: 0.13.5
all possible versions conflict with previously selected packages.
previously selected package `xmlparser v0.13.3`
... which satisfies dependency `xmlparser = "=0.13.3"` of package `aws-smithy-xml v0.45.0`
... which satisfies dependency `aws-smithy-xml = "^0.45.0"` of package `aws-sdk-sts v0.15.0`
... which satisfies dependency `aws-sdk-sts = "^0.15.0"` of package `aws-config v0.15.0`
... which satisfies dependency `aws-config = "^0.15.0"` of package `identity v0.1.0 (/Users/tomaszpalys/IdeaProjects/comm/services/identity)`
failed to select a version for `xmlparser` which could resolve this conflictwhich probably can be fixed by bumping old versions of libs, but I don't think it is a good idea to further increase the scope of this diff. I've created a task https://linear.app/comm/issue/ENG-3102/rust-services-lib-extract-database-utils-from-identity-service to track it.
Comment Actions
[...] which probably can be fixed by bumping old versions of libs, but I don't think it is a good idea to further increase the scope of this diff.
yeah makes sense
| services/blob/Cargo.toml | ||
|---|---|---|
| 18 ↗ | (On Diff #23157) | It's not a mistake - blob still uses this lib |
| services/blob/Cargo.toml | ||
|---|---|---|
| 18 ↗ | (On Diff #23157) | Ah right, I missed that |