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

Removed use of # from the PR number #1249

Merged

Conversation

vaisakhkannan
Copy link
Contributor

@vaisakhkannan vaisakhkannan commented Jan 30, 2025

Fixes #1244

Removed use of Special character # from the PR number for the slack message

With the latest changes in this PR there is no issue with the text from GitHub (title of the PR), and it include '#' or '@' characters without any problems.

Screenshot 2025-02-10 at 2 50 06 PM

mrglavas
mrglavas previously approved these changes Jan 30, 2025
Copy link
Contributor

@mrglavas mrglavas left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks.

@turkeylurkey
Copy link
Member

In the screen shot you provide in this pull request you show sample text that reads "#749." It is not replaced with "private channel" possibly because our instance of Slack does not have a channel named 749 whereas it did seem to have a channel named "777" or some other number. So this example does not seem to prove there is no problem.

Can you try a test where you use a string from a github issue you open or perhaps you use your own dummy string that reads the same as a known existing channel?

@anusreelakshmi934
Copy link
Contributor

@vaisakhkannan Can we completely avoid using the # sign? As Paul mentioned, there's a chance that another PR number might have a channel with that format, which could cause an issue.

@vaisakhkannan
Copy link
Contributor Author

vaisakhkannan commented Feb 10, 2025

@anusreelakshmi934 , @turkeylurkey
I was able to reproduce the issue in my fork runs that you mentioned regarding the existing channel name (e.g., issue number 778).

Screenshot 2025-02-10 at 2 53 26 PM

I found a fix today and tested it with the latest changes, and it is working as expected for the '#' case. However, for the other special character '@' case, I couldn't identify the problem. I have added Slack IDs starting with '@' in the dummy PRs, and they are displaying correctly as expected. Below is the screenshot including the fix of # usages.

Screenshot 2025-02-10 at 2 49 53 PM Screenshot 2025-02-10 at 2 50 06 PM

Copy link
Contributor

@mrglavas mrglavas left a comment

Choose a reason for hiding this comment

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

Approving again.

Copy link
Contributor

@anusreelakshmi934 anusreelakshmi934 left a comment

Choose a reason for hiding this comment

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

looks good

@vaisakhkannan vaisakhkannan merged commit 62fcc6f into OpenLiberty:main Feb 13, 2025
4 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.

Avoid use of special characters in Slack reports
4 participants