Skip to content

Conversation

@keelerm84
Copy link
Member

No description provided.

@keelerm84 keelerm84 requested a review from mike-zorn December 24, 2024 17:28

router.Handle("/all", GetProjectKeyFromAuthorizationHeader(http.HandlerFunc(StreamServerAllPayload)))

router.PathPrefix("/sdk/flags/{flagKey}").
Copy link
Member Author

Choose a reason for hiding this comment

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

The GetServerFlags method already has a branch of logic that handles the individual flag request if the flagKey variable is set. However, we never defined that route so the variable would in fact get set.

}
} else {
body = ServerAllPayloadFromFlagsState(allFlags)
body = ServerFlagsFromFlagsState(allFlags)
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 PHP, the /sdk/flags endpoint should return only the flags in an array, not the full flags + segments payload.

Choose a reason for hiding this comment

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

Oh wait a sec. Will this break non-php sdks? I think we need this to have a 3rd case where we do just flags and not all?

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 don't believe this is a problem. This function only serves two routes -- /sdk/flags/{flagKey} and /sdk/flags.

These are routes that should exclusively be used by PHP SDKs. Non-PHP SDKs don't make flag only requests such as these.

I think we need this to have a 3rd case where we do just flags and not all?

Line 25 is serves all flags. Line 20 serves a single flag (assuming the flag key parameter is set).

The /all endpoint is serviced by the StreamServerAllPayload handler.

Choose a reason for hiding this comment

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

The /all endpoint is serviced by the StreamServerAllPayload handler.

Ahhh, OK. Cool. I forgot about that.

Copy link

@mike-zorn mike-zorn left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@mike-zorn mike-zorn self-requested a review January 2, 2025 16:58
@keelerm84 keelerm84 merged commit 80d8be2 into main Jan 2, 2025
9 checks passed
@keelerm84 keelerm84 deleted the mk/sdk-1023/php branch January 2, 2025 18:37
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.

3 participants