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

[2] Conversão chronos.models.* -> asgard.models.* #139

Conversation

@daltonmatos
Copy link
Member

daltonmatos commented Jul 17, 2019

No description provided.

daltonmatos added 8 commits Jul 16, 2019
Essa interface deve ser implementada por todos os backends que precisem
converter os models de seus clients internos (`asgard.clients.*.models.*`)
para os models do asgard (`asgard.models.*`).
Conversores de models do Chronos pra modelos do Asgard
fmt
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #139 into feature/scheduled-jobs-models will increase coverage by 0.63%.
The diff coverage is 97.39%.

Impacted file tree graph

@@                        Coverage Diff                        @@
##           feature/scheduled-jobs-models     #139      +/-   ##
=================================================================
+ Coverage                          87.76%   88.39%   +0.63%     
=================================================================
  Files                                 93       97       +4     
  Lines                               2689     2887     +198     
  Branches                             159      160       +1     
=================================================================
+ Hits                                2360     2552     +192     
- Misses                               316      322       +6     
  Partials                              13       13
Flag Coverage Δ
#typehint 42.07% <97.19%> (+8.13%) ⬆️
#unittest 92.5% <94.91%> (+0.07%) ⬆️
Impacted Files Coverage Δ
asgard/models/spec/schedule.py 100% <ø> (ø) ⬆️
asgard/models/base.py 95% <ø> (-0.24%) ⬇️
asgard/models/job.py 100% <100%> (ø) ⬆️
asgard/clients/chronos/models/job.py 100% <100%> (ø)
asgard/models/spec/fetch.py 100% <100%> (ø) ⬆️
asgard/backends/models/converters.py 100% <100%> (ø)
asgard/models/spec/constraint.py 100% <100%> (ø) ⬆️
asgard/models/spec/container.py 100% <100%> (ø) ⬆️
asgard/models/spec/env.py 100% <100%> (ø) ⬆️
asgard/models/agent.py 91.42% <100%> (+0.25%) ⬆️
... and 6 more

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 46f5a1a...92910ae. Read the comment docs.

@daltonmatos daltonmatos changed the title Feature/scheduled jobs from client to asgard model [2] Feature/scheduled jobs from client to asgard model Jul 18, 2019
daltonmatos added 12 commits Jul 18, 2019
As chaves são name e value e não `key` e `value`
Estava chamendo com os parametros invertidos
Se não fechamos a sessão, caso o loop seja trocado, o http_client
que já foi usado antes (e por isso já tem uma session aberta) para
de funcionar com o erro "loop is closed".

O que temos que fazer é melhorar a API desse http_client. Idealmente
exigino que ele seja instanciado antes de ser usado.
Em vez de fazermos assim:

```python
from asgard.http.client import http_client

async with http_client as client:
  await client.get(...)
```

Fazer algo nessa linha:
```python
from asgard.http.client import HttpClient

client = HttpClient(headers=..., ...)
await client.get(...)
```
namespace

Essa interface é quem vai implementar a adição/remoção do namespace
no nome das aplicações.
ModelConverterInterface[ScheduledJob, ChronosJob]
):
@classmethod
def to_asgard_model(cls, other: ChronosJob) -> ScheduledJob:

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 19, 2019

Collaborator

Que tal utilizar um nome mais semântico para other ? Talvez um job ou chronos_job

This comment has been minimized.

Copy link
@daltonmatos

daltonmatos Jul 22, 2019

Author Member

Penso que faz sentido, mas acredito tamém que o declaração de tipo já faz esse papel. Sua ideia era mudar apenas esse converter ou mudar todos?

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Acredito que mudar em todos faça mais sentido, porém se for entrar em um refactor-hell, nem vale a pena 😅

for v in other.volumes
]
return ContainerSpec(
type="DOCKER",

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 19, 2019

Collaborator

Essa informação de type não está presente no objeto other de alguma forma ?

This comment has been minimized.

Copy link
@daltonmatos

daltonmatos Jul 22, 2019

Author Member

Pois é. Esse campo type ainda está um grande interrogação na minha cabeça. O ContainerSpec do Chronos de fato define type="DOCKER" sim, mas de fato não sei o que fazer. Penso que essa parte pode se tornar abstrata pois podemos (será?) ter múltiplos tipos de containers, apesar de hoje só existir DOCKER mesmo.

Esse type pode até sair daqui, pois ja está definino na criaçao do asgard.models.spec.container.ContainerSpec. Vou remover, mas a dúvida em relação ao type continua. =D

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Talvez seja mais interessante remover isso daqui, considerando que já está definido na criação do objeto.

Se no futuro isso mudar (se mudar), de qualquer forma, teremos que fazer um refac geral na app.

asgard/backends/chronos/models/converters.py Outdated Show resolved Hide resolved
asgard/backends/chronos/models/converters.py Outdated Show resolved Hide resolved
asgard/clients/chronos/__init__.py Outdated Show resolved Hide resolved
daltonmatos added 6 commits Jul 19, 2019
O início do teste faz um registro de um job novo no chronos,
mas log em seguida, quando buscamos esse job o chronos responde que ele
não existe.

Essa espera é para dar tempo do chronos registrar tudo e responder HTTP 200.
Explica a nova arquitetura de models e ModelConverters.
Atualiza pontos onde o Chronos pode ser mencionado.
daltonmatos added 3 commits Jul 22, 2019
@daltonmatos daltonmatos changed the title [2] Feature/scheduled jobs from client to asgard model [2] Conversão chronos.models.* -> asgard.models.* Jul 22, 2019
daltonmatos added 4 commits Jul 22, 2019
…uled-jobs-get-job-by-id
…duled-jobs-from-asgard-to-client-model
…o feature/scheduled-jobs-from-client-to-asgard-model
@daltonmatos daltonmatos merged commit 92910ae into feature/scheduled-jobs-models Jul 23, 2019
0 of 3 checks passed
0 of 3 checks passed
ci/circleci: py368 CircleCI is running your tests
Details
ci/circleci: py36x CircleCI is running your tests
Details
ci/circleci: py37x CircleCI is running your tests
Details
@daltonmatos daltonmatos deleted the feature/scheduled-jobs-from-client-to-asgard-model branch Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.