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

CI status updates sent as direct message to author of commit #134

Merged
merged 7 commits into from
Feb 14, 2024

Conversation

koonwen
Copy link
Contributor

@koonwen koonwen commented Aug 8, 2023

This PR changes the channel routing logic for CI updates. The old behavior used to send ci updates to the default channel specified in the config. The new behavior now sends the ci update as a direct message to the author of the commit.

How it works

The slack post_message api is able to send a direct message to a slack user by filling the channel field instead with the user_id. Since the Github commit author email and the slack profile email is the same, I have injected a new slack api call to get the user_id of a profile from a user's email address.

Test

expect tests in test/slack_payloads.expected have been updated showing the slack notifications being sent to the author of the commit via the user_id

References

#81

Copy link
Contributor

@rr0gi rr0gi left a comment

Choose a reason for hiding this comment

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

This will probably get complaints from people who didn't break build but push on top of broken build, the ideal solution should notify only the first person who broke build, but we can try with this simple solution, easy to revert if too much noise.

PS unrelated to PR but monorobot should be switched to slack lib cc @sewenthy

lib/api_remote.ml Outdated Show resolved Hide resolved
lib/api_remote.ml Outdated Show resolved Hide resolved
test/slack_payloads.expected Outdated Show resolved Hide resolved
lib/api_remote.ml Outdated Show resolved Hide resolved
lib/action.ml Outdated Show resolved Hide resolved
lib/action.ml Outdated Show resolved Hide resolved
lib/api_remote.ml Outdated Show resolved Hide resolved
lib/api_remote.ml Outdated Show resolved Hide resolved
@koonwen koonwen marked this pull request as ready for review August 9, 2023 02:56
@Khady
Copy link
Contributor

Khady commented Aug 21, 2023

Since the Github commit author email and the slack profile email is the same

This isn't actually true for everyone. But I would say that it's still better than nothing. Later on we can figure out a way to inject a list of people.

@koonwen
Copy link
Contributor Author

koonwen commented Aug 21, 2023

This isn't actually true for everyone. But I would say that it's still better than nothing. Later on we can figure out a way to inject a list of people.

@Khady Right, I thought that might be an issue. Initially I was hoping to have the lookup_user function use a dynamic cache but it could easily also load in some list of user mappings. I can look at adding that

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.

4 participants