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
Chore/sentry #476
Chore/sentry #476
Changes from 14 commits
bba87e2
d2a1828
639dc8f
df66af1
4ab0c3f
13e07d6
5916549
db5eea2
ee0e546
f6ac30f
9d4f2b3
bf9c742
66cef63
9dec219
c680fcc
3748e2c
273bc2e
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.
hostname.includes()
не очень безопасно.В идеале фронт не должен знать где он запускается, и такие настройки получать либо зашитыми при билде, либо запросом в бэк.
Но эт сложна, так что предлагаю тут себя обезопасить:
includes
использоватьstartsWith
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.
тому есть причина: опенсорсность, 1. мы не знаем как у себя будут разворачивать мойру люди извне(с какими хост именами), а .includes дает хоть какой-то универсальности 2. тут просто нельзя добавить список наших хостов)
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.
Так как в сентри вы настроили белый список доменов, и там только контуровские,
то значит от развёрнутых извне мойр вы не хотите получать ошибки.
Поэтому:
Вообще плевать на другие мойры
Не вижу причин почему нельзя.
А ещё определение платформы ты затащил чисто под сентри который мы используем чисто под контуровскую мойру.
Поэтому предлагаю такие изменения:
сменить нейминг
platform
- >sentryKonturPlatform
илиkonturPlatform
Оставить комментарий, что сентри работает только для контуровских доменов и не влияет на самостоятельно развёрнутые мойры
Всё таки в енам запихнуть полные хостнеймы и вместо
includes()
использоватьstartsWith()
или даже===
(можно ещё оставить коммент с ссылкой на настройки мойры, где эти домены прописаны, но это необязательно)
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.
не соглашусь.
а по поводу окружения, пока предлагаю оставить так и завести задачу на добавить к возвращаемому dsn еще и площадку, что бы можно было и сконфигурировать это дело и не хранить инфу о площадке на фронте
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.
Если данные о платформе утащите на бэк, будет прям огонь
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.
заведи сначала тасочку с пожелахами и че куда как делать, а там разберемся)