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

Usar variáveis de ambiente no docker-compose.development.yml #1602

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

Rafatcb
Copy link
Collaborator

@Rafatcb Rafatcb commented Jan 13, 2024

Mudanças realizadas

Tive um problema ao rodar os serviços já tendo a porta do SMTP ocupada, mesmo alterando as variáveis de ambiente. Então percebi que as variáveis de ambientes não estavam sendo usadas no infra/docker-compose.development.yml.

  • Estou usando strings explícitas como valores para as portas, conforme sugerido na documentação.
  • Estou usando o parâmetro --env-file pelo CLI. Tentei usar o env_file no .yml, mas não estava funcionando ("The "EMAIL_SMTP_PORT" variable is not set. Defaulting to a blank string."). Pelo o que entendi, o env_file passa as variáveis de ambiente para o container, e não para ser usada no próprio yml.
  • Aproveitei para usar o npm run services:up no .github/workflows/ci.yml, já que era o mesmo comando do package.json.

Testei localmente no Linux e Windows, também testei no Codespaces e Gitpod. Não precisei mudar a configuração .gitpod.yml para funcionar, mas não sei porquê.

Tipo de mudança

  • Correção de bug

@Rafatcb Rafatcb added infra Mudanças relacionadas à infraestrutura bug Comportamento diferente do esperado labels Jan 13, 2024
Copy link

vercel bot commented Jan 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tabnews ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 13, 2024 6:31pm

Copy link
Contributor

@kaique-soares kaique-soares left a comment

Choose a reason for hiding this comment

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

Muito bom @Rafatcb 🚀

Fiz 2 comentários, veja se faz sentido 🤝

Além do que comentei, você acha que compensa usar a kill-port para liberar a porta para o mailcatcher também, assim como fazemos com a porta 3000 ao rodar npm run dev?

@@ -32,7 +32,7 @@ jobs:

- name: Stop containers
if: always()
run: docker-compose -f infra/docker-compose.development.yml down
run: docker-compose --env-file .env -f infra/docker-compose.development.yml down
Copy link
Contributor

Choose a reason for hiding this comment

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

@Rafatcb, você acha que vale a pena criar um script services:down no package.json? Desta forma deixamos tudo centralizado.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Havia pensado nisso também. Como agora somos dois com a mesma ideia, vou colocar 👍 .

Sabe alguma forma simples de deixar os scripts mais limpos? Fiz alguns experimentos mas não consegui. Queria evitar precisar repetir todo o --env-file .env -f infra/docker-compose.development.yml, duas vezes por comando (seis vezes no total).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sabe alguma forma simples de deixar os scripts mais limpos?

Você diz fazer assim?

"services:up": "npm run docker:compose -- up -d",
"services:stop": "npm run docker:compose -- stop",
"services:down": "npm run docker:compose -- down",
"docker:compose": "docker compose --env-file .env -f infra/docker-compose.development.yml",

Isso funciona, mas também removi a opção de usar o alias docker-compose (mais detalhes em #1315), o que acredito que já podemos fazer, mas então precisa adequar as dependências globais no README. Não sei quais seriam as novas versões mínimas.

Se acharem que vale a pena a mudança, só precisamos ver quais versões colocar lá. E nem precisa ser a mais antiga compatível, mas também não precisamos recomendar as últimas versões.


Outro trecho que talvez compense unificar, pois se repete 3 vezes, é:

kill-port 3000 && npm run services:up && npm run migration:run && npm run migration:seed

Que nome será que faria sentido para um script com o trecho acima?


Também seria legal resolver o caso do && npm run services:stop que nunca é executado após forçar o encerramento do NextJS.

No Windows até resolve se mudar o script next para "next": "next dev || exit 0", pois ao usar o Ctrl+C é perguntado quais processo encerrar, e dá pra continuar o script. Mas no Linux sempre encerra tudo e não roda o services:stop

Alguma ideia do que fazer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Você diz fazer assim?

Era isso mesmo, @aprendendofelipe . Não lembrava que era só passar o --.

Que nome será que faria sentido para um script com o trecho acima?

Não consegui pensar num bom nome. Talvez por serem usados antes dos comandos next dev, next build e next start pudesse ser algo como next:prepare. Ou separar em dois comandos (podem ter outro nome):

"next:prepare": "kill-port 3000 && npm run services:up",
"db:prepare": "npm run migration:run && npm run migration:seed"

Alguma ideia do que fazer?

Dei uma pesquisada, mas a única sugestão de solução que vi era usando || mesmo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dei uma pesquisada, mas a única sugestão de solução que vi era usando || mesmo.

Isso sim, mas a dúvida é mais sobre alguma alternativa ao Ctrl+C no Linux, pois ele encerra tudo e não roda o que tem depois do ||.

Com a alteração do commit abaixo já está rodando o services:stop após os testes locais (Windows e Linux). 🎉

a0ccc4b

Depois vou fazer mais testes antes de abrir o PR, e ver se dá para organizar mais. Se bem que organização, nesse caso, é muito mais uma questão de opinião, que serão bem vindas. 💪

infra/docker-compose.development.yml Show resolved Hide resolved
@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Jan 13, 2024

@kaique-soares obrigado pelas sugestões 😄

Além do que comentei, você acha que compensa usar a kill-port para liberar a porta para o mailcatcher também, assim como fazemos com a porta 3000 ao rodar npm run dev?

Acho que não. Conseguimos usar o kill-port 3000 porque é a porta padrão usada pelo next dev, já que não especificamos nenhuma ali. Inclusive, temos a variável de ambiente NEXT_PUBLIC_WEBSERVER_PORT que é usada para conseguir enviar o link de ativação no e-mail, por exemplo, mas que não é necessariamente a porta em que está executando o serviço.

Para usar as variáveis do .env nos scripts do package.json, me parece que poderíamos usar a biblioteca env-cmd. Eu nunca usei ela, apenas vi a indicação pelo repositório do cross-env (que eu já usei).

Dependendo do que o desenvolvedor tem na máquina, o comando kill-port é até invasivo 😅. Ele foi adicionado por causa do issue #274. Não sei se é legal ter ele finalizando outros processos.

The environment variables were not being considered when defining the `HOST_PORT` of the containers
in the docker-compose.development.yml file.
Copy link
Collaborator

@aprendendofelipe aprendendofelipe left a comment

Choose a reason for hiding this comment

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

Bora pro merge? Ou vai fazer algo mais?

@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Jan 13, 2024

Bora pro merge? Ou vai fazer algo mais?

Creio que esteja tudo certo 👍

@aprendendofelipe aprendendofelipe merged commit cbcc71c into main Jan 13, 2024
7 checks passed
@aprendendofelipe aprendendofelipe deleted the use-port-from-env-on-docker branch January 13, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Comportamento diferente do esperado infra Mudanças relacionadas à infraestrutura
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants