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

release/38851105: Validation server side and docker-compose #2

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

UnicoYal
Copy link
Collaborator

@UnicoYal UnicoYal commented Sep 17, 2024

Задача №38851105

Что было не так:

Не было сервиса валидации и docker-compose файла

Что было сделано:

  • слои api и service;
  • GRPC-сервер;
  • Базовая валидация почты, пароля, дня рождения и пола;
  • Пока не проверяем подтверждение пароля, фронты не будут готовы к первому рк;
  • Прото-контракт;
  • docker-compose.yml.

Чек лист до ревью :

  • PR разбит на логические коммиты (если нет, то нужно разбить);
  • PR не содержит секретов (пароли, токены, ключи);
  • PR не содержит лишних файлов (например, .env);
  • PR не содержит коммитов, которые не относятся к задаче;

Перед тем, как отдать на ревью нужно убедиться, что все пункты выполнены

как вливать ветку

Если это обычная задача, то просто squash merge в develop
Если это хотфикс, то обычный merge в master и develop (должно быть два pull request). При этом в ветке должен быть один коммит
Если это релиз, то обычный merge и в master и в develop. Там будет несколько коммитов

@UnicoYal UnicoYal force-pushed the release/38851105-validation-service branch from acbb3e3 to 90656be Compare September 18, 2024 10:44
@UnicoYal UnicoYal force-pushed the release/38851105-validation-service branch from 90656be to e347106 Compare September 18, 2024 11:56
Copy link
Collaborator

@Starlexxx Starlexxx left a comment

Choose a reason for hiding this comment

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

пока очень скомкано выглядит, на валидацию точно надо добавить тесты

@@ -0,0 +1,44 @@
version: '3'
Copy link
Collaborator

Choose a reason for hiding this comment

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

deprecated

image: 'redis:latest'
command: redis-server
ports:
- '6379:6379'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Если у вас сеть, то порты по-моему не обязательно пробрасывать, вы в коде будете ходить по названию контейнера, зато если вы фаерволом не закроете порт, то через пару часов посмотрите, что будет в редиске храниться, я в свое время угарнул)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Оставлю только для сервиса, чтобы из постмана можно было долбить

"github.com/go-park-mail-ru/2024_2_GOATS/validation-service/internal/app"
)

const serverPort = 5050
Copy link
Collaborator

Choose a reason for hiding this comment

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

в конфиг

desc "github.com/go-park-mail-ru/2024_2_GOATS/validation-service/internal/pb/validation"
)

type App struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

хочу видеть сущность конфиг, где будет listener, он из ямлика будет читать адрес и порт. Рут контекст будет обогощаться этим конфигом, потом вы из него сможете достать конфиг где угодно

Copy link
Collaborator Author

@UnicoYal UnicoYal Sep 20, 2024

Choose a reason for hiding this comment

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

Что-то типо?

type Config struct {
	listenerAddress string `yaml:"listener_address"`
	listenerPort    int    `yaml:"listener_port"`
}

type App struct {
	serverPort        int
	validationService validationAPI.ValidationService
	config            Config
}

В конструкторе App читаю конфиг из ямла и добавляю в контекст
Пока у меня контекст не прокидывается вниз, надо ли его передавать в конструкторы слоев?

package errors

var (
ErrInvalidEmailCode = "invalid_email"
Copy link
Collaborator

Choose a reason for hiding this comment

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

А почему не сделать обычные ошибки через errors.New

Email string
Password string
PasswordConfirm string
Sex string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Даже если вы собираетесь поддерживать 50+ гендеров, как нетфликс, string это расточительство

@@ -0,0 +1,46 @@
package validation
Copy link
Collaborator

Choose a reason for hiding this comment

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

в названии файла очепятка


var (
emailRegex = regexp.MustCompile("^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$")
passwordLength = 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

кажется, это константа должна быть

desc "github.com/go-park-mail-ru/2024_2_GOATS/validation-service/internal/pb/validation"
)

func (i *Implementation) ValidateRegistration(ctx context.Context, req *desc.ValidateRegistrationRequest) (*desc.ValidationResponse, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Вот пока не знаю, как избавиться от контекста в аргументах функции. Типо это же из pb файла такой интерфейс
И вообще надо ли избавляться?

@UnicoYal UnicoYal force-pushed the release/38851105-validation-service branch from 9c9a019 to 57c3621 Compare September 21, 2024 09:47
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