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

ECS-01 feat: cria cluster ecs, service e task definition #2

Merged
merged 24 commits into from
Aug 10, 2022

Conversation

EzzioMoreira
Copy link
Contributor

@EzzioMoreira EzzioMoreira commented Mar 17, 2022

Criar cluster ecs, service e task definition.

Modulo que cria cluster ECS para projetos Dados abertos de feira e mtst.

  • Garanta que seu topic/feature/bugfix branch tenha uma branch nomeada e não a sua branch main esteja no PR
  • Dê um titulo que expresse o objetivo do PR
  • Associe seu PR a uma Issue criada no repositósito. Caso seja uma correção de linguagem ou pequenas correções, não é necessário
  • Descreva o objetivo do PR
  • Inclua links relevantes para a sua modificação/sugestão/correção
  • Descreva um passo-a-passo para testar o seu PR

Issue

issue#1

Objetivo

Criar um cluster para os projetos Dados abertos feira e MTST.

Referências

Como testar

terraform init
terraform plan
terraform apply

@afonsoaugusto
Copy link
Member

Show demais o pr @EzzioMoreira e @chnacib.
Parabéns a todos os envolvidos

@edsoncelio
Copy link

Boa! Tá ficando bem bom!

Só acrescentando alguns pontos:

  • Dar uma olhada nas falhas do tfsec (se alguma delas não fizer sentido, vale ignorar by código).
  • Um dos requisitos na demanda original é a capacidade de uso de variáveis de ambiente vindas de algum serviço de gestão de segredos da AWS (secret manager ou parameter store por ex), só pra dar uma ideia, deixando aqui um exemplo.
  • (outro ponto que é mais uma sugestão) usar alguma tool pra adicionar a linha no final dos arquivos, o editorconfig faz isso bem demais :D

Qualquer dúvida sobre qualquer um dos pontos, só chamar lá no discord :D

@gomex gomex marked this pull request as draft March 29, 2022 11:53
@edsoncelio edsoncelio changed the title ECS-01 feat: cria cluster ecs, service e teask definition ECS-01 feat: cria cluster ecs, service e task definition Apr 2, 2022
@edsoncelio
Copy link

cc @EzzioMoreira mais alguns checks do tfsec pra dar uma olhada :)

Copy link
Member

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Tá ficando legal, ótimo trabalho! Deixei uns comentários de melhorias e algumas dúvidas, mas nada de muito problemático.

Acho que só precisa definir isso do cluster, se cria dentro do módulo ou não. Se for deixar opcional como está, eu acho que precisa ajustar os resources que dependem do cluster para serem opcionais também. Ficaria tipo assim:

locals {
  cluster_count = var.create_cluster ? 1 :0
}

 resource "aws_appautoscaling_target" "ecs_target" {
  count = locals.cluster_count
  # ...
}

 resource "aws_ecs_service" "service_cluster" {
  count = local.cluster_count
  # ...
}

Do jeito que está eu acho que daria erro se o cluster não for criado?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
service.tf Outdated Show resolved Hide resolved
sg.tf Outdated Show resolved Hide resolved
sg.tf Outdated Show resolved Hide resolved
versions.tf Outdated Show resolved Hide resolved
local.tf Outdated Show resolved Hide resolved
@chnacib
Copy link
Contributor

chnacib commented Apr 30, 2022

Refatorado para ter um Produto Viável Mínimo de acordo com a demanda original.

Alterações:

  • Load Balancer apenas na API
  • Variável para template de cada container(imagem,nome)
  • Variável para portas de cada container
  • 1 Task-definition apenas para 3 containers rodando pois precisam se conectar entre si internamente (segundo o Gomex, o tika ainda não haverá autenticação)

autoscaling.tf Outdated Show resolved Hide resolved
autoscaling.tf Outdated Show resolved Hide resolved
autoscaling.tf Outdated Show resolved Hide resolved
@ghost ghost force-pushed the feat/cria-cluster-ecs branch from b544cdc to b09a2d1 Compare May 1, 2022 15:22
@EzzioMoreira
Copy link
Contributor Author

@chnacib Faz um fork do projeto da mentoria e cria uma nova brach para refatoração. 🤘
#2 (comment)

@chnacib chnacib force-pushed the feat/cria-cluster-ecs branch from b09a2d1 to 339e941 Compare May 1, 2022 17:58
@chnacib
Copy link
Contributor

chnacib commented May 2, 2022

MVP finalizado. Aguardando Review

@gomex gomex marked this pull request as ready for review May 15, 2022 17:30
Copy link
Contributor

@gomex gomex left a comment

Choose a reason for hiding this comment

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

O PR está muito bom, mas esses secrets precisam necessariamente serem removidos do módulo de ECS. Essa lista precisa ser informada no lugar onde chama o módulo, que no caso atual será o repositório mariaquiteria.

cpu = var.container_cpu
memory = var.container_memory
essential = true
secrets = [{
Copy link
Contributor

Choose a reason for hiding this comment

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

Esses secrets devem ser apresentados pelo uso do módulo. Essas variáveis não podem ser hardcoded dentro do módulo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Vc pode pegar uma variável que tem todas as informações que precisam ser passadas pelo usuário.

Eu não tentei ainda, mas algo mais ou menos assim:

secrets = var.secrets

E no input do secrets no modulo root (O que chama esse módulo) seria algo mais ou menos assim:

[     
        {
        "name" : "DJANGO_SECRET_KEY",
        "valueFrom" : data.aws_ssm_parameter.main["django_secret_key"].arn
        },
        {
        "name" : "DJANGO_SETTINGS_MODULE",
        "valueFrom" : data.aws_ssm_parameter.main["django_settings_module"].arn
        },
...
]   

@gomex gomex merged commit ebfe0d6 into mentoriaiac:main Aug 10, 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.

6 participants