Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
FastAPI Template: Основа #3
FastAPI Template: Основа #3
Changes from 3 commits
bebbcb9
f65ad74
cb98517
cf1a29d
b1b5b5c
26f9f2f
4aad7d4
eb95e93
f47dc4b
7932032
0f1d74e
d8e0220
82db9c0
f1e5639
db1a2b0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У тебя
entrypoint
есть вDockerfile
. Насколько я понимаю, тогда здесь это не нужно.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь тоже, пожалуй, будет
- "${server_port}:${server_port}"
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я бы сделал так. Мы с Сережей научились использовать шаблоны и передавать переменные и шаблоны для nginx (пример).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я бы убрал Redis :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
На данном этапе можно убрать, но он понадобится при построении API на этапе кеширования ответов
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Подскажи зачем нам нужно две сети?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Чтобы разделять, кому нужно видеть бекенд, а кому нет. Тому же nginx не нужно видеть, что у нас есть база данных, и наоборот
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Возможно.
Только будут пограничные случаи. Например, jaeger, будет работать внутри, но один порт у него должен быть открыт наружу. То же самое с сервисами kibana итп.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Аналогичные комментарии как к dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь и далее я бы использовал прямые импорты. Например,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Интернет говорит, что должно быть
sqlalchemy.sql.func.now()
. Надо бы перепроверить в документации SQLAlchemy (она у меня сейчас нормально не открывается).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Возможно, но данная конструкция работала раньше. Если будет работать
sqlalchemy.sql.func.now()
, то добавлю еёThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если мы делаем schemas в API, то может сразу добавим версионирование
/lib/api/v1/schemas/
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не совсем понимаю смысл этого файла. Можешь объяснить, пожалуйста, и зачем нам в сервисе
app = FastAPI()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это заготовка под проверку токена при запросе к API. В одном из ревью ревьювер указал на необходимость использования авторизации по токену.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В моём понимание, это должна быть middleware. Например, здесь проверяем наличие header'а
X-Request-Id
и без него не пускаем дальше. Здесь я обновлял токен, что точно не нужно. Однако, там есть часть с проверкой самого токена (но в целом, это не самый удачный пример).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: Я бы даже вынес это в отдельный PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нам не нужно здесь инициализировать postgres клиента?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А здесь удалять postgres клиента?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нужно, не доглядел
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я бы убрал
load_dotenv
совсем. Давайте лучше укажем здесь значения по умолчанию (для локальной разработки), а в .env будут настройки для prod'а.