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

Strip ANSI color codes from notifications #2508

Merged
merged 6 commits into from
Aug 29, 2023

Conversation

costasd
Copy link
Contributor

@costasd costasd commented Aug 11, 2023

Not very long ago, ansi color support was enabled in dnscontrol, to ease reviewing of changesets.

This doesn't play well with notifications, as they usually can't render these codes; slack for example outputs the escape codes, leading to ugly and hard to grasp messages. See #2499 for a few examples.

This changeset ensures that any message coming to a notifier will get stripped of ansi color codes, before passed on to the notifier.

That way, notifiers are then free to re-add colors in a way that is supported in the relevant platform.

Not very long ago, ansi color support was enabled in dnscontrol, to
ease reviewing of changesets.

This doesn't play well with notifications, as they usually can't
render these codes; slack for example outputs the escape codes,
leading to ugly and hard to grasp messages.

This changeset ensures that any message coming to a notifier will
get stripped of ansi color codes, before passed on to the notifier.

That way, notifiers are then free to re-add colors in a way that is
supported in the relevant platform.
@cafferata
Copy link
Collaborator

Looks good and works as I would expect.

@tlimoncelli tlimoncelli changed the title Disable ansi colors in notifications Strip ANSI color codes from notifications Aug 17, 2023
@costasd
Copy link
Contributor Author

costasd commented Aug 25, 2023

should be good now.

@tlimoncelli tlimoncelli merged commit 8bf996b into StackExchange:master Aug 29, 2023
2 checks passed
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.

None yet

3 participants