-
Notifications
You must be signed in to change notification settings - Fork 11
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
refactor: add sentry dsn rotation logic #466
Conversation
await this.updateConfigFile({ | ||
...configFile, | ||
updatedAt: new Date() | ||
}); |
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.
Seems this is redundant. Should we update the configuration as though the rotation was successful?
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.
I think yes, just because if we don't do that, it will try to perform rotation on every bright-cli
call causing at most 1.5 seconds delay.
Since rotation logic is not crucial for the cli for now, I think it's ok in case of failure to repeat it later.
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.
As you wish. However, I'm still uncertain about how it is supposed to work for long-running processes like the repeater which is intended to work for months.
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.
Let's have a separate PR for the background rotation
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.
@lsndr please open a ticket in Jira
relates-to: #458