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

Ff 1471 #40

Open
wants to merge 7 commits into
base: production
Choose a base branch
from
Open

Ff 1471 #40

wants to merge 7 commits into from

Conversation

eversonbueno
Copy link

@eversonbueno eversonbueno commented Jan 2, 2023

[COREAPI-XXXX]

Proposed changes

Describe your changes briefly and objectively.

  • Foo
  • Bar
  • Baz

Checklist

  • Have you written tests for your changes?
  • Have you successfully run tests with your changes locally?
  • Have you lint your code locally prior to submission?

@andersoncontreira
Copy link
Contributor

Vai ser necessário deixar esse PR bem preenchido, de acordo com o template.

@andersoncontreira
Copy link
Contributor

Acredito que seja interessante adicionar alguns testes e dataprovider, em caso de dúvida me procure inbox que eu contextualizo melhor.

@andersoncontreira
Copy link
Contributor

andersoncontreira commented Jan 2, 2023

Revisar a estrutura para se adequar aos guidelines da linguagem: https://github.com/golang-standards/project-layout.
Em caso de dúvida pode me chamar inbox.

.env.example Outdated
# Configs LocalStack
LOCALSTACK_HOST=http://0.0.0.0:4566
LOCALSTACK_SQS_HOST=http://0.0.0.0:4566/000000000000/test-queue
Copy link
Contributor

Choose a reason for hiding this comment

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

Precisamos reduzir a quantidade de variaveis em relação ao localstack, por exemplo, a variável da região é uma variável global que atende a outras integrações, então não compensa existir LOCALSTACK_SQS_REGION.

Podemos ter algo nesse sentido:
LOG_LEVEL=debug
REGION_NAME=us-east-1
SQS_ENDPOINT=http://localstack:4566
SQS_LOCALSTACK=http://localstack:4566
LOCALSTACK_ENDPOINT=http://localstack:4566
APP_QUEUE=http://localstack:4566/000000000000/test-queue

Copy link
Contributor

Choose a reason for hiding this comment

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

Estuda a possibilidade de centralizar o arquivo de variáveis de ambiente na pasta /env/. assim vamos atender uma guideline da MM.
Aqui segue um exemplo: https://github.com/madeiramadeirabr/template-serverless-lambda-python/tree/production/examples/lambda_sqs/env

- 8080:8080

localstack:
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

}, nil
}

func (s SQS) Delete(ctx context.Context, queueURL, rcvHandle string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Acredito que podemos ter operações para a fila e para as mensagens, exemplo: Delete e DeleteQueue, sei que já está praticamente neste formato, talvez acrescentar a função/método faltante. Operações de deleção lembre de sempre retornar um bool para indicar o sucesso da operação.

// Long polls given amount of messages from a queue.
Receive(ctx context.Context, queueURL string) (*Message, error)
// Deletes a message from a queue.
Delete(ctx context.Context, queueURL, rcvHandle string) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Talvez acrescentar alguns sufixos como Message e Queue, exemplo:
SendMessage(), ReceiveMessage(), DeleteQueue(), DeleteMessage(), CreateQueue()

@@ -0,0 +1,20 @@
package cloud

type SendRequest struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ao meu ver, como fazem parte do contexto de SQS, me parece mais sensato estar no mesmo pacote "aws"

@@ -0,0 +1,30 @@
#!/bin/bash
# **************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Vamos precisar aplicar umas atualizações nesse carinha, me chame inbox para te ajudar nesse processo.

@@ -0,0 +1,12 @@
#!/bin/bash
# **************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Vamos precisar atualizar também.

#!/bin/bash
# **************************
# Localstack SQS Receive Messages Tool
# Version: 1.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Vamos precisar atualizar também.

@@ -0,0 +1,73 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Vamos precisar atualizar também.

@@ -1,4 +1,4 @@
#!/bin/sh
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Precisamos identificar o uso desse script, quem sabe vamos necessitar mover para a pasta ./scripts/tools

@sonarcloud
Copy link

sonarcloud bot commented Feb 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

2 participants