feat: introduces API v14 to fetch new cells feature config - WPB-22364 #4052
Conversation
Test Results 5 files 757 suites 7m 25s ⏱️ Results for commit 3f877eb. ♻️ This comment has been updated with latest results. |
jullianm
requested review from
a team,
KaterinaWire and
samwyndham
and removed request for
a team
samwyndham
left a comment
samwyndham
left a comment
There was a problem hiding this comment.
Nice work. I've added a comment for a small change to do in the AWS client. Feel free to discuss.
| return signed.absoluteString | ||
| } | ||
|
|
||
| private func makeS3Client() async throws -> any S3ClientProtocol { |
There was a problem hiding this comment.
suggestion / issue: I'm not sure it's safe to have multiple S3 clients. Luckily I think the AWS SDK already provides a way to lazily provide the endpoint. See that the config can optionally take an EndpointResolver instead of an endpoint. We are doing something similar to lazily provide the access token using CredentialIdentityResolver. If you haven't already done so, please give that a try instead of creating a new S3 client each time
|
|
||
| public extension Feature { | ||
|
|
||
| struct CellsInternal: Codable { |
There was a problem hiding this comment.
question: What does internal mean here ?
| public struct Config: Codable { | ||
| public let backend: Backend | ||
|
|
||
| public init(backend: Backend = .init(url: nil)) { |
There was a problem hiding this comment.
Question: Why is the default needed here?
Issue
This PR introduces API v14 which is required to fetch wire cells backend URL from a newly introduced feature config.
A key change in this PR is to resolve the cells server URL dynamically since we now get its value after pulling all the resources during initial sync.
To avoid dealing with optionals in many parts of the codebase and keep the structure as close as it is currently, the solution was to inject a URL resolver closure in the factory which will be invoked at runtime by the
RestAPIand theAWSClient.Note: at the time of this PR, API v14 is still under development
Testing
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: