feat: Added support for Authenticated Datafiles#271
Conversation
…nd create constructor
… add access token to authorization header
…ditional for it based on if access token is provided
…figManager will be used
pawels-optimizely
left a comment
There was a problem hiding this comment.
LGTM, although I still think the changes are negligible and it would be easier to do it in existing PollingConfigManager by adding access_token as an optional parameter. Also, it would be easier to make a change in the code if users decided to instantiate PollingConfigManager themselves and switched to private datafiles instead.
|
Please wait for @aliabbasrizvi to take a look as well :) |
I think the choice will be very explicit here. |
| skip_json_validation=False, | ||
| user_profile_service=None, | ||
| sdk_key=None, | ||
| access_token=None, |
There was a problem hiding this comment.
This will break the SDK, since a user can choose not to use named arguments, but rather pass them in as positional arguments. This should be the last argument in the method.
Also, I am not sure if access_token needs to be introduced here. It is a concept in the config manager and one can simple invoke the AuthDatafilePollingConfigManager and pass that in. So this is probably not useful.
There was a problem hiding this comment.
Agree that constructing AuthDatafilePollingConfigManager then passing it here as config_manager is a good way to go. I think this is how you customize other aspects of datafile fetching like URL and polling interval. The token isn't relevant to the Optimizely class.
There was a problem hiding this comment.
good catch on the order of arguments, wondering how it passed the unit tests.
IMO, adding access_token here as the last optional argument does not hurt and even makes sense. Each config manager (Polling and Static) with default parameters is referenced in here, so not sure why AuthDatafilePollingConfigManager has to be the exception. If access_token was set on PollingConfigManager then this could be considered as an option just like poling interval, but AuthDatafilePollingConfigManager is a separate entity.
There was a problem hiding this comment.
Done. Though, I think that access_token should still be included in the constructor in order to use AuthDatafilePollingConfigManager in the case where someone chooses to create an optimizely instance.
aliabbasrizvi
left a comment
There was a problem hiding this comment.
Just a question on the class name. Looks good otherwise. @pawels-optimizely thoughts?
Summary
(AuthDatafilePollingConfigManager)that extendsPollingConfigManageroptimizely.pyto determine the type of config manager that will be used depending on input parametersTest Plan
AuthDatafilePollingConfigManager