Skip to content

Conversation

GomathiselviS
Copy link
Contributor

SUMMARY

This PR incorporates a proposal presenting alternatives for an effective code review tracking approach.

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

Copy link
Contributor

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

My thoughts: I dislike option 1 because it seems like duplication of effort. Github already indicates when reviews are completed and/or changes are requested, so I wouldn't want us to add an extra step to put that information in Slack as well. Option 2 seems potentially better since it wouldn't actually require extra effort, it would just consolidate the information in one place for all PRs. However I'm not sure it will address the issue that I was hoping to see us manage better, which is having some kind of indication that a review is underway.

@hakbailey
Copy link
Contributor

I was thinking about this some more and wonder if commenting on Jira tickets would be another possible approach? The downside to that is it would only apply to PRs that have associated Jira tickets. The upside is it would keep all activity related to a ticket, including review progress, in one place.

@gravesm
Copy link
Member

gravesm commented Feb 12, 2024

I worry about any kind of automated notifications in slack. I think these will pretty quickly just be ignored and end up as noise. What are the problems we are trying to solve here?

@alinabuzachis
Copy link
Contributor

alinabuzachis commented Feb 21, 2024

I think option 1 seems like duplication of effort, as Helen mentioned. If we integrate Option 2 into the team channel, the notifications will probably introduce a lot of noise and some relevant information (announcements, etc.) may be lost. Integrating Option 2 into a dedicated channel might be better, but we already have a lot of channels to sift through and sometimes information might not show up if not checked regularly (we will have to check it regularly). I will probably lean toward Option 3, but I also think we should improve our overall review process by moving Jira tickets from "Code Review" to "In Progress" when needed and vice versa, or try to be more responsive to reviews. Generally, when I see that a PR has already been reviewed (and I have similar suggestions) and comments have not been addressed, I skip it, but this doesn't mean that I didn't pass thru it.

Copy link
Member

@gravesm gravesm left a comment

Choose a reason for hiding this comment

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

I still think the best way to solve this problem is for PR authors to just post in the team slack channel when they need reviews. If anything, people will be more likely to ignore automated reminders than specific requests.


**Time**: 9:00 AM EST

**Repositories**: `amazon.aws`, `amazon.cloud`, `cloud-content-handbook`, `cloud.common`, `community.aws`
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should include community supported repos. This is also missing all the validated content repos, the provider repos and several supported collections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those repositories are managed by a different org, so I'll need permission to add the reminders. Once this proposal is approved, I'll proceed with adding them.

@GomathiselviS
Copy link
Contributor Author

I still think the best way to solve this problem is for PR authors to just post in the team slack channel when they need reviews. If anything, people will be more likely to ignore automated reminders than specific requests.

Scheduled reminders can be configured to post once or twice a week. By consolidating PR review requests into a single notification, team members can easily access and prioritize them without the need to search through Slack messages. I propose trying this with bi-weekly reminders to gather feedback. If it proves ineffective, we can easily remove the reminders.

@GomathiselviS GomathiselviS requested a review from gravesm April 26, 2024 15:28
@hakbailey
Copy link
Contributor

I guess maybe I'd like us to back up a bit and have a discussion about why it takes so long for us to get PRs reviewed. There are a lot of possible reasons, such as: people don't know there are PRs ready to review, people don't have/aren't making time for regular code review, PRs are large so it takes a long time to review them, likely other reasons I haven't thought of, etc. This notification system would only resolve the first of those possible reasons and I'm not sure that's the primary issue we're dealing with...it could be, I'd just like to chat about the root causes as a team before we start implementing solutions.

@alinabuzachis
Copy link
Contributor

alinabuzachis commented Apr 30, 2024

I have a different idea. How about creating a script that runs on AWS, for example, that for each repository we manage collects pull requests (we may need to use some labels like ready_to_review to collect only these) opened by each member who is part of our team. The script needs to run daily (like our status update in Slack) and there are two options:

  • We include a link in our schedules daily status update on Jira to a very simple user interface where we can see the list of all PRs that have the ready_to_review label and remind everyone to scroll through the list. The list can be updated twice a day (once in the morning (EMEA time) on one in the evening (US time)).
  • We could investigate and see if Slack can be integrated with third-party APIs. An API should post in Slack a basic list with all open PRs in the morning (at the same time as our status update in Slack) and one in the evening (U.S. time) - but having two posts could be noisy.

This approach will help to have a centralized information with all the PRs that need to be reviewed, avoiding having to scroll through Jira each time for each ticket and check if there is a linked PR and, if there is not, go get it from Github, or get any other PR that is not linked to Jira but has been published in the channel.

@GomathiselviS
Copy link
Contributor Author

I have a different idea. How about creating a script that runs on AWS, for example, that for each repository we manage collects pull requests (we may need to use some labels like ready_to_review to collect only these) opened by each member who is part of our team. The script needs to run daily (like our status update in Slack) and there are two options:

  • We include a link in our schedules daily status update on Jira to a very simple user interface where we can see the list of all PRs that have the ready_to_review label and remind everyone to scroll through the list. The list can be updated twice a day (once in the morning (EMEA time) on one in the evening (US time)).
  • We could investigate and see if Slack can be integrated with third-party APIs. An API should post in Slack a basic list with all open PRs in the morning (at the same time as our status update in Slack) and one in the evening (U.S. time) - but having two posts could be noisy.

This approach will help to have a centralized information with all the PRs that need to be reviewed, avoiding having to scroll through Jira each time for each ticket and check if there is a linked PR and, if there is not, go get it from Github, or get any other PR that is not linked to Jira but has been published in the channel.

Hi @alinabuzachis From my understanding, the Scheduled reminders feature in GitHub achieves similar functionality to what you've described. While it doesn't involve a script directly, it can be configured to send reminders via a Slack channel. I've outlined the configurations available, such as specifying the reminder timing and the repositories to include. We can integrate the label feature you mentioned. With the configurations I have mentioned, users need to manually add reviewers (in this case, the cloud team) if we want the PR to be included in the reminder.

I can include a demo in the team call to explain the configurations. Let me know if you have any further questions or if you'd like to discuss this further.

@alinabuzachis
Copy link
Contributor

alinabuzachis commented Apr 30, 2024

I have a different idea. How about creating a script that runs on AWS, for example, that for each repository we manage collects pull requests (we may need to use some labels like ready_to_review to collect only these) opened by each member who is part of our team. The script needs to run daily (like our status update in Slack) and there are two options:

  • We include a link in our schedules daily status update on Jira to a very simple user interface where we can see the list of all PRs that have the ready_to_review label and remind everyone to scroll through the list. The list can be updated twice a day (once in the morning (EMEA time) on one in the evening (US time)).
  • We could investigate and see if Slack can be integrated with third-party APIs. An API should post in Slack a basic list with all open PRs in the morning (at the same time as our status update in Slack) and one in the evening (U.S. time) - but having two posts could be noisy.

This approach will help to have a centralized information with all the PRs that need to be reviewed, avoiding having to scroll through Jira each time for each ticket and check if there is a linked PR and, if there is not, go get it from Github, or get any other PR that is not linked to Jira but has been published in the channel.

Hi @alinabuzachis From my understanding, the Scheduled reminders feature in GitHub achieves similar functionality to what you've described. While it doesn't involve a script directly, it can be configured to send reminders via a Slack channel. I've outlined the configurations available, such as specifying the reminder timing and the repositories to include. We can integrate the label feature you mentioned. With the configurations I have mentioned, users need to manually add reviewers (in this case, the cloud team) if we want the PR to be included in the reminder.

I can include a demo in the team call to explain the configurations. Let me know if you have any further questions or if you'd like to discuss this further.

Yes, that will be helpful to better understand. If it's allowing to have a unified view of all the PRs we need to review, it will be great! Can you please record the demo because I'll be off tomorrow (if you're planning to demo it tomorrow)? Thanks.

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