feat: Utility class to produce IConfigurationWire instances#241
Merged
feat: Utility class to produce IConfigurationWire instances#241
IConfigurationWire instances#241Conversation
aarsilv
approved these changes
Mar 13, 2025
Contributor
aarsilv
left a comment
aarsilv
left a comment
There was a problem hiding this comment.
I like everything but the names! Naming is hard...
| // This SDK causes the cloud endpoint below to serve the UFC test file with bandit flags. | ||
| const BANDIT_SDK_KEY = 'this-key-serves-bandits'; | ||
|
|
||
| describe('ConfigurationWireHelper', () => { |
rasendubi
reviewed
Mar 14, 2025
| */ | ||
| public static build( | ||
| sdkKey: string, | ||
| opts: SdkOptions = { sdkName: 'android', sdkVersion: '4.0.0' }, |
Collaborator
There was a problem hiding this comment.
is there any reason we're choosing these values for defaults? They seem extremely off from being default
rasendubi
reviewed
Mar 14, 2025
| * Fetches configuration data from the API and build a Bootstrap Configuration (aka an `IConfigurationWire` object). | ||
| * The IConfigurationWire instance can be used to bootstrap some SDKs. | ||
| */ | ||
| public async fetchBootstrapConfiguration(): Promise<IConfigurationWire> { |
Collaborator
There was a problem hiding this comment.
Any reason we're calling it "bootstrap configuration" vs just "configuration"?
When I head "bootstrap configuration" I imagine: sdk key and set of urls to fetch configuration from
rasendubi
reviewed
Mar 14, 2025
| /** | ||
| * Helper class for fetching and converting configuration from the Eppo API(s). | ||
| */ | ||
| export class ConfigurationWireHelper { |
Collaborator
There was a problem hiding this comment.
This looks extremely similar to configuration fetcher. Is there a significant difference? can this be just a new configuration fetcher?
rasendubi
reviewed
Mar 14, 2025
|
|
||
| if (!configResponse?.flags) { | ||
| console.warn('Unable to fetch configuration, returning empty configuration'); | ||
| return Promise.resolve(ConfigurationWireV1.empty()); |
Collaborator
There was a problem hiding this comment.
minor: given the function is async, I'm pretty sure you can just do this here:
return ConfigurationWireV1.empty();
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Towards FF-4076
Stacked on #240
Motivation and Context
The gen2 offline init (aka, bootstrap) requires an
IConfigurationWirepayload (instead of just the flag UFC json, which does not contain bandits).Users of this
bootstrapapi will need to be able to generate the payload.Description
ConfigurationWireHelperclass to fetch configs from the Eppo API endpoints and re-encode them as anIConfigurationWireinstanceHow has this been tested?