Skip to content

feat: python rtl optional config ApiSettings init parameter #996

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

Merged
merged 6 commits into from
Feb 25, 2022
Merged

feat: python rtl optional config ApiSettings init parameter #996

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
79017c3
feat, python rtl: optional config ApiSettings init parameter
kevin-wallace-wpro Feb 18, 2022
348fa80
add SettingsConfig TypedDict, update comments
kevin-wallace-wpro Feb 22, 2022
354f78d
update read_config type to return SettingsConfig
kevin-wallace-wpro Feb 24, 2022
a004173
remove unused type
kevin-wallace-wpro Feb 24, 2022
ae33294
loop over keys, ignore mypy
kevin-wallace-wpro Feb 25, 2022
3e24d6a
Merge branch 'main' into init-via-user-provided-settings
joeldodge79 Feb 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions python/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,13 @@ Install looker_sdk using pipenv
Configuring the SDK
===================

The SDK supports configuration through a ``.ini`` file on disk as well
as `setting environment variables <https://github.com/looker-open-source/sdk-codegen#environment-variable-configuration>`_ (the latter override the former).
The SDK supports configuration through

1. an ``.ini`` file on disk
2. `setting environment variables <https://github.com/looker-open-source/sdk-codegen#environment-variable-configuration>`_
3. providing your own implementation of the ApiSettings class

. The latter override the former.

**Note**: The ``.ini`` configuration for the Looker SDK is a sample
implementation intended to speed up the initial development of python
Expand Down Expand Up @@ -132,6 +137,36 @@ example file:
For any ``.ini`` setting you can use an environment variable instead. It takes the form of
``LOOKERSDK_<UPPERCASE-SETTING-FROM-INI>`` e.g. ``LOOKERSDK_CLIENT_SECRET``

A final option is to provide your own implementation of the ApiSettings class. It is easiest to subclass ``api_settings.ApiSettings`` and override the ``read_config`` function (don't forget a call to ``super().read_config()`` if appropriate, Example below). However, at a minimum your class must implement the `api_settings.PApiSettings` protocol.


.. code-block:: python

import os
import looker_sdk
from looker_sdk import api_settings

class MyApiSettings(api_settings.ApiSettings):
def __init__(self, *args, **kw_args):
self.my_var = kw_args.pop("my_var")
super().__init__(*args, **kw_args)

def read_config(self) -> t.Dict[str, str]:
config = super().read_config()
# See api_settings.SettingsConfig for required fields
if self.my_var == "foo":
config["base_url"] = "https://foo.com"
config["client_id"] = os.getenv("FOO_CLIENT")
config["client_secret"] = os.getenv("FOO_SECRET")
else:
config["base_url"] = "https://bar.com"
config["client_id"] = os.getenv("BAR_CLIENT")
config["client_secret"] = os.getenv("BAR_SECRET")
return config

sdk = looker_sdk.init40(config_settings=MyApiSettings(my_var="foo"))
...


Code example
============
Expand Down
16 changes: 12 additions & 4 deletions python/looker_sdk/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,14 @@ def _settings(


def init31(
config_file: str = "looker.ini", section: Optional[str] = None
config_file: str = "looker.ini",
section: Optional[str] = None,
config_settings: Optional[api_settings.ApiSettings] = None,
) -> methods31.Looker31SDK:
"""Default dependency configuration"""
settings = _settings(config_file, section)
settings = (
_settings(config_file, section) if config_settings is None else config_settings
)
settings.is_configured()
transport = requests_transport.RequestsTransport.configure(settings)
return methods31.Looker31SDK(
Expand All @@ -65,10 +69,14 @@ def init31(


def init40(
config_file: str = "looker.ini", section: Optional[str] = None
config_file: str = "looker.ini",
section: Optional[str] = None,
config_settings: Optional[api_settings.ApiSettings] = None,
) -> methods40.Looker40SDK:
"""Default dependency configuration"""
settings = _settings(config_file, section)
settings = (
_settings(config_file, section) if config_settings is None else config_settings
)
settings.is_configured()
transport = requests_transport.RequestsTransport.configure(settings)
return methods40.Looker40SDK(
Expand Down
15 changes: 12 additions & 3 deletions python/looker_sdk/rtl/api_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,21 @@
from looker_sdk.rtl import transport

if sys.version_info >= (3, 8):
from typing import Protocol
from typing import Protocol, TypedDict
else:
from typing_extensions import Protocol
from typing_extensions import Protocol, TypedDict

from typing_extensions import Required
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we're using typing_extensions for Required and it's not available till 3.11(?) we need to update this and setup.py (and we should specify the minimum version of typing_extensions to use)

if sys.version_info >= (3, 8):
    from typing import Protocol
else:
    from typing_extensions import Protocol, TypedDict
if sys.version_info >= (3, 11):
    from typing import Required
else:
    from typing_extensions import Required

starting to get a little unwieldy - I've seen other libraries consolidate into a _compat.py library to do all this. But for now I think we're ok leaving it inline here.

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 tried with 3.11.0a4 but Required wasn't in there.

So I've

  1. kept the import coming from typing_extensions
  2. updated setup.py to require typing-extensions >= 4.1.1 regardless of the python version

I think that is the best way? Note we still can't use 3.11 anyway because of #944 (I pulled Required from typing_extensions running 3.11 and got the error referenced in that issue).


class SettingsConfig(TypedDict, total=False):
client_id: Required[str]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required is a relatively recent addition to typing_extensions, looks like about 3 months ago.

pyright support is there which is nice..

settings-config

Copy link
Contributor

Choose a reason for hiding this comment

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

cool! I'm approving action runs so we can see how this goes on all the python versions we support

client_secret: Required[str]
base_url: Required[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

base_url is technically not required. It's required to be an instance property on the settings object and the default implementation gets it from read_config but that's not required. I know it's confusing - honestly the only reason read_config exists is so that our default implementation doesn't have to store client_id or client_secret in python memory for security purposes. Otherwise they'd just be required instance properties too and there would be no read_config

verify_ssl: str
timeout: str

class PApiSettings(transport.PTransportSettings, Protocol):
def read_config(self) -> Dict[str, str]:
def read_config(self) -> SettingsConfig:
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 added the type here and my editor was happy with it. Adding it to read_config would probably require further code changes, probably not worth it?

Copy link
Contributor

Choose a reason for hiding this comment

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

let's see how the mypy and unittest runs go. If TypeDict.Required is available on all our supported python versions then it actually would be nice (actually probably required) to update the default implementation's type signature

...


Expand Down Expand Up @@ -94,6 +102,7 @@ def __init__(
self.agent_tag += f" {sdk_version}"

def read_config(self) -> Dict[str, str]:
# See SettingsConfig for required fields
cfg_parser = cp.ConfigParser()
try:
config_file = open(self.filename)
Expand Down