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

GET /jobs #150

Merged
merged 40 commits into from Aug 1, 2019
Merged

GET /jobs #150

merged 40 commits into from Aug 1, 2019

Conversation

@daltonmatos
Copy link
Member

daltonmatos commented Jul 24, 2019

No description provided.

daltonmatos added 6 commits Jul 24, 2019
Recebe *args, para ficar mais simples de chamar com múltiplas fixtures
O nome do job na fixture não refletia o nome do aquivo no disco.
@daltonmatos daltonmatos force-pushed the feature/scheduled-jobs-list branch from 20d876e to fc5aab7 Jul 24, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 24, 2019

Codecov Report

Merging #150 into master will increase coverage by 0.24%.
The diff coverage is 95.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   91.44%   91.69%   +0.24%     
==========================================
  Files         103      104       +1     
  Lines        3005     3215     +210     
  Branches      166      209      +43     
==========================================
+ Hits         2748     2948     +200     
- Misses        244      254      +10     
  Partials       13       13
Flag Coverage Δ
#typehint 44.81% <53.09%> (+1%) ⬆️
#unittest 95.78% <98%> (+0.12%) ⬆️
Impacted Files Coverage Δ
asgard/api/resources/users.py 100% <ø> (ø) ⬆️
asgard/api/users.py 91.08% <100%> (+0.08%) ⬆️
asgard/api/resources/__init__.py 100% <100%> (ø)
asgard/http/exceptions.py 100% <100%> (ø) ⬆️
asgard/api/resources/jobs.py 100% <100%> (ø) ⬆️
asgard/services/jobs.py 100% <100%> (ø) ⬆️
asgard/backends/chronos/models/converters.py 100% <100%> (ø) ⬆️
asgard/backends/jobs.py 100% <100%> (ø) ⬆️
asgard/models/job.py 98.33% <100%> (+0.46%) ⬆️
asgard/exceptions/__init__.py 100% <100%> (ø) ⬆️
... and 5 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 ce53fd9...8077dcd. Read the comment docs.

@auth_required
async def list_jobs(request: web.Request):
user = await User.from_alchemy_obj(request["user"])
account = await Account.from_alchemy_obj(request["user"].current_account)

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 25, 2019

Collaborator

É possível obter .current_account diretamente do objeto user já populado acima no lugar de utilizar o objeto populado na request request['user'] ?

This comment has been minimized.

Copy link
@daltonmatos

daltonmatos Jul 25, 2019

Author Member

É possível sim. São o mesmo objeto nas duas linhas (request["user"]). Na verdade quero sumir com isso aí, e já receber (user: User, account: Account) na assinatura do controller.

for job in chronos_jobs
if job.name.startswith(filter_prefix)
]
all_jobs.sort(key=lambda job: job.id)

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jul 25, 2019

Collaborator

👏

This comment has been minimized.

Copy link
@daltonmatos

daltonmatos Jul 25, 2019

Author Member

Pois é. Isso vai resolver uma annoyance do chronos hoje, mas o ideal mesmo é podermos ter um storage que já devolve ordenado. Mas até lá.... =D

asgard/clients/chronos/client.py Outdated Show resolved Hide resolved
itests/util.py Outdated Show resolved Hide resolved
itests/util.py Show resolved Hide resolved
Adequando à regra de nomeação de constantes
@pabrrs
pabrrs approved these changes Jul 25, 2019
Copy link
Collaborator

pabrrs left a comment

🌵 🍰

Os jobs retornados não estavam tendo seus namespaces removidos, o que
significa que um job não ia conseguir ser editado nunca, pois no get ele
vem com namespace e no post um novo namespace será adicoinado.
@daltonmatos daltonmatos requested a review from pabrrs Jul 25, 2019
@pabrrs
pabrrs approved these changes Jul 25, 2019
daltonmatos added 4 commits Jul 26, 2019
@pabrrs
pabrrs approved these changes Jul 26, 2019
daltonmatos added 8 commits Jul 26, 2019
Sempre que criamos um novo job ele precisa receber uma constraint que é
quem vai definir em qual grupo de máquinas esse job vai rodar.

Essa constraint é a `owner`. **Todo** job precisa ter constraint
`owner:LIKE:{account.owner}` onde o {account} é a conta que esse job
pertence.
daltonmatos added 20 commits Jul 29, 2019
Muitas vezes quando buscamos um conteúdo no chronos logo após esse
conteúdo ter sido criado o chronos responde como não encontrado.
Deve apenas retornar um HTTP e a exception deve ser lançada pelo código
do backend.

O ajuste aqui foi apenas no mock, o código estava correto
Essa implementação é um paliativo enquando nao refazemos a API do
`asgard.http.client`. Quando fizermos essa re-implementação poderemos
voltar a usar o client padrão aqui.
A auth vem de uma nova envvar: SCHEDULED_JOBS_SERVICE_AUTH
Dessa forma evitamos os `.split(":")` e deixamos a validação para o
Pydantic, pagando o preço de ter uma envvar com um conteúdo mais complexo.
O código não estava checando se a env já existe antes de setar o valor
final
…d-jobs-create
…s-jobs-delete
…obs-update
…obs-create
…obs-list
@daltonmatos daltonmatos merged commit 8077dcd into master Aug 1, 2019
7 checks passed
7 checks passed
ci/circleci: py368 Your tests passed on CircleCI!
Details
ci/circleci: py36x Your tests passed on CircleCI!
Details
ci/circleci: py37x Your tests passed on CircleCI!
Details
codecov/patch/typehint 53.09% of diff hit (target 43.8%)
Details
codecov/patch/unittest 98% of diff hit (target 95.65%)
Details
codecov/project/typehint 44.81% (+1%) compared to ce53fd9
Details
codecov/project/unittest 95.78% (+0.12%) compared to ce53fd9
Details
@daltonmatos daltonmatos deleted the feature/scheduled-jobs-list branch Aug 29, 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.