gouline / dbt-metabase

dbt + Metabase integration
https://pypi.org/project/dbt-metabase/
MIT License
442 stars 63 forks source link

Allow custom http adapter for certificate handling #188

Closed alexisflipo closed 7 months ago

alexisflipo commented 7 months ago

First of all, thank you for the development of this library. We use it often in the company i work for.

RATIONALE

Since we changed the authentication from Airflow to Metabase we reached some issues even with the latest version. This PR aims to add an other way to handle certificates but without needing them to be physically present (since the path is required on cert parameter). I used the requests-pkcs12 library to handle this behavior

CONSTRAINTS

SOLUTION

Thanks,

Don't hesitate to contact me if you look for further informations

gouline commented 7 months ago

CI checks also still failing, make sure they pass locally before you push.

alexisflipo commented 7 months ago

I made all the commits I needed to do. Sorry for the earlier multiple commits. I'm trying to dev quickly between some of my tasks. Is it ok if I add it as an extra with a requirements-extra.txt for the future extras that might be defined ? I handled the import in the client with a try except. Tell me if it seems coherent for you.

gouline commented 7 months ago

I prefer the custom HTTP adapter approach. Not seeing the value of the added complexity and brittle try logic for ultimately a one-liner very specific to your environment:

self.session.mount(self.base_url, Pkcs12Adapter(pkcs12_data=pkcs12_data[0], pkcs12_password=pkcs12_data[1]))

Same can be achieved by adding an optional http_adapter: HTTPAdapter to the interface and client and then:

if not http_adapter:
    http_adapter = HTTPAdapter(max_retries=Retry(total=3, backoff_factor=0.5))
self.session.mount(self.base_url, http_adapter)
alexisflipo commented 7 months ago

Yep got it. I changed it to make the http_adapter optional. I added the requests import in the inteface.py to add the right class in the type hint.