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

[4] GET /jobs/{job_id} #142

Conversation

@daltonmatos
Copy link
Member

daltonmatos commented Jul 19, 2019

No description provided.

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.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 19, 2019

Codecov Report

Merging #142 into feature/scheduled-jobs-from-asgard-to-client-model will increase coverage by 0.38%.
The diff coverage is 96.63%.

Impacted file tree graph

@@                                  Coverage Diff                                   @@
##           feature/scheduled-jobs-from-asgard-to-client-model     #142      +/-   ##
======================================================================================
+ Coverage                                               88.57%   88.96%   +0.38%     
======================================================================================
  Files                                                      97      101       +4     
  Lines                                                    2924     3008      +84     
  Branches                                                  160      166       +6     
======================================================================================
+ Hits                                                     2590     2676      +86     
+ Misses                                                    321      319       -2     
  Partials                                                   13       13
Flag Coverage Δ
#typehint 44.15% <67.6%> (+0.31%) ⬆️
#unittest 92.9% <96%> (+0.31%) ⬆️
Impacted Files Coverage Δ
asgard/backends/models/converters.py 100% <ø> (ø) ⬆️
asgard/handlers/http.py 0% <0%> (ø) ⬆️
asgard/api/resources/jobs.py 100% <100%> (ø)
asgard/models/account.py 68.18% <100%> (ø) ⬆️
asgard/clients/chronos/__init__.py 100% <100%> (+45.45%) ⬆️
asgard/clients/chronos/models/job.py 100% <100%> (ø) ⬆️
asgard/http/client.py 68.42% <100%> (+0.85%) ⬆️
asgard/backends/chronos/impl.py 100% <100%> (ø)
asgard/backends/chronos/models/converters.py 100% <100%> (ø) ⬆️
asgard/backends/jobs.py 100% <100%> (ø)
... and 7 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 cab3a87...dcb7053. Read the comment docs.

daltonmatos added 2 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.
@daltonmatos daltonmatos changed the title GET /jobs/{job_id} [4] GET /jobs/{job_id} Jul 19, 2019
itests/asgard/api/test_jobs.py Outdated Show resolved Hide resolved
daltonmatos added 3 commits Jul 22, 2019
Copy link
Collaborator

pabrrs left a comment

Qual o impacto da alteração de key para name ?


class ChronosScheduledJobsBackend(ScheduledJobsBackend):
def __init__(self) -> None:
self.client = ChronosClient(settings.SCHEDULED_JOBS_SERVICE_ADDRESS)

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Aqui não é necessário chamar a inicialização da classe ScheduledJobsBackend ?

def __init__(self) -> None:
    self.client = ChronosClient(settings.SCHEDULED_JOBS_SERVICE_ADDRESS)
    super(ScheduledJobsBackend, self).__init__()

This comment has been minimized.

Copy link
@daltonmatos

daltonmatos Jul 22, 2019

Author Member

Por enquanto não, pois a ideia é não ter nada na classe mãe mesmo. Ela seria "apenas" uma interface, mostrando quais métodos as classes filhas devem implementar obrigatoriamente.

asgard/backends/chronos/models/converters.py Outdated Show resolved Hide resolved
@daltonmatos

This comment has been minimized.

Copy link
Member Author

daltonmatos commented Jul 22, 2019

Qual o impacto da alteração de key para name ?

Estava mapeado errado. 😄 Eu achei que o Chronos retorna os objetos que representam uma env como {"key": "...", "name": "..."} mas descobri, quando fiz o teste end to end que na verdade era {"name": "...", "value": "..."}.

daltonmatos added 5 commits Jul 22, 2019
Explica a nova arquitetura de models e ModelConverters.
Atualiza pontos onde o Chronos pode ser mencionado.
Copy link
Collaborator

pabrrs left a comment

👏

Algumas considerações sobre list comprehension.
(Perdoa a chatisse 😅)

env_spec_list = [
ChronosEnvSpec(name=name, value=value)
for name, value in other.items()
]

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Aqui é possível retornar direto o valor da lista:

def to_client_model(cls, other: EnvSpec) -> List[ChronosEnvSpec]:
    return [
        ChronosEnvSpec(name=name, value=value)
        for name, value in other.items()
    ]
)
for fetch_item in other
]

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Aqui também!

)
for fetch_item in other
]

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Aqui também!

constraint_spec.append(f"{item[0]}:{item[1]}:{item[2]}")
constraint_spec: ConstraintSpec = [
f"{item[0]}:{item[1]}:{item[2]}" for item in other
]

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 22, 2019

Collaborator

Aqui também!

…uled-jobs-get-job-by-id
@daltonmatos daltonmatos merged commit dcb7053 into feature/scheduled-jobs-from-asgard-to-client-model Jul 23, 2019
2 of 3 checks passed
2 of 3 checks passed
ci/circleci: py36x CircleCI is running your tests
Details
ci/circleci: py368 Your tests passed on CircleCI!
Details
ci/circleci: py37x Your tests passed on CircleCI!
Details
@daltonmatos

This comment has been minimized.

Copy link
Member Author

daltonmatos commented Jul 23, 2019

Algumas considerações sobre list comprehension.
(Perdoa a chatisse )

Nem é!! Estou mergeando todos os PRs em um só e vou fazer esses ajustes lá. 🎉

@daltonmatos daltonmatos deleted the feature/scheduled-jobs-get-job-by-id 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.