-
Notifications
You must be signed in to change notification settings - Fork 212
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
Prevent < and > in commit titles from breaking dag-sync slack message #2837
Conversation
dag-sync.sh
Outdated
@@ -30,6 +30,10 @@ have_dag=$(git log -p -1 "$new" --pretty=format: --name-only | grep "catalog/dag | |||
|
|||
# Pull out the subject from the new commit | |||
subject=$(git log -1 --format='%s') | |||
# Swap the < & > characters for more complex unicode variants so they don't get | |||
# interpreted as delimiters by Slack | |||
subject=${subject//>/≻} |
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.
Why not convert them to an HTML entity as described in the docs1?
- Replace the less-than sign, < with
<
- Replace the greater-than sign, > with
>
Footnotes
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.
That is a much better idea! I'll give that a shot 😁
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.
This is creative, but @obulat's solution of using <
and >
seems more "right".
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.
LGTM! Thanks for the screenshot showing that it works.
subject=${subject//>/>} | ||
subject=${subject//</<} |
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 would've thought that the &
or ;
would need another layer of escaping.
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.
Me too, but apparently not since it's in ${...}
!
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.
Great solution 💯
Description
This PR fixes an issue that arose when #2817 was merged: the commit title
timestamp_created -> timestamp_update (#2817)
broke the Slack message component of the DAG sync since the>
character was interpreted by Slack as the end of the link definition.This change adds a step to replace the
<
and>
character with two variants (≺
and≻
respectively). These characters, though they look roughly similar, don't register in Slack as delimiters and thus don't break the message syntax when merged!Testing Instructions
This was tested in our Slack instance:
Before
After
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin