Add Docker development environment#45
Conversation
angeloINTJ
left a comment
There was a problem hiding this comment.
Code Review: PR #45 — Add Docker Development Environment
Obrigado pela contribuição, @JohnMartin0301! O PR está bem estruturado e atende os critérios de aceitação da issue #40. Abaixo estão os itens que precisam de ajuste antes do merge.
✅ O que está bom
- Estrutura limpa — apenas 3 arquivos, 63 linhas adicionadas, sem alterações desnecessárias
- Imagem dentro do budget — 1.66 GB (limite: 2 GB)
- Build e testes passam — screenshots comprovam
pio rune ambos os suites de teste (native+native_history) - Documentação clara — tabela de equivalência entre comandos Docker e PlatformIO é útil
- Comentários em inglês — seguindo as convenções do projeto
🔴 Bloqueantes (devem ser resolvidos antes do merge)
1. Falta .dockerignore
O build context do Docker inclui o repositório inteiro — .git/, .pio/ (se existir localmente), docs/promotion/, etc. Isso deixa o build mais lento e pode causar cache invalidation desnecessária.
Ação: Criar um .dockerignore com pelo menos:
.git/
.pio/
docs/promotion/
2. PlatformIO sem version pinning
pip install platformio instala a versão mais recente. Se uma versão futura quebrar compatibilidade, o build Docker para de funcionar sem motivo aparente.
Ação: Pin a versão: pip install --no-cache-dir platformio==6.1.x (ou a versão atual usada)
3. Plataforma Pico W sem git ref fixo
pio pkg install --global --platform "https://github.com/maxgerhardt/platform-raspberrypi.git" baixa o HEAD do branch default do repositório. Mudanças upstream podem quebrar o build a qualquer momento.
Ação: Adicionar o ref tag/commit usado, ex.:
RUN pio pkg install --global \
--platform "https://github.com/maxgerhardt/platform-raspberrypi.git#<commit-or-tag>"Use o mesmo ref que o projeto já referencia no platformio.ini.
🟡 Sugestões (recomendadas, mas não bloqueiam)
4. curl instalado mas não usado
O Dockerfile instala curl mas ele nunca é utilizado no build ou runtime. Remover economiza ~2 MB e reduz superfície.
Ação: Remover curl \ do apt-get install.
5. Artefatos de build com ownership root
No Linux, o volume mount faz com que .pio/ e outros diretórios gerados pelo container pertençam ao root. O contribuidor precisará de sudo para limpá-los depois.
Ação sugerida: Adicionar ao docker-compose.yml:
build:
build: .
user: "${UID:-1000}:${GID:-1000}"
volumes:
- ".:/workspace"E documentar export UID GID no CONTRIBUTING.md para usuários Linux.
6. Missing trailing newline no docker-compose.yml
O arquivo termina sem newline no final da última linha, o que é contra convenções POSIX e pode causar warnings em alguns editores.
7. docker-compose.yml — comentário desalinhado
# Runs both native unit test suites ← col 0
test: ← indentadoSugiro alinhar o comentário com o nível de indentação do serviço, ou movê-lo para dentro como chave comment: (suportado em algumas versões, mas o mais simples é só indentar).
🔵 Informações
- O
platformio.inido projeto referencia a plataforma sem um ref fixo também (linha 15). Isso é um problema pré-existente, não introduzido por este PR — mas seria bom alinhar o Dockerfile com o mesmo ref quando for pinado. - Considere adicionar um workflow de CI que execute
docker compose run testpara validar que a imagem continua funcional em PRs futuros (pode ser uma issue separada).
Resumo
| Categoria | Contagem |
|---|---|
| 🔴 Bloqueante | 3 |
| 🟡 Sugestão | 4 |
| 🔵 Info | 2 |
Excelente trabalho no geral — o PR está a 90% do merge. Com as 3 correções bloqueantes resolvidas, fica pronto para aprovar. 🚀
- Add .dockerignore to exclude .git/, .pio/, docs/promotion/ from Docker build context
- Pin PlatformIO Core to 6.1.19 for reproducible builds
- Pin platform-raspberrypi to commit 64c93ed (matches default branch HEAD)
- Remove unused curl from Dockerfile apt packages
- Add user mapping (${UID}:${GID}) to docker-compose services (avoids root-owned artifacts on Linux)
- Align comment indentation in docker-compose.yml
- Add trailing newline to docker-compose.yml
- Add Linux UID/GID export instructions to CONTRIBUTING.md
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
angeloINTJ
left a comment
There was a problem hiding this comment.
Merged — thanks @JohnMartin0301! Really clean work on the Docker environment. The Dockerfile is lean, the compose setup is well-structured, and the CONTRIBUTING.md integration with the command-equivalence table is a nice touch. Image size at 1.66 GB comfortably under budget and both native test suites pass — exactly what contributor onboarding needed.
I pushed a few small fixes to your branch:
- Added .dockerignore — keeps .git/ and .pio/ out of the build context
- Pinned PlatformIO Core to 6.1.19 and the platform to a known-good commit for reproducible builds
- Removed unused curl from the apt packages
- Added UID/GID user mapping so Linux users don't get root-owned build artifacts
- Minor formatting fixes: comment alignment, trailing newline
Second community contribution to SIMUT! Hope you stick around — there are more DevOps issues waiting if you are interested.
|
Just wanted to say — my apologies for the initial review being in Portuguese. That was my mistake. The appreciation message above still stands in full: really clean work, and we're genuinely grateful for the contribution. Second community contributor to SIMUT! 🚀 |
|
Hi, thank you for the review, feedback, and the fixes you added. I really appreciate the time you took to explain the improvements, I learned a lot from it. No worries about the Portuguese review and thank you for the kind words. I'm glad the contribution was useful and I'm honored to be the second community contributor to SIMUT. I'll definitely keep an eye on the repository and would be happy to contribute again in the future. |
Description
Adds a Docker-based development environment so contributors can build and test without installing PlatformIO, Python, or the ARM toolchain locally.
Dockerfile— based onpython:3.11-slim, installs PlatformIO and pre-caches the Pico W ARM toolchaindocker-compose.yml—buildandtestservicesCONTRIBUTING.md— added Docker quick start section under Development SetupVerified on clean checkout:
docker compose run build-> SUCCESS (98.4% flash, 00:14:34)docker compose run test-> 27/27 passed (native), 22/22 passed (native_history)Type of Change
Testing
Describe how you tested these changes:
pio run -e pico_w_releasepio test -e native)Checklist
Related Issues
Closes: #40