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

allow apps to not include the user in a notification #698

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

agrobbin
Copy link
Contributor

While some may find the current OS user helpful, in other contexts, that user is randomly assigned and not material to the notification itself. Consider a deployment enviroment like Heroku, which will use a different OS user for each deployable, and each deploy.

It may also cause grouping issues in services like Sentry, which use the exception / notification content to determine whether the situation has occurred already.

This does not change the default behavior, but rather introduces a new option to skip including that information in notifications, for those who want to.

@agrobbin agrobbin force-pushed the skip-user-in-notification-data branch from 905352b to 92f9f3b Compare January 15, 2024 01:27
@flyerhzm
Copy link
Owner

@agrobbin I'd like to merge the PR, but it breaks the CI. Please fix it first.

While some may find the current OS user helpful, in other contexts, that user is randomly assigned and not material to the notification itself. Consider a deployment enviroment like Heroku, which will use a different OS user for each deployable, and each deploy.

It may also cause grouping issues in services like Sentry, which use the exception / notification content to determine whether the situation has occurred already.

This does not change the default behavior, but rather introduces a new option to skip including that information in notifications, for those who want to.
@agrobbin agrobbin force-pushed the skip-user-in-notification-data branch from 92f9f3b to feb8ed1 Compare January 15, 2024 12:10
@agrobbin
Copy link
Contributor Author

@felixonmars should be fixed now!

@flyerhzm flyerhzm merged commit 0e8417a into flyerhzm:main Jan 16, 2024
10 checks passed
@agrobbin
Copy link
Contributor Author

Thanks for the quick review and merge @flyerhzm! I really appreciate it. Hopefully this is something that will be helpful for others.

I saw you just released a new version a week or so ago, but hopefully this is something that can be released soon.

@flyerhzm
Copy link
Owner

@agrobbin thank you for your contribution, I'll release it later today.

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