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

feat(slack): Support set username and icon from template in slack #340

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

ayatk
Copy link
Contributor

@ayatk ayatk commented Sep 25, 2024

When multiple users set triggers and templates for one slack service in monorepos, etc., there was an issue where the icon and username could not be changed. Therefore, I made it possible to set a custom username and icon in the template used for Slack notifications.

If the username and icon are already set on the service side and the username and icon are also set on the template, the template will take priority.

And there were no tests for Slack notifications, so I added them.

Copy link
Member

@reggie-k reggie-k left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
I have tested locally and found that in some use cases the notification is sent once with the username and icon, and once without them, meaning with the default ones, I tried that with "on-sync-succeeded" trigger.

@ayatk
Copy link
Contributor Author

ayatk commented Sep 27, 2024

@reggie-k Thank you for your review!

I'm sorry, but I don't really understand the situation. Is it okay to understand that in the case of the "on-sync-succeeded" trigger, two notifications are sent: one with the username and icon set, and one without?

@reggie-k
Copy link
Member

@reggie-k Thank you for your review!

I'm sorry, but I don't really understand the situation. Is it okay to understand that in the case of the "on-sync-succeeded" trigger, two notifications are sent: one with the username and icon set, and one without?

Yeah, exactly

@ayatk
Copy link
Contributor Author

ayatk commented Sep 29, 2024

I looked into it, but I couldn't reproduce the situation you described...
When I tested it using go run examples/certmanager/cli/main.go template notify, I was able to set the icon and username without any problems, so I think the problem is probably somewhere else (such as the config) that is not related to this PR.

I have tested locally and found that in some use cases the notification is sent once with the username and icon, and once without them, meaning with the default ones, I tried that with "on-sync-succeeded" trigger.

I would like to confirm, does this problem occur in the revision before this PR? For example, if the same problem does not occur when the username and icon fields are set in service.slack, I will know that the code I wrote is causing the problem.

@reggie-k
Copy link
Member

Not sure I understand what to confirm? The problem is that, while the icon and username are set in the config like described in the docs of this PR, in the 2 slack notifications produced upon manually syncing the app from the UI, only one is actually with the username and icon configured, and the other one does not have the username and the icon.

@reggie-k
Copy link
Member

I reproduced it upon running locally make run and then syncing the app from the UI.
This is how the sync notification look like in slack:
Screenshot 2024-09-30 at 16 46 30

You can see that there are 3 messages sent to slack, only one of them is with the icon and the username, and two look like default icon and username.

Copy link
Member

@reggie-k reggie-k left a comment

Choose a reason for hiding this comment

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

Sorry!
This was a problem with my local config.
LGTM

Copy link
Member

@pasha-codefresh pasha-codefresh left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. Please open PR into Argo CD with notification-engine update

@pasha-codefresh pasha-codefresh merged commit 2fef5c9 into argoproj:master Oct 7, 2024
3 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.

3 participants