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

Selki/node sdk relay features/bandits handling #104

Merged
merged 32 commits into from
Jan 31, 2025

Conversation

maya-the-mango
Copy link
Contributor

No description provided.

Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

💪 I didn't have time to dive in but looking good at a quick glance!

Copy link
Member

@leoromanovsky leoromanovsky left a comment

Choose a reason for hiding this comment

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

Quick comment - this needs to be integrated into github actions:

https://github.com/Eppo-exp/sdk-test-data/blob/main/.github/workflows/test-sdk-packages.yml#L10

On its own it does not produce any meaningful testing. After integrating the github actions will compile the code from source, start the server and collect test results. Are you wanting to add that in this PR or a stacked one? I prefer this PR as it would make reviewing the code in it easier since we can see whether it passes.

@maya-the-mango
Copy link
Contributor Author

Quick comment - this needs to be integrated into github actions:

https://github.com/Eppo-exp/sdk-test-data/blob/main/.github/workflows/test-sdk-packages.yml#L10

On its own it does not produce any meaningful testing. After integrating the github actions will compile the code from source, start the server and collect test results. Are you wanting to add that in this PR or a stacked one? I prefer this PR as it would make reviewing the code in it easier since we can see whether it passes.

@leoromanovsky I wanted to do a stacked one to avoid bigger PR but now that you mentioned that It does makes sense to keep it in one PR. I will tag you once more once I add everything regarding workflows.

aarsilv and others added 2 commits January 17, 2025 11:24
* test case for special characters

* update obfuscated config

* update names in description

* include unevaluated allocation

* update cyrillic language example

* fix reason
Copy link
Collaborator

@typotter typotter left a comment

Choose a reason for hiding this comment

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

This is awesome! love seeing this come together and especially love seeing the tests!
Thanks for your patience.

There's some work to be done with the client proxy - I think we can get by without any extra type casting

@@ -0,0 +1,99 @@
<p align="center">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any notes or commands you can write here for the next person to work on this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this file a bit. It will grow more once I finish build-and-run script

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -1186,6 +1186,104 @@
}
],
"totalShards": 10000
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

merge artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why it shows in diff, but that's something that is already in main branch. It probably was added when I rebased main onto feature branch

@@ -0,0 +1,143 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

merge artifact?

maya-the-mango and others added 6 commits January 20, 2025 11:04
* inital app from create-expo-app
* add Eppo SDK; traditional and precomputed client
* Eppo Client provider and basic flag/bandit display
Copy link
Collaborator

@typotter typotter left a comment

Choose a reason for hiding this comment

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

Not really sure why the react relay is showing up in your PR. some weird merge/rebase artifact.
Other than that, looks great! I've captured that we need to add dynamic SDK versioning to the build-and-run

@@ -0,0 +1,99 @@
<p align="center">
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,12 @@
#!/usr/bin/env bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

can merge and as and we'll iterate on the script.

@@ -0,0 +1,8 @@
// https://docs.expo.dev/guides/using-eslint/
Copy link
Collaborator

Choose a reason for hiding this comment

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

weird merge artifact? the react-native-sdk-relay is already in the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tried merging main into feature branch, but the artifacts didnt go away.

@@ -33,7 +33,7 @@ jobs:
test_data_branch: ${{ github.head_ref || github.ref_name }}
sdk_branch: main

test-node-client-sdk:
test-js-client-sdk:
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks!

@maya-the-mango maya-the-mango merged commit fb54768 into main Jan 31, 2025
@maya-the-mango maya-the-mango deleted the selki/node-sdk-relay-features/bandits-handling branch January 31, 2025 11:03
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.

4 participants