Skip to content

feat(datafile-management): Integrate config_manager with Optimizely #179

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

Merged
merged 16 commits into from
Jun 12, 2019

Conversation

aliabbasrizvi
Copy link
Contributor

@aliabbasrizvi aliabbasrizvi commented Jun 5, 2019

Summary

In this change, we move to using config_manager.PollingConfigManager and config_manager.StaticConfigManager based on how the SDK is initialized. With the change the SDK may be initialized by either:

  • Providing sdk_key only: in this case, the PollingConfigManager will be used which will request and update config at default interval of 5 minutes.
  • Providing datafile only: in this case, the StaticConfigManager will be used and the SDK will continue to behave like it has done previously i.e. it will need to be re-initialized.
  • Providing both options: in this case, the PollingConfigManager will be used and the SDK will start with a config based on the provided datafile and will update config at default interval of 5 minutes.

To use more options on the PollingConfigManager like different update_interval, url, url_template, users will have the option to create an instance of PollingConfigManager and pass it to Optimizely as config_manager option.

Test plan

  • Manual testing
  • Unit tests

@aliabbasrizvi aliabbasrizvi added the wip Work in progress label Jun 5, 2019
@aliabbasrizvi
Copy link
Contributor Author

Not ready for review yet.

@aliabbasrizvi aliabbasrizvi removed the wip Work in progress label Jun 10, 2019
@aliabbasrizvi aliabbasrizvi requested a review from travisbeck June 10, 2019 22:53
@coveralls
Copy link

coveralls commented Jun 10, 2019

Coverage Status

Coverage decreased (-0.4%) to 98.949% when pulling ebb0ae3 on ali/integrate_with_optly into e078a48 on datafile-manager.

@aliabbasrizvi aliabbasrizvi changed the title feat(datafile-management): Integrate config_manager into Optimizely feat(datafile-management): Integrate config_manager with Optimizely Jun 11, 2019
self.logger.exception(error_msg)
self.error_handler.handle_error(error_to_handle)
return
if not self.config_manager:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to combine this with the sdk_key check? There doesn't seem to be additional logic outside of the inner if clause

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

lgtm

@aliabbasrizvi aliabbasrizvi force-pushed the ali/integrate_with_optly branch from fbea190 to 1e56ddd Compare June 12, 2019 06:11
@aliabbasrizvi aliabbasrizvi requested a review from mjc1283 June 12, 2019 06:31
@aliabbasrizvi aliabbasrizvi merged commit 08f8780 into datafile-manager Jun 12, 2019
@aliabbasrizvi aliabbasrizvi deleted the ali/integrate_with_optly branch June 12, 2019 20:55
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