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: Update configuration language to match new data system #267

Merged
merged 8 commits into from
Nov 19, 2024

Conversation

keelerm84
Copy link
Member

@keelerm84 keelerm84 commented Nov 19, 2024

No description provided.

@keelerm84 keelerm84 requested a review from a team as a code owner November 19, 2024 16:02
@keelerm84
Copy link
Member Author

Definitely want to review this one commit at a time.

Copy link
Contributor

@cwaldren-ld cwaldren-ld left a comment

Choose a reason for hiding this comment

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

Let's pair on this

"github.com/launchdarkly/sdk-test-harness/v2/servicedef"
)

type sdkDataSystemSourceConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might make sense to call it DataSystemConfig, and DataSystemOptionPolling etc without the "Source" part.

)...)

request := dataSource.Endpoint().RequireConnection(t, time.Second)
request := dataSystem.Synchronizers.primary.Endpoint().RequireConnection(t, time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it'd make sense to add a .PrimarySync() or .Synchronizer() helper that pulls out the primary. That way, the bulk of the existing tests that don't care about the existence of a secondary don't need to really know about it

// creating an SDK client.
//
// This behavior differs between SDK types as follows:
//
// - Server-side SDKs in streaming mode use *only* the streaming service.
// - Server-side SDKs in streaming mode use *only* the streaming synchronizing service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Think it's fair to keep this as streaming service, since synchronizer is a name to describe the general class of thing

stream.streamingService.PushUpdate("flag", "flag-key", 2, c.makeFlagData("flag-key", 2, updatedValue))
stream.streamingService.PushHeartbeat()
stream.streamingService.PushPayloadTransferred("updated", 2)
dataSystem.Synchronizers.primary.streamingService.PushHeartbeat()
Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I think having a helper would definitely make this easier to deal with

@keelerm84 keelerm84 merged commit d1a9109 into feat/fdv2 Nov 19, 2024
2 checks passed
@keelerm84 keelerm84 deleted the mk/sdk-897/language-update branch November 19, 2024 19:43
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