feat(gsheets2): passed secrets as param

[testinnplayin]

Change Summary

Passes secrets as a factory function to Google Sheets 2 connector.
Secrets will no longer be visible in the front-end as a potential input in the connector configuration modal.

NOTE:
I am aware that the 'bag' that contains kwargs is checked in a stupid way but the signature object of the inspect module can't be used in 3.6 like in 3.7 and 3.8 probably due to the fact that we have an abstract function in the way (_retrieve_data inside the ToucanConnector). This is a workaround up until the time we drop 3.6 support. It's to prevent secrets being passed to connectors that don't use it and thus provoking an error.

NOTE:
base_route should probably be a class property but that should probably be a different PR since it should be generalized throughout the app. As such, we have merely hidden an instantiated baseroute property and will create the class property some other time.

Related issue number

#206

Checklist

• Unit tests for the changes exist
• Tests pass on CI and coverage remains at 100%
• Documentation reflects the changes where applicable

[codecov]

Codecov Report

Merging #209 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #209   +/-   ##
=======================================
  Coverage   97.74%   97.75%           
=======================================
  Files          43       43           
  Lines        2220     2223    +3     
=======================================
+ Hits         2170     2173    +3     
  Misses         50       50           

Impacted Files	Coverage Δ
...ctors/google_sheets_2/google_sheets_2_connector.py	100.00% <100.00%> (ø)
toucan_connectors/toucan_connector.py	98.61% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35b7e72...d8f06e3. Read the comment docs.

[davinov]

Thanks for reworking this PR :) Passing a method to retrieve the secrets seems more versatile and convenient :)

2 notes :

• as the type of the argument is a callable (function), it would be clearer to name it get_secrets instead of just secrets
• passing it though all functions is very verbose, may we just add it once in ToucanConnector as property?

Here is a small example of what it could be:

from pydantic import BaseModel
from typing import Callable

class DummyDataSource(BaseModel):
    table: str

class DummyConnector(BaseModel):
    host: str
    secrets_id: str
    
    get_secrets: Callable[[str], dict]
            
    def get_df(self, data_source: DummyDataSource):
        print('get_df for table', data_source.table)
        print('with secrets: ', self.get_secrets(self.secrets_id))

def get_dummy_secrets(secrets_id):
    if secrets_id == 'aaa':
        return {'token': '1234567890'}
    elif secrets_id == 'bbb':
        return {'token': 'azertyuiop'}

conn_config = {'host': '0.2.3.4', 'secrets_id': 'aaa'}
conn = DummyConnector(get_secrets=get_dummy_secrets, **conn_config)
ds = DummyDataSource(table='plop')

conn.get_df(ds)

(I run it in a jupyter notebook to check that it's actually working ;) )

[testinnplayin]

Thanks for reworking this PR :) Passing a method to retrieve the secrets seems more versatile and convenient :)

2 notes :

* as the type of the argument is a callable (function), it would be clearer to name it `get_secrets` instead of just `secrets`

* passing it though all functions is very verbose, may we just add it once in `ToucanConnector` as property?

Here is a small example of what it could be:

from pydantic import BaseModel
from typing import Callable

class DummyDataSource(BaseModel):
    table: str

class DummyConnector(BaseModel):
    host: str
    secrets_id: str
    
    get_secrets: Callable[[str], dict]
            
    def get_df(self, data_source: DummyDataSource):
        print('get_df for table', data_source.table)
        print('with secrets: ', self.get_secrets(self.secrets_id))

def get_dummy_secrets(secrets_id):
    if secrets_id == 'aaa':
        return {'token': '1234567890'}
    elif secrets_id == 'bbb':
        return {'token': 'azertyuiop'}

conn_config = {'host': '0.2.3.4', 'secrets_id': 'aaa'}
conn = DummyConnector(get_secrets=get_dummy_secrets, **conn_config)
ds = DummyDataSource(table='plop')

conn.get_df(ds)

(I run it in a jupyter notebook to check that it's actually working ;) )

Si je rajoute ça en tant que propriété 'Callable' ça me donne deux erreurs : l'etl n'accepte plus GoogleSheets2Connector comme connector valable mais aussi ça provoque une erreur dans le JSONSchema.

Je pense qu'on serait obligé de le mettre dans le JSONSchema quand-même pour éviter l'erreur et puis du coup on va avoir à nouveau le problème de l'affichage dans le formbuilder.

Est-ce que c'est vraiment, vraiment nécessaire de le stocker comme propriété du connecteur ?

[davinov]

Si je rajoute ça en tant que propriété 'Callable' ça me donne deux erreurs : l'etl n'accepte plus GoogleSheets2Connector comme connector valable mais aussi ça provoque une erreur dans le JSONSchema.

Well, I thought it would make more sense to pass these parameter once to the connector, and then forget about it. Rather than adding kwargs everywhere in the connector.

I think the issue about the typing and schema could be solved. We do have similar stuff in the init_subclass of ToucanConnectors (a bit tricky), or with an underscored property and a setter (simpler).

However, this would also nicely be solved by solving #206 :p Hoping to do this in a near future.

Est-ce que c'est vraiment, vraiment nécessaire de le stocker comme propriété du connecteur ?

Of course not... Sorry about being picky

[testinnplayin]

Sorry, only just realized I answered in French: my mistake!

You're not being picky at all :) You're probably right... I'm just struggling to get it to work right.

[davinov]

Hi again!
I've been sleeping on this and, as we will probably very soon tackle #206 , it's probably a more pragmatic approach to get a first working version as you did.
So let's keep that the way it is :) And we'll think about a more future-proof API later on

[davinov]

Nice use of partial :)

[davinov]

Thanks for commenting on this!
Should we rather use the python version check in the if so it's more obvious?
If some coverage issue happen, it's OK to add a pragma comment to indicate that this branch will not be covered.