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.

Sign up for GitHub

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

Jump to bottom

feat: add odp event flush interval #414

Merged
merged 15 commits into from
Feb 7, 2023
Merged

feat: add odp event flush interval #414

merged 15 commits into from
Feb 7, 2023

Conversation

Mat001
Copy link
Contributor

@Mat001 Mat001 commented Jan 31, 2023

Summary

  • Add odp flush interval argument to optimizely client to expose it for testing purposes with FSC test suite
  • only for internal use, will not be documented for public use

Test plan

  • unit tests for odp flush interval functionality in odp event mangaer
  • unit tests to verify setting flush interval to zero is accepted at the client level

Jira story

@Mat001 Mat001 requested a review from a team as a code owner January 31, 2023 23:21
@Mat001
tests fix
9fbc22d
@Mat001
Merge branch 'mpirnovar/add_odp_event_flush_interval' of github.com:o…
da8cea8 
…ptimizely/python-sdk into mpirnovar/add_odp_event_flush_interval
jaeopt
jaeopt approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM - nits
Also I see a new empty file added at the end "myapp2.py". intentional?

optimizely/helpers/sdk_settings.py Outdated Show resolved Hide resolved
optimizely/odp/odp_event_manager.py Outdated Show resolved Hide resolved
andrewleap-optimizely
andrewleap-optimizely requested changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@andrewleap-optimizely andrewleap-optimizely left a comment

Choose a reason for hiding this comment

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

Looking great so far, just a couple of suggestions. Let's chat today about the send_event tests.

tests/test_odp_event_manager.py Show resolved Hide resolved
tests/test_odp_event_manager.py Outdated Show resolved Hide resolved
tests/test_optimizely.py Outdated Show resolved Hide resolved
tests/test_optimizely.py Outdated Show resolved Hide resolved
optimizely/odp/odp_event_manager.py Outdated Show resolved Hide resolved
optimizely/helpers/sdk_settings.py Outdated Show resolved Hide resolved
optimizely/odp/odp_event_manager.py Outdated Show resolved Hide resolved
@andrewleap-optimizely andrewleap-optimizely changed the title Mpirnovar/add odp event flush interval feat: add odp event flush interval Feb 1, 2023
andrewleap-optimizely
andrewleap-optimizely approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@andrewleap-optimizely andrewleap-optimizely left a comment

Choose a reason for hiding this comment

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

lgtm. just a small documentation suggestion

optimizely/helpers/sdk_settings.py Outdated Show resolved Hide resolved
@Mat001 Mat001 merged commit e13482f into master Feb 7, 2023
@Mat001 Mat001 deleted the mpirnovar/add_odp_event_flush_interval branch February 7, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaeopt jaeopt jaeopt approved these changes

@andrewleap-optimizely andrewleap-optimizely andrewleap-optimizely approved these changes

Assignees

@Mat001 Mat001

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mat001 @jaeopt @andrewleap-optimizely