Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Introduce DataSystem configuration #259

Merged
merged 6 commits into from
Nov 14, 2024

Conversation

keelerm84
Copy link
Member

This commit introduces the concept of the data system configuration. In addition, all existing polling and streaming configurations have been updated to be expressed in terms of primary synchronizers.

As it currently stands, most of the tests are passing. There are a handful that fail which I believe are expected. Even so, this is going into a feature branch, so I'm not too concerned about the failures just yet.

All tests passing except:

  * evaluation/parameterized/prerequisites/prerequisite cycle is detected at top level, recursion stops/prerequisite cycle is detected at top level, recursion stops/evaluate flag without detail
  * evaluation/parameterized/prerequisites/prerequisite cycle is detected at top level, recursion stops/prerequisite cycle is detected at top level, recursion stops/evaluate flag with detail
  * evaluation/parameterized/prerequisites/prerequisite cycle is detected at deeper level, recursion stops/prerequisite cycle is detected at deeper level, recursion stops/evaluate flag without detail
  * evaluation/parameterized/prerequisites/prerequisite cycle is detected at deeper level, recursion stops/prerequisite cycle is detected at deeper level, recursion stops/evaluate flag with detail
  * streaming/fdv2/reconnection state management/saves previously known state
  * streaming/fdv2/reconnection state management/replaces previously known state
  * streaming/fdv2/reconnection state management/updates previously known state
  * streaming/fdv2/ignores model version
  * streaming/fdv2/disconnects on goodbye
  * polling/requests/method and headers/GET/http
  * polling/requests/URL path is computed correctly/no environment filter/base URI has no trailing slash/GET
  * polling/requests/URL path is computed correctly/no environment filter/base URI has a trailing slash/GET
  * polling/requests/URL path is computed correctly/environment_filter_key="encoding_not_necessary"/base URI has no trailing slash/GET
  * polling/requests/URL path is computed correctly/environment_filter_key="encoding_not_necessary"/base URI has a trailing slash/GET
  * polling/requests/URL path is computed correctly/environment_filter_key="encoding necessary +! %& ( )"/base URI has no trailing slash/GET
  * polling/requests/URL path is computed correctly/environment_filter_key="encoding necessary +! %& ( )"/base URI has a trailing slash/GET
  * polling/payload/large payloads
  * service endpoints/using separate service endpoints properties/streaming
@keelerm84 keelerm84 requested a review from a team as a code owner November 13, 2024 21:21
@@ -12,7 +12,6 @@ import (

func doClientSideStreamTests(t *ldtest.T) {
t.Run("requests", doClientSideStreamRequestTest)
t.Run("updates", doClientSideStreamUpdateTests)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed these update tests from the client and server tests because they don't make sense in the fdv2 work.

Originally these tests were making sure we handled individual flag / segment version values appropriately when updating the in memory store. That doesn't matter anymore (we strictly go off payload version), so we don't need this.

A subsequent commit will bring back something similar when we start interacting with the data stores. But they won't be written this like I would hope.

--

This deletion goes until the next comment.

func doServerSideStreamUpdateTests(t *ldtest.T) {
NewCommonStreamingTests(t, "doServerSideStreamUpdateTests").Updates(t)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end update cleanup.

@@ -77,7 +77,14 @@ func WithEventsConfig(eventsConfig servicedef.SDKConfigEventParams) SDKConfigure
// WithPollingConfig is used with StartSDKClient to specify a non-default polling configuration.
func WithPollingConfig(pollingConfig servicedef.SDKConfigPollingParams) SDKConfigurer {
return helpers.ConfigOptionFunc[servicedef.SDKConfigParams](func(configOut *servicedef.SDKConfigParams) error {
configOut.Polling = o.Some(pollingConfig)
dataSystem := configOut.DataSystem.OrElse(servicedef.DataSystem{})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For FDv1 support, it is assumed that there is only ever polling or streaming. So we can safely assume, for this first iteration, that these must be equivalent to the primary synchronizer of FDV2 fame.

Subsequent commits will rename these methods to be like WithPrimaryPolling or something of that nature. Letting that evolve as I refactor.

newState := config.Streaming.Value()
newState.BaseURI = d.endpoint.BaseURL()
config.Streaming = o.Some(newState)
dataSystem := config.DataSystem.OrElse(servicedef.DataSystem{})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ridiculous unpacking and re-assignment we discussed over Slack.

@keelerm84 keelerm84 requested review from cwaldren-ld and removed request for a team November 13, 2024 21:26
@@ -237,8 +237,8 @@ func (b *ServerSDKDataBuilder) BuildServerSDKData() ServerSDKData {
segments := maps.Clone(b.segments)

return map[DataItemKind]map[string]json.RawMessage{
"flags": flags,
"segments": segments,
"flag": flags,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fdv1, that key was used as the data kind that was receiving the put object. It is still used that way in fdv2, but those keys are not pluralized.

@keelerm84 keelerm84 merged commit 2f53654 into feat/fdv2 Nov 14, 2024
2 checks passed
@keelerm84 keelerm84 deleted the mk/sdk-895/add-data-system branch November 14, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants