feat: adiciona código de erro semântico em casos de ceps mal formatados #38
Conversation
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/filipedeschamps/brasilapi/7rlpfxpeb |
paulo-santana
changed the title
|
@paulo-santana sensacional!!!! Mas em paralelo, eu to confuso, porque o Github não está acionando o Action, dado que o Now fez o deploy? Alguém tem alguma idéia? |
|
Cara, eu não faço ideia. Eu criei uma conta na Zeit pra poder começar a debugar, mas assim que ativei as Actions no meu fork e dei um pull request da actions pra master, ele executou os testes normalmente. Saímos do "na minha máquina funciona" pro "no meu repositório funciona". Como eu manjo pouco de github e menos ainda de actions, não sei como sair daqui pra ajudar a resolver o problema. Será que tá faltando alguma coisa no script pra fazer ele funcionar com PRs de forks diferentes?
|
|
Pelo que eu vi aqui - (thread no github.community) e no maxheld83/ghactions#262, parece que o Github intencionalmente não roda workflows para PRs que vem de outros forks, pois ele injeta informações sensíveis do repositório e do dono dele na hora da execução. Aí pessoas mal intencionadas poderiam listar secrets e chaves privadas e esse tipo de coisa apenas mandando pull requests. Pelo que parece, o workflow é executado na fork, mas não sei se tem como acessar o resultado. Que treta... |
Não seria isso? https://github.com/paulo-santana/BrasilAPI/actions/runs/52633671
Isso me parece gambiarra do GitHub, todos os outros CI/CD executam contra o PR e contra a branch, e deixam por secrets diferentes pra master e pros PRs. |
Esse é o workflow de outro pr que eu fiz apenas pra testar, da minha branch Action pra minha branch Master. Já esse PR aqui, de acordo com o appleton no primeiro link que eu mandei, teria executado o workflow no meu repositório. Eu tava pensando aqui que talvez ele não tenha executado porque eu não estava com a Actions ativada. Agora que eu tô, tô querendo mandar mais um PR pra ver se ele executa o workflow de vez kkkkkkkk. Aguentem mais uma notificação aí |
|
Bom, vamos lá: desses dois últimos commits que eu fiz, o primeiro executou os testes no meu fork, o segundo não. O primeiro executou porque eu tinha configurado o projeto na minha conta da Zeit. Então, quando ele viu o commit, o Now fez o deploy tanto na minha conta quanto na do Filipe e disparou o evento pros nossos repositórios. No meu repo os testes rodaram, mas no base não, por conta dessa limitação do github. Mas antes de fazer o segundo commit, eu deletei o projeto da minha conta da Zeit, então o bot só fez o deploy na conta do Filipe. E só a conta dele emitiu o evento. A minha não. E aí consequentemente o workflow não foi executado. Se cada pessoa que for contribuir pro projeto tiver que criar uma conta na zeit e implementar o próprio fork lá... Parece muita burocracia. E ainda tem o fato dos admins do repositório base terem que entrar no fork pra ver se os testes estão passando |
|
Show @paulo-santana obrigado por todos os testes!!! @paulogdm você sabe dizer como isso é feito no repositório do Next.js? Pelo que vejo nos PRs de lá, pessoas que não tem acesso write (não são E eu não consigo ver nada de especial dentro da action, pegando por exemplo esta action Build, test, and deploy / build (pull_request). Será que o comportamento do Github muda se o target do merge não for a branch default? No caso o Next.js usa a branch canary. @lucianopf já passou por esse tipo de situação em algum repositório? |
|
Em paralelo, o argumento de não rodar as actions de um fork por motivos de segurança, eu até entendo se não houver a separação dos tokens por branches (como o Travis faz por exemplo), mas o hook do |
|
Eu pesquisei bastante de ontem pra hoje, mas não consegui achar nada diferente do que eu encontrei um mês atrás. Aliás, pelo que eu vi nesse repositório do Next.js do Zeit, ele já executava actions bem antes de você ter implementado aqui, então tudo aquilo sobre o GH não disparar workflows pra PRs de Forks nunca foi aplicável aqui... O mais bizarro de tudo é que "do nada", desde o seu comentário ontem, os checks estão esperando o workflow rodar, e eu não consigo imaginar o porquê. Sobre o Now funcionar, eu chutaria que é porque ele é um app de terceiros, com suas próprias políticas. Na documentação da integração dele com o Github tem um parágrafo explicando que quando um PR de um fork altera o arquivo de configuração ( Eu vou fazer mais um commit pra ver se ele executa o workflow dessa vez |
É que eu coloquei esse check como
Perfeito 👍 |
Altera o estilo do código do arquivo status/v1/index.js
|
@filipedeschamps eu criei um projeto bem simples com o ambiente parecido com o da BrasilAPI, e depois de fazer alguns testes com um outro perfil eu vi que os workflows rodam normalmente pra PRs de forks desde que o evento configurado pra eles seja o Aqui eu tenho 2 arquivos de workflow, um é uma cópia do seu, que é ativado com o evento Isso resolveria o problema atual, mas de início criaria um novo, que é a perda da sincronia com o bot na publicação do projeto. "Amanhoje" vou ver se tem algum jeito de fazer o workflow disparar com o evento pull_request, mas esperar a execução dos testes até que o Now finalize o deploy. |
|
Boa @paulo-santana, será que se vc alterar essa config no arquivo de config do seu REPO já não configura corretamente pra esse PR e trigga os testes? 👀 🙏 |
|
Eu to trabalhando num workflow que realiza os testes localmente próprio servidor que o GH Actions disponibiliza. Ele até funciona, mas tem um problema em que o Jest não finaliza o processo, então o workflow fica empacado no step dele e nunca envia o resultado. Inclusive eu já to terminando, daqui a pouco eu mando um PR. Vai ser uma medida paliativa enquanto a gente não consegue testar contra o deploy na Zeit |
|
Fala @paulo-santana voltando aqui para esse PR! Show que funciona no |
|
Funcionou como esperado! Mas talvez o interessante seria eu ter dado mais semântica para o PR nessa branch. De qualquer forma, eu considero um avanço 👍 |
|
Ah @paulo-santana nessa branch eu fiz o merge também da sua inserção no README 👍 |
|
Se funciona, não vejo por que não utilizar dessa forma. Melhor do que não ter ou ter que ficar buscando outras soluções. Inclusive, se for confirmar que vai continuar com essa, pode apagar o PR #50, ou avisa que eu apago. No mais, acho que trabalhar dessa forma vai precisar de algumas instruções no README.md ou mesmo em um CONTRIBUTING.md. Posso fazer um, se preferir |
Cobre o primeiro dos itens sugeridos pelo @filipedeschamps no pr #29, adicionando semântica aos status code em caso de erros no endpoint
/cep/v1.Um problema que eu notei é que a API do cep-promise aceita letras como valor. Mesmo sabendo que é extremamente improvável que alguém vá fazer uma requisição dessas, isso não é um caso que deveria ser tratado? Usar letras aqui gera um
"service_error", porque o cep-promise ainda envia as requisições e os serviços dizem que não encontraram nada. O correto seria, na minha visão, retornar também um"validation_error"pra esse caso.Como a BrasilAPI apenas captura as exceções lançadas pelo cep-promise e não faz validação, eu achei melhor não mexer nisso aqui agora. Acho que a mudança, se ela for mesmo absolutamente necessária, teria que ser feita direto na lib