Skip to content
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

Added support for stores #46

Merged
merged 6 commits into from
Dec 23, 2022
Merged

Added support for stores #46

merged 6 commits into from
Dec 23, 2022

Conversation

rg2011
Copy link
Collaborator

@rg2011 rg2011 commented Dec 16, 2022

Importa en la librería el mecanismo de stores que estamos usando en varias ETLs:

  • Propone una interfaz común Store para almacenar entidades NGSIv2 en backends genéricos. La interfaz consiste en un solo método con un solo parámetro, entities.
  • Implementa dos Stores ajustados a la interfaz:
    • orionStore que escribe las entidades a un cbManager con send_batch
    • sqlFileStore que escribe las entidades a un fichero SQL. Añade una dependencia a la librería psycopg2-binary

@@ -37,7 +37,8 @@
#Paquetes necesarios para que funcione la librería. Se instalarán a la vez si no lo tuvieras ya instalado
INSTALL_REQUIRES = [
'requests==2.21.0',
'urllib3==1.24.1'
'urllib3==1.24.1',
'psycopg2-binary>=2.9.5'
Copy link
Contributor

Choose a reason for hiding this comment

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

Dudo de si es bueno dejar dependencias abiertas. ¿Mejor == o >=? ¿Que dice la literatura al respecto?

Copy link
Collaborator Author

@rg2011 rg2011 Dec 16, 2022

Choose a reason for hiding this comment

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

Nos ha pasado en algunas ETLs que hemos tenido que cambiar nuestro requirements.txt porque por ejemplo usábamos una versión de requests distinta a la 2.21.0, y al incluir tc_etl_lib, el pip install -r requirements.txt daba conflicto.

Al ser tc_etl_lib una libreria más que el usuario va a importar, creo que es bueno que sea más permisiva y deje que el usuario elija las versiones de sus dependencias, dentro de un rango de compatibilidades. El usuario puede tener otras dependencias que requieran una versión de requests compatible pero no exactamente la misma.

Digo requests porque es la más propensa a estar ya en otras dependencias o en el requirements.txt del usuario. Aunque en esta PR la que se añade es psycopg2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

la literatura parece que concuerda: https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/

It is not considered best practice to use install_requires to pin dependencies to specific versions, or to specify sub-dependencies (i.e. dependencies of your dependencies). This is overly-restrictive, and prevents the user from gaining the benefit of dependency upgrades.

Copy link
Collaborator Author

@rg2011 rg2011 Dec 16, 2022

Choose a reason for hiding this comment

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

me he puesto a escribir y resulta que ya estaba todo en el issue #12 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Pues sí, parece que estais alineeados con #12 :)

Solo faltaría aplicarlo a las demás (requests, etc.). Pero eso ya en otra PR, la que ataque directamente el #12

Copy link
Collaborator

Choose a reason for hiding this comment

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

si me parece bien que sea más permisiva la librería en cuanto a versiones.

Copy link
Contributor

Choose a reason for hiding this comment

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

NTC

@@ -0,0 +1,192 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

Creo que este es el primer test que metemos para la libreria... ¿pudiera ser? ¡Bien! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Recordemos tenemos este issue por ahí: #3

entities = ...
store(entities)
```

## Changelog
Copy link
Contributor

Choose a reason for hiding this comment

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

Habría que incluir en el changelog la entrada o entradsa corresponiente a los cambios en esta PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@arcosa
Copy link
Collaborator

arcosa commented Dec 20, 2022

Tiene buena pinta todo. LGTM

Copy link
Contributor

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan fgalan merged commit 7682777 into master Dec 23, 2022
@fgalan fgalan deleted the feature/store branch December 23, 2022 08:21
@fgalan fgalan mentioned this pull request Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants