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

Chore/sentry #476

Merged
merged 17 commits into from
Jan 22, 2024
Merged

Chore/sentry #476

merged 17 commits into from
Jan 22, 2024

Conversation

EduardZaydler
Copy link
Member

@EduardZaydler EduardZaydler commented Jan 12, 2024

PR Summary

getting dsn key via api, added get dsn function,
added getPlatform func to identify current platform

@EduardZaydler EduardZaydler requested a review from a team as a code owner January 12, 2024 12:36
@EduardZaydler
Copy link
Member Author

/build

@EduardZaydler
Copy link
Member Author

/build

src/app.tsx Outdated Show resolved Hide resolved
src/app.tsx Outdated Show resolved Hide resolved
Comment on lines 14 to 23
export enum Platform {
LOCAL = "local",
DEV = "dev",
STAGING = "staging",
PROD = "prod",
}

const isPlatform = (platform: Platform): boolean => {
return window.location.hostname.includes(platform);
};
Copy link

Choose a reason for hiding this comment

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

hostname.includes() не очень безопасно.

В идеале фронт не должен знать где он запускается, и такие настройки получать либо зашитыми при билде, либо запросом в бэк.

Но эт сложна, так что предлагаю тут себя обезопасить:

  1. вместо includes использовать startsWith
  2. в enum Platform прописать полные hostname'ы

Copy link
Member Author

Choose a reason for hiding this comment

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

тому есть причина: опенсорсность, 1. мы не знаем как у себя будут разворачивать мойру люди извне(с какими хост именами), а .includes дает хоть какой-то универсальности 2. тут просто нельзя добавить список наших хостов)

Copy link

Choose a reason for hiding this comment

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

Так как в сентри вы настроили белый список доменов, и там только контуровские,
то значит от развёрнутых извне мойр вы не хотите получать ошибки.

Поэтому:

  1. мы не знаем как у себя будут разворачивать мойру люди извне(с какими хост именами)

Вообще плевать на другие мойры

  1. тут просто нельзя добавить список наших хостов)

Не вижу причин почему нельзя.

А ещё определение платформы ты затащил чисто под сентри который мы используем чисто под контуровскую мойру.

Поэтому предлагаю такие изменения:

  1. сменить нейминг platform - > sentryKonturPlatform или konturPlatform

  2. Оставить комментарий, что сентри работает только для контуровских доменов и не влияет на самостоятельно развёрнутые мойры

  3. Всё таки в енам запихнуть полные хостнеймы и вместо includes() использовать startsWith() или даже ===
    (можно ещё оставить коммент с ссылкой на настройки мойры, где эти домены прописаны, но это необязательно)

Copy link
Member

Choose a reason for hiding this comment

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

на правах крокодила и прочитавшего по диагонали: если что можно утащить в конфиги ж

Copy link
Member Author

@EduardZaydler EduardZaydler Jan 17, 2024

Choose a reason for hiding this comment

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

Вообще плевать на другие мойры

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

Copy link
Member

Choose a reason for hiding this comment

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

так мб сразу?
нам одну строчку у себя добавить как я поняла надо на бэке

Copy link

Choose a reason for hiding this comment

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

Если данные о платформе утащите на бэк, будет прям огонь

Copy link
Member Author

Choose a reason for hiding this comment

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

будем Даню ждать или кто-нибудь из ребят сделает?

Copy link
Member

Choose a reason for hiding this comment

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

заведи сначала тасочку с пожелахами и че куда как делать, а там разберемся)

src/helpers/common.ts Outdated Show resolved Hide resolved
sol-un
sol-un previously approved these changes Jan 17, 2024
src/app.tsx Outdated Show resolved Hide resolved
GreoGi
GreoGi previously approved these changes Jan 17, 2024
FonDumik
FonDumik previously approved these changes Jan 18, 2024
@EduardZaydler EduardZaydler dismissed stale reviews from FonDumik and GreoGi via 3748e2c January 22, 2024 07:39
@EduardZaydler
Copy link
Member Author

/build

Copy link

github-actions bot commented Jan 22, 2024

Build and push Docker images with tag: 2024-01-22.17d714c

@EduardZaydler EduardZaydler merged commit 17d714c into master Jan 22, 2024
1 check passed
@EduardZaydler EduardZaydler deleted the chore/sentry branch January 22, 2024 10:46
@EduardZaydler EduardZaydler restored the chore/sentry branch January 22, 2024 15:30
@EduardZaydler EduardZaydler deleted the chore/sentry branch January 22, 2024 15:31
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.

5 participants