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

[1] Início do modelo ScheduledJob #137

Merged

Conversation

@daltonmatos
Copy link
Member

daltonmatos commented Jul 10, 2019

Ainda não sei onde ficarão os modelos que são comuns a vários outros,
como por exemplo, ConstraintSpec, EnvSpec, etc.

Se formos chamá-los de *Spec talvez faça sentido estar em
asgard.models.specs.*.

daltonmatos added 4 commits Jul 9, 2019
Ainda não sei onde ficarão os modelos que são comuns a vários outros,
como por exemplo, ConstraintSpec, EnvSpec, etc.

Se formos chamá-los de `*Spec` talvez faça sentido estar em
`asgard.models.specs.*`.
Esses sub-modelos estão em `asgard.models.spec.*`
daltonmatos added 5 commits Jul 12, 2019
Quando usamos o client inteito no context block, assim:

```
from asgard.http.client import http_client

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

A session deve permanecer aberta para que possamos ter novos blocos como
esse pelo código. Fechando a session on final do bloco ia nos obrigar a
criar uma nova session a cada novo bloco, o que nem é recomendado pela
doc do aiohttp client: https://docs.aiohttp.org/en/stable/client_quickstart.html

para os casos onde fazemos:

```
async with http_client.get(...) as response:
  ...
```

Estamos ainda criando uma nova session para cada context block.
O Ambiente local agora conta com um chronos rodando. Ele vai servir para
os testes de integração da api de scheduled jobs
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #137 into feature/remove-erros-key-from-models will increase coverage by 0.44%.
The diff coverage is 100%.

Impacted file tree graph

@@                           Coverage Diff                            @@
##           feature/remove-erros-key-from-models     #137      +/-   ##
========================================================================
+ Coverage                                 87.32%   87.76%   +0.44%     
========================================================================
  Files                                        86       93       +7     
  Lines                                      2595     2689      +94     
  Branches                                    158      159       +1     
========================================================================
+ Hits                                       2266     2360      +94     
  Misses                                      316      316              
  Partials                                     13       13
Flag Coverage Δ
#typehint 33.93% <91.93%> (+3.67%) ⬆️
#unittest 92.43% <100%> (+0.29%) ⬆️
Impacted Files Coverage Δ
asgard/models/job.py 100% <100%> (ø)
asgard/models/spec/env.py 100% <100%> (ø)
asgard/http/client.py 67.56% <100%> (ø) ⬆️
asgard/models/spec/constraint.py 100% <100%> (ø)
asgard/models/spec/container.py 100% <100%> (ø)
asgard/models/spec/fetch.py 100% <100%> (ø)
asgard/models/spec/schedule.py 100% <100%> (ø)
asgard/conf.py 100% <100%> (ø) ⬆️
asgard/api/jobs.py 100% <100%> (ø)
... 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 523122c...5df7ad1. Read the comment docs.

daltonmatos added 9 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
@daltonmatos daltonmatos marked this pull request as ready for review Jul 18, 2019
@daltonmatos daltonmatos changed the title Início do modelo ScheduledJob [1] Início do modelo ScheduledJob Jul 18, 2019
daltonmatos added 8 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(...)
```
daltonmatos added 9 commits Jul 19, 2019
namespace

Essa interface é quem vai implementar a adição/remoção do namespace
no nome das aplicações.
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.
async def index_jobs(request: web.Request):
data = await request.json()
job = ScheduledJob(**data)
return web.json_response(job.dict())

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Alguns questionamentos:

  • Esse método não deveria ter o decorator @auth_required ?
  • index_jobs é responsável por criar jobs ? Se for, o que acha de chama-lo create_job ?

This comment has been minimized.

Copy link
@daltonmatos

daltonmatos Jul 22, 2019

Author Member

Isso mudou! Esse código era apenas um esboço de como a controller (handler) deveria ser. Era só pra eu ter uma ideia da "cara" que o código teria. Esse código já muodu no #142. E fiz exatamente o que você sugeriu aqui. Mudei o nome e adicionei o @auth_required. 🎉

asgard/models/spec/constraint.py Outdated Show resolved Hide resolved
class ScheduleSpec(BaseModel):
type = "ASGARD"
value: str
tz: str = "UTC"

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Aqui também não parece ser necessário tipar tz

This comment has been minimized.

Copy link
@daltonmatos

daltonmatos Jul 22, 2019

Author Member

Correto.

conftest.py Show resolved Hide resolved
dev/chronos.sh Show resolved Hide resolved
self.session_class_mock.assert_called_with(
timeout=default_http_client_timeout
)
client.session.close.assert_not_awaited()

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Talvez seja possível remover essa instrução client.session.close.assert_not_awaited() de dentro do async with client, pois logo abaixo ela é chamada novamente. Assim, essa função será chamada apenas ao final do fluxo, após o with.

Suggested change
client.session.close.assert_not_awaited()

This comment has been minimized.

Copy link
@daltonmatos

daltonmatos Jul 22, 2019

Author Member

O que eu tentei fazer aqui é ter a certeza que, durante a existência do "context block" a sessioin não é fechada, ou seja, pelo tempo que meu context existir tenho a garantia de que a session está pronta para ser usada.

daltonmatos added 9 commits Jul 22, 2019
Explica a nova arquitetura de models e ModelConverters.
Atualiza pontos onde o Chronos pode ser mencionado.
…uled-jobs-get-job-by-id
…duled-jobs-from-asgard-to-client-model
…o feature/scheduled-jobs-from-client-to-asgard-model
…o feature/scheduled-jobs-models
@daltonmatos daltonmatos merged commit 5df7ad1 into feature/remove-erros-key-from-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 Your tests are queued behind your running builds
Details
ci/circleci: py37x Your tests are queued behind your running builds
Details
@daltonmatos daltonmatos deleted the feature/scheduled-jobs-models 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.