-
Notifications
You must be signed in to change notification settings - Fork 5
feat: adds boilerplate for config with factories. #1
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
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b4f9dd6
feat: adds boilerplate for config with factories.
tdstein f0237af
fix: corrects environment variable name.
tdstein 72494f0
ci: adds action to test on push.
tdstein f271e3f
Merge pull request #2 from posit-dev/tdstein/gh-actions
tdstein 219d4ba
build: adds linting and static type checking
tdstein File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[report] | ||
fail_under = 100 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
from requests import PreparedRequest | ||
from requests.auth import AuthBase | ||
|
||
|
||
class Auth(AuthBase): | ||
def __init__(self, key) -> None: | ||
self.key = key | ||
|
||
def __call__(self, r: PreparedRequest) -> PreparedRequest: | ||
r.headers["Authorization"] = f"Key {self.key}" | ||
return r |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
from unittest.mock import Mock | ||
|
||
from .auth import Auth | ||
|
||
|
||
class TestAuth: | ||
def test_auth_headers(self): | ||
key = "foobar" | ||
auth = Auth(key=key) | ||
r = Mock() | ||
r.headers = {} | ||
auth(r) | ||
assert r.headers == {"Authorization": f"Key {key}"} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
from requests import Session | ||
from typing import Optional | ||
|
||
from .auth import Auth | ||
from .config import ConfigBuilder | ||
|
||
|
||
class Client: | ||
def __init__( | ||
self, endpoint: Optional[str] = None, api_key: Optional[str] = None | ||
) -> None: | ||
builder = ConfigBuilder() | ||
builder.set_api_key(api_key) | ||
builder.set_endpoint(endpoint) | ||
self._config = builder.build() | ||
self._session = Session() | ||
self._session.auth = Auth(self._config.api_key) | ||
|
||
def get(self, endpoint: str, *args, **kwargs): # pragma: no cover | ||
return self._session.request( | ||
"GET", f"{self._config.endpoint}/{endpoint}", *args, **kwargs | ||
) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
from unittest.mock import MagicMock, Mock, patch | ||
|
||
from .client import Client | ||
|
||
|
||
class TestClient: | ||
@patch("posit.client.Session") | ||
@patch("posit.client.ConfigBuilder") | ||
@patch("posit.client.Auth") | ||
def test_init(self, Auth: MagicMock, ConfigBuilder: MagicMock, Session: MagicMock): | ||
api_key = "foobar" | ||
endpoint = "http://foo.bar" | ||
config = Mock() | ||
config.api_key = api_key | ||
builder = ConfigBuilder.return_value | ||
builder.set_api_key = Mock() | ||
builder.set_endpoint = Mock() | ||
builder.build = Mock(return_value=config) | ||
client = Client(api_key=api_key, endpoint=endpoint) | ||
ConfigBuilder.assert_called_once() | ||
builder.set_api_key.assert_called_once_with(api_key) | ||
builder.set_endpoint.assert_called_once_with(endpoint) | ||
builder.build.assert_called_once() | ||
Session.assert_called_once() | ||
Auth.assert_called_once_with(api_key) | ||
assert client._config == config |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import os | ||
|
||
from abc import ABC, abstractmethod | ||
from dataclasses import dataclass | ||
from typing import List, Optional | ||
|
||
|
||
@dataclass | ||
class Config: | ||
api_key: Optional[str] = None | ||
endpoint: Optional[str] = None | ||
|
||
|
||
class ConfigProvider(ABC): | ||
@abstractmethod | ||
def get_value(self, key: str) -> Optional[str]: | ||
raise NotImplementedError # pragma: no cover | ||
|
||
|
||
class EnvironmentConfigProvider(ConfigProvider): | ||
def get_value(self, key: str) -> Optional[str]: | ||
if key == "api_key": | ||
return os.environ.get("CONNECT_API_KEY") | ||
|
||
if key == "endpoint": | ||
return os.environ.get("CONNECT_ENDPOINT") | ||
tdstein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return None | ||
|
||
|
||
class ConfigBuilder: | ||
def __init__( | ||
self, providers: List[ConfigProvider] = [EnvironmentConfigProvider()] | ||
) -> None: | ||
self._config = Config() | ||
self._providers = providers | ||
|
||
def build(self) -> Config: | ||
for key in Config.__annotations__: | ||
if not getattr(self._config, key): | ||
setattr( | ||
self._config, | ||
key, | ||
next( | ||
(provider.get_value(key) for provider in self._providers), None | ||
), | ||
) | ||
return self._config | ||
|
||
def set_api_key(self, api_key: Optional[str]): | ||
self._config.api_key = api_key | ||
|
||
def set_endpoint(self, endpoint: Optional[str]): | ||
self._config.endpoint = endpoint |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
from unittest.mock import Mock, patch | ||
|
||
from .config import Config, ConfigBuilder, EnvironmentConfigProvider | ||
|
||
|
||
class TestEnvironmentConfigProvider: | ||
@patch.dict("os.environ", {"CONNECT_API_KEY": "foobar"}) | ||
def test_get_api_key(self): | ||
provider = EnvironmentConfigProvider() | ||
api_key = provider.get_value("api_key") | ||
assert api_key == "foobar" | ||
|
||
@patch.dict("os.environ", {"CONNECT_ENDPOINT": "http://foo.bar"}) | ||
def test_get_endpoint(self): | ||
provider = EnvironmentConfigProvider() | ||
endpoint = provider.get_value("endpoint") | ||
assert endpoint == "http://foo.bar" | ||
|
||
def test_get_value_miss(self): | ||
provider = EnvironmentConfigProvider() | ||
value = provider.get_value("foobar") | ||
assert value == None | ||
|
||
|
||
class TestConfigBuilder: | ||
def test_build(self): | ||
builder = ConfigBuilder() | ||
assert builder._config == Config() | ||
|
||
def test_build_with_provider(self): | ||
provider = Mock() | ||
provider.get_value = Mock() | ||
builder = ConfigBuilder([provider]) | ||
builder.build() | ||
for key in Config.__annotations__: | ||
provider.get_value.assert_any_call(key) | ||
|
||
def test_set_api_key(self): | ||
builder = ConfigBuilder() | ||
builder.set_api_key("foobar") | ||
assert builder._config.api_key == "foobar" | ||
|
||
def test_set_endpoint(self): | ||
builder = ConfigBuilder() | ||
builder.set_endpoint("http://foo.bar") | ||
assert builder._config.endpoint == "http://foo.bar" |
This file was deleted.
Oops, something went wrong.
Empty file.
This file was deleted.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Will sync vs async be a concern? I know
httpx
supports both but I don't know how important that might be for users of this clientThere 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.
I'm not sure. I started with requests since it is the most popular HTTP library after urllib3. And the simplest to use, in my opinion.
Are there situations when calling the Connect API where asynchronous design becomes necessary?
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.
pyshiny (and other fastapi-base apps) rely heavily on python's asyncio apis. This has come up in the databricks sql client as well. There are a few justifications for the request for async in this issue that might also be applicable for our client.
databricks/databricks-sql-python#176
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.
Thanks!
httpx
looks great, but it's marked as beta. Do you have opinions onaiohttp
? That seems like the most stable library to bring in.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.
I haven't used it but that sounds like a good option to me. FWIW Joe also recommended
aiohttp
when we were asking him some questions about async shiny: https://positpbc.slack.com/archives/C03HH2ZUU/p1702668760303599There 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.
I'm skeptical that we'll want to bother with async queries against the Connect API
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.
If we expect the sdk to be used from inside Connect-hosted content like fastapi then a non-blocking api could be important. Based off of Joe's comment from the slack thread, it sounds like request's blocking IO is a problem for uvicorn apps.
We should test this to confirm the behavior but if all other requests are blocked while we're waiting for a request to the Connect API to resolve then that could be a blocker for obtaining an access token inside a piece of content.
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.
Could someone write a
uvicorn
application that async wraps calls to this library?I also wouldn't want to force a consumer to set up
asyncio
and intoawait
'ing everything.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.
Sure, probably. Let's table this for now. I think this is something that warrants more discussion though. We should test what it feels like to use this SDK from different content types in Connect. Especially single-threaded uvicorn content like pyshiny and fastapi. I'm interested to see whether the content can service multiple requests at the same time without using an async requests library
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.
I agree, let's defer, this feels like YAGNI in PR #1. If we find out that we are bottlenecking because of this, we should totally revisit.