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

Adiciona o pREST como uma possibilidade de API ao serviço #168

Merged
merged 13 commits into from
Dec 19, 2022

Conversation

vmesel
Copy link
Contributor

@vmesel vmesel commented Dec 2, 2022

Esse PR adiciona o pREST como uma solução da issue #167, permitindo uma a utilização da ferramenta para configuração de queries.

docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
Copy link
Owner

@cuducos cuducos left a comment

Choose a reason for hiding this comment

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

Muitas dúvidas de novato.

Mas marquei o request changes pelo comentário do depend_on no serviço da Minha Receita. É um erro corriqueiro que temos que evitar.

Comment on lines 12 to 14
depends_on:
postgres:
condition: service_healthy
Copy link
Owner

Choose a reason for hiding this comment

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

Eu não incluiria isso, como justificado em outro PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cuducos se o banco não tiver subido antes do pREST, ele falha na conexão. Como que a gente faz sem condition?

Copy link
Owner

Choose a reason for hiding this comment

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

Se o banco tem que subir antes do serviço prest esse bloco tem que estar no serviço prest e não no serviço minha-receita (e ele já está, linhas 43-45).

@@ -32,3 +35,29 @@ services:
- 5555:5432
environment: *credentials
command: ["postgres", "-c", "log_statement=all"]

prest:
image: prest/prest:latest
Copy link
Owner

Choose a reason for hiding this comment

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

Não seria melhor (no sentido de previsibilidade e reprodutibilidade) especificar uma versão?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seria!

docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
prest/config/prest.toml Outdated Show resolved Hide resolved
prest/config/prest.toml Outdated Show resolved Hide resolved
Co-authored-by: Eduardo Cuducos <[email protected]>
@vmesel vmesel requested review from cuducos and avelino and removed request for cuducos December 2, 2022 14:33
prest/config/prest.toml Outdated Show resolved Hide resolved
api/config/prest.toml Outdated Show resolved Hide resolved
@vmesel vmesel requested review from cuducos and removed request for avelino December 16, 2022 13:46
Copy link
Owner

@cuducos cuducos left a comment

Choose a reason for hiding this comment

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

Acho que está ótimo, apenas estou coordenando com o @vmesel de a gente documentar o que foi feito nesse PR (talvez no CONTRIBUTIUNG.md) e depois usar a #167 para criar uma lista de próximos passos.

Vini, se quiser pode fazer um squash dos commits, senão faço na hora do merge.

O que gostaria de documentar antes do merge?

Qualquer coisa que responda algumas perguntas como:

  • Agora preciso rodar o serviço pREST para a Minha Receita funcionar?
  • Para q serve o pREST no Minha Receita?
  • Como eu rodo o pREST sem ser por Docker?
  • Como eu uso o Minha Receita pelo pREST não pela API antiga?

@cuducos cuducos changed the base branch from main to prest December 19, 2022 00:58
@cuducos cuducos merged commit 1bc747f into cuducos:prest Dec 19, 2022
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.

3 participants