-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: do not emit error when RelayProxyEndpointsWithoutEvents is used #110
Merged
cwaldren-ld
merged 6 commits into
v7
from
cw/sc-234826/endpoint-error-with-relay-constructor
Mar 14, 2024
Merged
fix: do not emit error when RelayProxyEndpointsWithoutEvents is used #110
cwaldren-ld
merged 6 commits into
v7
from
cw/sc-234826/endpoint-error-with-relay-constructor
Mar 14, 2024
Conversation
This file contains 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
This pull request has been linked to Shortcut Story #234826: Using Relay Proxy Endpoints constructor results in error. |
cwaldren-ld
force-pushed
the
cw/sc-234826/endpoint-error-with-relay-constructor
branch
from
March 5, 2024 20:18
ad9b9c9
to
ae2bb13
Compare
cwaldren-ld
commented
Mar 11, 2024
keelerm84
approved these changes
Mar 14, 2024
Co-authored-by: Matthew M. Keeler <[email protected]>
cwaldren-ld
deleted the
cw/sc-234826/endpoint-error-with-relay-constructor
branch
March 14, 2024 17:34
cwaldren-ld
pushed a commit
that referenced
this pull request
Mar 14, 2024
🤖 I have created a release *beep* *boop* --- ## [7.1.1](v7.1.0...v7.1.1) (2024-03-14) ### Bug Fixes * do not emit error when RelayProxyEndpointsWithoutEvents is used ([#110](#110)) ([0551bf1](0551bf1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When
RelayProxyEndpointsWithoutEvents
is used to configure an SDK'sServiceEndpoints
, it throws an error-level log message explaining that only some URIs are set.This logic was intended to surface a common mistake of only setting one URI when the developer meant to set all 3. In this case the error is incorrect as we're making this choice explicitly as a supported use-case: use Relay for streaming and polling, but send events to LaunchDarkly.
There are many ways to approach fixing this in a backwards compatible manner. Ideally, we'd have constructors that embody the supported use-cases.
To avoid introducing a new construction API, I've taken a minimal approach of adding a new setter/getter on the
ServiceEndpoint
struct to indicate if partial-specification of custom URIs was intentional or not.This way, we can later check that the constructed
ServiceEndpoints
intentionally hasn't set the events endpoint and decide whether or not to log a message.