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

Proposta de padronização de erros #129

Closed
wants to merge 7 commits into from
Closed

Proposta de padronização de erros #129

wants to merge 7 commits into from

Conversation

liverday
Copy link
Contributor

Esse pull request carrega o início de uma proposta para a padronização dos erros do tn.
Alguns pontos para se considerar nesse PR:

  • Foi criado um middleware para ser utilizado no next-connect para incluir um trace-id para cada uma das requests.
  • Foi criada uma factory para lidar com uuid, chamei de UuidMaker mas não é um bom nome, rs. Estou aberto a sugestões.
  • Criei uma pasta chamada errors dentro da pasta infra do projeto feita para guardar todos os erros tratados que teremos na aplicação. A minha dúvida é se isso pertenceria a pasta de infra, ou entraria em uma pasta de domain, por fazer parte das nossas regras de negócio.
  • Dentro do onErrorHandler, eu coloquei uma verificação onde só muda o status code da requisição quando for um erro tratado por nós.
  • Criei um arquivo chamado does-not-exist-error.js para dar um exemplo de como seria criar um erro que retorne 404 para o client.

Fico aberto a sugestões de melhoria ou discussões a respeito da implementação.

@vercel
Copy link

vercel bot commented Sep 23, 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.

@brunofamiliar
Copy link
Contributor

Bacana a proposta!! Só uma observação, acredito que essa página inicial de colaboradores não será mais editada, @filipedeschamps certo??

@filipedeschamps
Copy link
Owner

@brunofamiliar correto meu caro!

E muito obrigado pela contribuição @liverday eu ia trabalhar nisso hoje e você adiantou muito* tudo, estamos quase lá 😍 vou fazer o Review colocando alguns comentários 👍 e se me permitir, dou continuidade ao PR assim como fiz com o último do @rodrigoKulb com outros ajustes 👍

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.

Muito obrigado por todas as contribuições meu caro, matou a pau!!! Vou dar continuidade nas suas implementações em outros PRs com o objetivo de remover algumas abstrações e tentar aumentar a cobertura nos testes 👍

infra/errors/app-error.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
infra/uuid.js Outdated Show resolved Hide resolved
pages/init/collaborators.json Outdated Show resolved Hide resolved
pages/api/v1/migrations/index.public.js Show resolved Hide resolved
pages/api/v1/migrations/index.public.js Outdated Show resolved Hide resolved
@liverday
Copy link
Contributor Author

@filipedeschamps Resolvi todos os apontamentos feito no review! Fique a vontade de continuar com as implementações e fico feliz de ter ajudado!

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.

Sensacional!! Obrigado pelos ajustes e pela contribuição 👍 Vou fazer merge desse PR, mas antes de fechar a issue de fato, vou fazer outros ajustes 🤝 👍

@filipedeschamps
Copy link
Owner

@paulogdm tava cavucando a internet por uma solução [1][2][3], mas não estou conseguindo encontrar, veja o que está acontecendo na lista de checks: o deploy preview da Vercel parece que está bloqueando os outros checks.

image

Você sabe dizer se existe alguma opção em que tente fazer o deploy, mas que não apareça como um check? Ou ao menos que não iniba os outros checks?

Vi que um outro recurso que também solucionaria isso, mas está no roadmap: vercel/vercel#5776

Seja com qual recurso for, seria legal alguém que não tenha permissão para deploy, mas tenha write no repositório conseguir rodar os checks 👍 (e as vezes vai, quando randomicamente muda a ordem dos checks)

@liverday
Copy link
Contributor Author

@filipedeschamps Peço perdão, criei um novo commit aqui agora para realizar os lints do Prettier, que acabou retirando seu approve.

@filipedeschamps
Copy link
Owner

@liverday sem problemas! To numa branch local aqui onde fiz o squash dos seus commits para traduzir tudo em uma única mensagem em inglês (os seus commits estavam em português) e já aproveitei para fazer o rebase (e agora) consertar o lint 👍

Em cima desse commit eu vou fazer algumas alterações que vão acabar impactando diretamente o PR #130 mas sem problemas também, pois vamos ajustando ao longo da jornada 🤝

@rhandrade
Copy link
Contributor

@filipedeschamps Será que não daria para fazer nesse nesse modelo? Usaria o modelo do Ignored Build Step para rodar um script personalizado que estaria no root do projeto, no qual ele só tentaria fazer o deploy se fosse seu usuário. Ai ativaria a opção para expor as variáveis de ambiente automaticamente, como mostra nesse link.

Talvez fazendo dessa forma, o cancelamento do deploy da Vercel não barre os outros checks. Como somente seu usuário tem permissão de fazer deploy lá, isso resolveria o problema.

#!/bin/bash

echo "VERCEL_GIT_COMMIT_AUTHOR_LOGIN: $VERCEL_GIT_COMMIT_AUTHOR_LOGIN"

if [[ "$VERCEL_GIT_COMMIT_AUTHOR_LOGIN" == "filipedeschamps" ]] ; then  
  echo "✅ - Build and deploy can proceed"
  exit 1;
else  
  echo "🛑 - Build and deploy cancelled because author is not filipedeschamps."
  exit 0;
fi

image

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

4 participants