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

Cria Endpoint /status para lidar com Health Check das dependências do TabNews #130

Merged
merged 2 commits into from
Oct 18, 2021
Merged

Cria Endpoint /status para lidar com Health Check das dependências do TabNews #130

merged 2 commits into from
Oct 18, 2021

Conversation

liverday
Copy link
Contributor

@liverday liverday commented Sep 24, 2021

Esse PR é referente à issue: #102 e ele cria um endpoint com rota /status para verificar a saude dos componentes.

Esse PR contém as mudanças que estão pendentes de merge do PR: #131

Esse PR Contempla:

  • Criação do endpoint /status que roda todos os health-indicators do projeto.
  • Cria uma factory health-checker, onde o objeto criado nela é responsável por executar todos os health-checks disponíveis no TN.
  • Cria um database-health-indicator que é responsável por realizar uma query no banco retornando as conexões ativas.
  • Cria um teste integrado para validar a saude do banco de dados usado nos testes.

@vercel
Copy link

vercel bot commented Sep 24, 2021

@liverday is attempting to deploy a commit to the TabNews Team on Vercel.

To accomplish this, @liverday needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@filipedeschamps
Copy link
Owner

@liverday antes de eu mergulhar na busca por um editor, eu vim aqui no repositório :) desculpa esse vai e volta, mas conseguindo achar uma pessoa para editar, vai melhorar muito minha participação aqui no repositório 🤝

Estava testando seu PR, e segue minhas observações:

  1. O JSON retornado está bem legal, bem completo! 🤝

  2. Você chegou a testar outros caminhos a não ser o de sucesso? Digo isso pois ao rodar o status com o banco down, o código estoura por não ter sido importado o custom error:
    image

  3. E você chegou a rodar os testes? Ele quebra por um problem similar de custom error:
    image

  4. Será que fiz um pull errado aqui no meu teste local?

  5. De qualquer forma, não sei se você vai ter interesse, mas como desafio seria legal você reimplementar mas da forma mais simples que conseguir pensar. Por exemplo, tentar resolver esse problema inteiro dentro um Model (numa pasta model que não existe), sem separar em arquivos por enquanto 👍

@liverday
Copy link
Contributor Author

Oi @filipedeschamps, você vai me perdoar, por um erro meu um commit não tinha sido feito o push.

Acabei de atualizar o PR.
A respeito do desafio, irei pensar numa solução, mas só para ver se eu entendi. A intenção é colocar tudo relacionado ao health-check dentro de um arquivo na pasta model, certo?

Se eu entendi direito, vou começar a implementação já!

Muito obrigado pelo feedback e perdão pela falta de atenção.

@liverday
Copy link
Contributor Author

liverday commented Oct 15, 2021

Acabei de criar um arquivo model/health.js (Fiquei na dúvida se chamava de health.js ou status.js) onde consolida toda a regra de negócio que envolve o health-check.

O que vocês acham dessa implementação?

Copy link
Owner

@filipedeschamps filipedeschamps left a comment

Choose a reason for hiding this comment

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

Que show! 😍 ficou mais simples e isso é ótimo 🤝 duas coisas bem simples para fazermos o merge:

  1. Uma modificação bem pequena nos testes, para cobrir toda interface.
  2. Fazer rebase e squash de todos os commits 👍

Parabéns pela implementação, estamos bem próximos 😍


expect(serverStatusResponse.status).toEqual(200);
expect(serverStatusBody.updated_at).toBeDefined();
expect(serverStatusBody.dependencies.database).toEqual(expect.objectContaining({ status: 'healthy' }));
Copy link
Owner

Choose a reason for hiding this comment

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

Aqui eu faria um teste contra toda a interface pública, então isso deveria também testar a existência da chave opened_connections 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pronto, resolvido!

@filipedeschamps
Copy link
Owner

Ah, e rodei local, tanto os testes quanto derrubando o banco e o endpoint se comportou perfeitamente 👍

Em paralelo, deu até vontade de mover o Migrator pra dentro dos models, para que os controllers usem apenas os models e nada da camada da infra... mas isso deixa para outro PR 🤝

@liverday
Copy link
Contributor Author

Realizei o squash dos commits conforme solicitado @filipedeschamps!

@filipedeschamps filipedeschamps changed the base branch from main to release October 18, 2021 20:21
@filipedeschamps filipedeschamps merged commit 5cb9703 into filipedeschamps:release Oct 18, 2021
@filipedeschamps filipedeschamps deleted the status-endpoint-com-health-check branch October 18, 2021 20:23
@filipedeschamps
Copy link
Owner

Show! Fiz o squash numa branch release pra rodar os checks, mas não rodaram. Quebrou alguma integração e estou verificando 🤝

@filipedeschamps filipedeschamps restored the status-endpoint-com-health-check branch October 18, 2021 20:29
@filipedeschamps
Copy link
Owner

@liverday alguns checks rodaram no outro PR, e quebrou na mensagem de commit (mas acho que foi por conta do squash que eu fiz pelo Github, porque ainda tinham dois commits no PR), mas o mais importante é que quebrou nas regras de linting.

Você pode abrir um novo PR consertando o lint (npm run lint:fix) e fazendo o squash de tudo? 🤝

@liverday
Copy link
Contributor Author

@filipedeschamps Fiz um novo PR aqui: #136.

Nesse, eu fiz o squash dos commits deixando uma única mensagem, rodei o npm run lint:fix e parece okay agora!
Esse foi legal, aprendi varias coisas, haha.

E mais uma vez, perdão se houve alguma confusão por falta de entendimento de minha parte.

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.

None yet

2 participants