-
Notifications
You must be signed in to change notification settings - Fork 35
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
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
optimizely/optimizely.py
Outdated
@@ -40,6 +41,7 @@ def __init__( | |||
skip_json_validation=False, | |||
user_profile_service=None, | |||
sdk_key=None, | |||
access_token=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question on the class name. Looks good otherwise. @pawels-optimizely thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
(AuthDatafilePollingConfigManager)
that extendsPollingConfigManager
optimizely.py
to determine the type of config manager that will be used depending on input parametersTest Plan
AuthDatafilePollingConfigManager