Sprint 1 · #1 — Configurar CI/CD com GitHub Actions - #10
Conversation
📋 Code Review — PR #10 — CI/CD (Issue #1)✅ Acertos
|
c4rlosfb
left a comment
There was a problem hiding this comment.
📋 Code Review — PR #10 (fix/issue-1 → master)
✅ Acertos
-
CI/CD Workflow (
.github/workflows/ci.yml) — Bem estruturado, com jobs separados delintebuild. Uso correto denpm cipara instalação determinística. Cleanup comif: always()garante remoção do container mesmo em falha. -
ESLint Config (
app/eslint.config.js) — Usa o formato flat config (ESLint v9+), com separação apropriada de globals para Node.js e browser. Regras bem escolhidas:semi,quotes,no-unused-vars,indent,prefer-const,no-var. -
Smoke Tests (
app/test/smoke.js) — Cobre endpoints críticos: GET/users, GET/metrics, POST/register(200/201 ✅ e 400 ❌). Usahttpnativo sem dependências extras. Exit code reflete resultado. Código limpo e bem organizado. -
package.json — Scripts
lintetestbem definidos.devDependenciescorretamente declaradas (eslint,@eslint/js). -
.gitignore— Abrangente e bem organizado: node_modules, IDE, env, Docker, logs. -
Whitespace cleanup — Boa prática limpar trailing spaces e garantir newline final (
app/app.js,app/cpu-worker.js). -
README.md— Badges de CI e Last Commit agregam visibilidade ao projeto.
🚨 Erros
-
❌ CONFLITO COM MASTER (mergeStateStatus: DIRTY) — O PR não pode ser mergeado no estado atual:
.gitignore— Conflito add/add (ambas branches adicionaram arquivo idêntico; trivial de resolver, mas existe).app/app.js— Conflito de conteúdo:fix/issue-1alterou whitespace (remoção de trailing spaces);masteradicionou imports decatalog,checkoutemetricsno mesmo bloco. Precisa de resolução manual com rebase/merge do master.- Necessário: Fazer
git rebase masterougit merge masterna branchfix/issue-1e resolver os conflitos antes de novo push.
-
❌ Node.js versão 24 no CI — Node 24 não é LTS (atual LTS é 22). Pode causar instabilidade ou incompatibilidade com pacotes npm. Use
node-version: 22(LTS).
⚠️ Warnings
-
⚠️ ESLint apenaswarn— Todas as regras estão configuradas comowarn, o que não bloqueia o build no CI. Considere promover regras críticas paraerror:no-unused-vars,no-var,prefer-const. -
⚠️ Smoke test no CI usasleep 3— Frágil. Se o container demorar mais para iniciar (runners lentos), o teste falha. Usecurl --retry 5 --retry-delay 2 --retry-connrefusedou um loopuntil. -
⚠️ npm testnão roda no CI — O scripttestdefinido nopackage.jsonexecutanode test/smoke.jsdiretamente contra a app rodando. O CI faz um smoke test via curl no Docker, que é diferente. Considere adicionar um job que executenpm testcontra o container também. -
⚠️ Bufferno smoke test — O smoke test usaBuffer.byteLength()que depende do globalBuffer. Funciona no Node.js, mas verifique se não vai depreciar em versões futuras.
💡 Sugestões
- 💡 Adicionar
lint:fix— Script"lint:fix": "eslint . --fix"para auto-correção. - 💡 Adicionar
npm testao CI — Um terceiro job no workflow para rodarnpm testcontra o container Docker. - 💡 Retry loop no lugar de
sleep 3— Exemplo:- name: Smoke test run: | docker run -d --name app-smoke -p 3001:3001 observabilidade-app:ci for i in $(seq 1 10); do curl -sf http://localhost:3001/users && break sleep 2 done
- 💡
node-version: 22(LTS) — Mais estável e recomendado para produção. - 💡 Considerar
matrixstrategy — Testar em múltiplas versões do Node (20, 22) para garantir compatibilidade. - 💡 Adicionar
paths-ignoreno CI — Ex: ignorar mudanças só no README para não disparar workflow desnecessariamente.
🏷️ Veredito
🚨 REQUEST CHANGES — O PR agrega ótimo valor (CI/CD, linting, smoke tests), mas não está mergeável no estado atual devido aos conflitos com master. Após rebase/merge com resolução de conflitos e correção da versão do Node no CI, estará pronto para aprovação.
Resumo das ações necessárias:
- 🔴 Fazer rebase/merge do
masternafix/issue-1e resolver conflitos em.gitignoreeapp/app.js - 🟡 Alterar
node-version: 24→22(LTS) noci.yml - 🟡 (Opcional) Substituir
sleep 3por retry loop no CI - 🟢 (Sugestão) Promover regras ESLint críticas para
error
|
Correções aplicadas com base na review (@c4rlosfb): ✅ Conflitos resolvidos — Branch recriada a partir do Aguardando re-review. 🚀 |
|
Correções aplicadas:
Pronto para merge. @KauaN-png |
Resolve a issue #1. Alterações realizadas:
.github/workflows/ci.yml — Pipeline CI/CD com 3 jobs:
npm cie cache de dependênciasdocker compose build) + validação (docker compose config)docker compose down -v. Smoke tests executam com-f(falha rápido em erro)app/eslint.config.js — Flat config ESLint v9.x com:
@eslint/js)error:no-unused-vars,no-var,prefer-constno-console: off (permitido para logs)public/*enode_modules/*app/package.json —
@eslint/jseeslintcomo devDependencies + scriptlintREADME.md — Badge de status do CI/CD Pipeline