-
Notifications
You must be signed in to change notification settings - Fork 6
Enhance Slack notifications and automate GitHub assignment replies based on issue labels #29
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
Conversation
|
@GarvitSinghal47 I cannot assign you as a reviewer, but please do review if you'd like. |
GarvitSinghal47
left a comment
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.
Done with the first round of structure review — will now add comments focused on the logic if find any.
|
Hi @iamshobhraj, thanks so much - I set this up in my test environment, and saw main pieces work! Fantastic work. I plan to test and review in more detail gradually, and meanwhile, already feel free to work on @GarvitSinghal47's review if you'd like. And @GarvitSinghal47 thanks for review, it all makes sense. |
@GarvitSinghal47 Thanks for the review and guidance. I have made some changes based on your review and added some suggestions aswell |
MisRob
left a comment
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.
Hi @iamshobhraj, so I tested all the many paths successfully 🎉
I really appreciate the effort you clearly put into following the diagram and all requirements that @GarvitSinghal47 prepared, and thanks Garvit for that too - it helped me during review.
I'm leaving few notes, mostly a bit of cleanup and renaming to improve clarity.
Finally, I will invite @rtibbles to review security aspect, and give us final approve.
Then we can complete transfer of this work to production together. Thank you!
| if(matchedKeyword){ | ||
| let lastBotComment; | ||
| let PastBotComments = await findRecentCommentsByUser(LE_BOT_USERNAME); | ||
| if(PastBotComments.length > 0){ |
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.
When I was testing this, I realized that for team members who are not familiar with the 1 hour optimization, not sending a bot reply and Slack notification about it may look like an error, despite it being intentional.
If we're skipping GitHub bot reply because it has been already sent in the past hour, could we send Slack notification about it too, that would include a brief note about why it's being skipped?
So for this situation, on Slack we would see:
- [repo] New comment on issue: Test issue by MisRob2
- [repo] Bot response skipped on issue: Test issue (less than 1 hour since last bot reply)
Additional benefit is that I will too see that everything is working as expected, and will be able to recognize this scenario easily from when there is an unexpected problem with the action.
Nothing blocking! I know this wasn't in the original requirements. I can open a follow-up issue or update it myself in case it wouldn't be convenient for you.
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 sounds a like a very good step to include in this workflow and if there is a benefit of adding this, I will be more than happy to work on it. Can you open a follow up issue and assign it to me.
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.
Thanks a lot, I will open an issue soon and let you know
for clarity.
to align with the emerging patterns and structures of other newer workflows.
to align it with the new workflow name.
|
Thanks a lot @iamshobhraj! I re-reviewed and re-tested everything. All important pieces work well. I only made few smaller tweaks that I will push now - mostly around organization, naming, and final messaging to align everything to some newer patterns. More details in commits. I will now merge and do a final test in LE environment before I turn it on in other repositories. Meanwhile, if you figure out any way to improve security, that will definitely be welcome. Following-up on your Slack note, as soon as I open an issue for #29 (comment), I will think about how to make work for you easier. I have my own Slack test environment and GitHub organization. Perhaps I could try to invite you there. Generally I'd like to explore how to allow contributors develop and test actions easily without requiring you to set up everything from scratch - I went through the process myself so I know how cumbersome and time consuming it is. Perhaps we can try exploring this together, if you'd be open to it. |
Thanks @MisRob for reviewing and helping with this PR and also the offer, this offer sounds great! I’d be happy to explore this and see how we can make things easier for contributors. Let me know the next steps. |
|
Perfect @iamshobhraj. Yesterday I observed the workflow run in production - all looks great, except one hiccup I've just reported here #30. I also opened a follow-up we talked about above #31. From my perspective, the first issue is more important, but feel free to pick any of them, if you still wish so.
Thanks a lot for being available. Let me invite you to my Slack and GitHub test organization today and let's see how it works. I will send you a Slack message soon with more. |
I think this issue is more important. I'll prioritize and work on this first and try to fix it in couple of days so that everything works correctly in production
After #30 gets fixed I'll start working on this. |
Summary
Updated the existing
notify_team_new_commentworkflow.This update adds the following steps :-
I've tested this by creating a testing environment.
References
addresse issue #23