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

make status notifications minimal #162

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Conversation

thatportugueseguy
Copy link
Collaborator

After merging and deploying #160 we realized that we need faster feedback and thus can't wait for a full build to finish.

We need to get slack notifications on the build is failing gh notification. This means that when we get the gh notification we don't have any certainty about which steps will fail in that build, we only know one for certain that something has failed and that the whole pipeline will have a failed build at the end.

So we discussed and opted for making the notifications as minimal as possible.

example success notification:
Screenshot 2024-10-08 at 14 09 42

DM notification doesn't have repeated commit message or author, since the author is the user receiving the message
Screenshot 2024-10-08 at 14 10 07

updated tests

Comment on lines +268 to +269
(* if we have a DM notification, we don't need to repeat the commit message and author because
the user receiving the message is already the author of that commit. Users handles start with U *)
Copy link
Contributor

Choose a reason for hiding this comment

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

we even never need to have duplicated information?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the channel notifications we could remove the duplicate commit message and keep only the author. I kept it for the sake of the format we had before, but I think it's ok to remove the duplicate commit message entirely

commit.author.name author);
(* if we have a DM notification, we don't need to repeat the commit message and author because
the user receiving the message is already the author of that commit. Users handles start with U *)
(match Devkit.Stre.starts_with channel "U" with
Copy link
Contributor

Choose a reason for hiding this comment

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

feels very very hackish

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hum, you are right. Should update to use a regular expression. Ids in slack start with U for users and C for channels, so that was my reasoning, but since we don't use # prefixes when declaring slack channels, it is better to go with the regexp.

Copy link
Contributor

Choose a reason for hiding this comment

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

can't we use an actual type instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can, it's probably a good idea, but it forces changes in a lot of places and more or less repeated, I'll have another pass at the code to see if I can refactor the rule matching in some way to minimize the places where we need to make these changes. If it's not possible, then need to make the changes in all the places that get the channels list for the notifications

@Khady
Copy link
Contributor

Khady commented Oct 9, 2024

merging as it restores the quick feedback loop, can iterate on message format in another PR

@Khady Khady merged commit 38b0672 into master Oct 9, 2024
0 of 3 checks passed
@Khady Khady deleted the make-notifications-minimal branch October 9, 2024 03:06
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.

2 participants