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

Improve PR review experience #9699

Closed
andrross opened this issue Sep 1, 2023 · 5 comments
Closed

Improve PR review experience #9699

andrross opened this issue Sep 1, 2023 · 5 comments
Labels
Build Libraries & Interfaces enhancement Enhancement or improvement to existing feature or request

Comments

@andrross
Copy link
Member

andrross commented Sep 1, 2023

The most important information on the front page of a PR is the author's description of the change and the reviewers' comments. However, we have made liberal use of the "create-or-update" GitHub action for our various workflows to post information directly on the PR. This is all well intentioned and there is important information being posted, but I believe we've probably gone to far and the single-to-noise ratio here has dropped, and too often comments are obscured by the dreaded "load more" button. Also my scroll wheel finger is getting quite the workout :) Here are some specific thoughts I have to improve things:

  • Move to a "no news is good news" model. If an action succeeds, I think the green checkmark at the bottom of the page is sufficient. No need to make a comment indicating success.
  • Dedupe and cancel workflows when PR updates are pushed. Workflows can be slow, and it is quite common to push a commit, find a problem, then push another commit a few minutes later. Currently both of these commits will complete a gradle check run and both runs will report a result as a comment.
  • Change the "check" and "check compatibility" workflows to update a single comment with the latest results instead of only appending (not sure if this is a good idea to be honest)

Any other ideas?

@BhumikaSaini-Amazon
Copy link
Contributor

BhumikaSaini-Amazon commented Sep 4, 2023

+1 for this, especially true for critical / long-running PRs.

I haven't tested on the feasibility of this yet, but does it make sense to create a new GitHub issue (with relevant tags added) for each PR (similar to the auto backport PRs for example), wherein all of the GitHub actions can post the results etc.? The issue description could house the latest info from the most recent action runs. On the PR side, there could be a validation to resolve open comments on the linked issue before merging the PR. This would also keep the PR conversation relatively more clean with only the review comments displayed.

99% of the times, we are only looking for the results from the most recent runs. Its only when we want to maybe fetch a specific test seed or go back for another issue we observed in a previous build/action that we actually go looking for the previous build/action results.

@dblock
Copy link
Member

dblock commented Sep 5, 2023

I like the simple proposal of removing green noise!

Another idea is for gradle check to be deleting, or editing the previous gradle check comment, regardless of its status, so you only see the latest. This is something that danger does.

andrross added a commit to andrross/OpenSearch that referenced this issue Sep 5, 2023
Gradle checks take 30+ minutes to complete. It is quite common to push a
commit to a PR, then quickly discover a new change is needed and push an
updated commit, which results in two workflows running and both post a
result comment to the PR. By deduping and canceling we will reduce load
on the GitHub action runner (probably not on the Jenkins server unless
the cancellation can propagate to Jenkins) and reduce noise on the PR.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
@andrross
Copy link
Member Author

andrross commented Sep 5, 2023

does it make sense to create a new GitHub issue (with relevant tags added) for each PR

@BhumikaSaini-Amazon This is an interesting idea, but I'm inclined not to do this because the information that is posted in comments is already available elsewhere (in the list of github action workflows at the bottom of the PR), it just takes several clicks to get to the relevant information. The reason we post comments is to highlight the information and make it accessible with fewer clicks, I just think we've gone way too far with this.

@andrross
Copy link
Member Author

andrross commented Sep 5, 2023

gradle check to be deleting, or editing the previous gradle check comment

@dblock I think this is possible with the create-or-update workflow we're using (see example). The biggest challenge might be race conditions because it is rather important that only the latest run update the single comment.

andrross added a commit to andrross/OpenSearch that referenced this issue Sep 6, 2023
Instead of appending a new comment every time check compatibility runs
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
andrross added a commit to andrross/OpenSearch that referenced this issue Sep 6, 2023
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
@minalsha minalsha added enhancement Enhancement or improvement to existing feature or request and removed bug Something isn't working untriaged labels Sep 7, 2023
andrross added a commit to andrross/OpenSearch that referenced this issue Sep 9, 2023
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
andrross added a commit that referenced this issue Sep 9, 2023
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates #9699

Signed-off-by: Andrew Ross <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this issue Sep 9, 2023
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates #9699

Signed-off-by: Andrew Ross <[email protected]>
(cherry picked from commit c100c0c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this issue Sep 10, 2023
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates #9699


(cherry picked from commit c100c0c)

Signed-off-by: Andrew Ross <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kaushalmahi12 pushed a commit to kaushalmahi12/OpenSearch that referenced this issue Sep 12, 2023
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
andrross added a commit to andrross/OpenSearch that referenced this issue Sep 14, 2023
This is the same change that was applied to the check comptability task
by PR opensearch-project#9954 to update a single comment with the latest result of the
gradle check workflow.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
andrross added a commit to andrross/OpenSearch that referenced this issue Sep 14, 2023
This is the same change that was applied to the check comptability task
by PR opensearch-project#9954 to update a single comment with the latest result of the
gradle check workflow.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
andrross added a commit to andrross/OpenSearch that referenced this issue Sep 14, 2023
This is the same change that was applied to the check comptability task
by PR opensearch-project#9954 to update a single comment with the latest result of the
gradle check workflow.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
@andrross
Copy link
Member Author

The summary here is that the "check compatibility" workflow has been updated to update a single comment, bringing it in line with the codecov behavior, which should substantially reduce noise on the PR comment feed. However, multiple contributors mentioned that they prefer that "gradle check" create a new comment for every run so that it generates a notification that the PR needs attention (either success or failure). There were also opinions that it is desirable to not cancel concurrent gradle check runs because the additional runs can help find flaky tests.

sarthakaggarwal97 pushed a commit to sarthakaggarwal97/OpenSearch that referenced this issue Sep 20, 2023
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
brusic pushed a commit to brusic/OpenSearch that referenced this issue Sep 25, 2023
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
Signed-off-by: Ivan Brusic <[email protected]>
andrross added a commit to andrross/OpenSearch that referenced this issue Nov 10, 2023
Continuing in the spirit of opensearch-project#9699, this makes the gradle check comment
less verbose while containing all the same information. The goal is to
reduce visual noise on the PR feed and maybe allow for more comments
before GitHub starts hiding older comments behind the "load more" link.

This is super subjective so happy to hear any opinions if the previous
format is preferred.

Signed-off-by: Andrew Ross <[email protected]>
andrross added a commit that referenced this issue Nov 10, 2023
Continuing in the spirit of #9699, this makes the gradle check comment
less verbose while containing all the same information. The goal is to
reduce visual noise on the PR feed and maybe allow for more comments
before GitHub starts hiding older comments behind the "load more" link.

This is super subjective so happy to hear any opinions if the previous
format is preferred.

Signed-off-by: Andrew Ross <[email protected]>
andrross added a commit to andrross/OpenSearch that referenced this issue Nov 10, 2023
Continuing in the spirit of opensearch-project#9699, this makes the gradle check comment
less verbose while containing all the same information. The goal is to
reduce visual noise on the PR feed and maybe allow for more comments
before GitHub starts hiding older comments behind the "load more" link.

This is super subjective so happy to hear any opinions if the previous
format is preferred.

Signed-off-by: Andrew Ross <[email protected]>
(cherry picked from commit dcb64d8)
reta pushed a commit that referenced this issue Nov 11, 2023
Continuing in the spirit of #9699, this makes the gradle check comment
less verbose while containing all the same information. The goal is to
reduce visual noise on the PR feed and maybe allow for more comments
before GitHub starts hiding older comments behind the "load more" link.

This is super subjective so happy to hear any opinions if the previous
format is preferred.

Signed-off-by: Andrew Ross <[email protected]>
(cherry picked from commit dcb64d8)
fahadshamiinsta pushed a commit to fahadshamiinsta/OpenSearch270 that referenced this issue Dec 4, 2023
Continuing in the spirit of opensearch-project#9699, this makes the gradle check comment
less verbose while containing all the same information. The goal is to
reduce visual noise on the PR feed and maybe allow for more comments
before GitHub starts hiding older comments behind the "load more" link.

This is super subjective so happy to hear any opinions if the previous
format is preferred.

Signed-off-by: Andrew Ross <[email protected]>
rayshrey pushed a commit to rayshrey/OpenSearch that referenced this issue Mar 18, 2024
Continuing in the spirit of opensearch-project#9699, this makes the gradle check comment
less verbose while containing all the same information. The goal is to
reduce visual noise on the PR feed and maybe allow for more comments
before GitHub starts hiding older comments behind the "load more" link.

This is super subjective so happy to hear any opinions if the previous
format is preferred.

Signed-off-by: Andrew Ross <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this issue Apr 25, 2024
Instead of appending a new comment every time check compatibility runs,
this will instead update a single comment with the latest results. The
history of previous runs is retained in the comment as GitHub allows you
to view the edited revisions of any comment. This matches the behavior
of the codecov workflow and should substantially reduce noise in PRs.

Relates opensearch-project#9699

Signed-off-by: Andrew Ross <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this issue Apr 25, 2024
Continuing in the spirit of opensearch-project#9699, this makes the gradle check comment
less verbose while containing all the same information. The goal is to
reduce visual noise on the PR feed and maybe allow for more comments
before GitHub starts hiding older comments behind the "load more" link.

This is super subjective so happy to hear any opinions if the previous
format is preferred.

Signed-off-by: Andrew Ross <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Libraries & Interfaces enhancement Enhancement or improvement to existing feature or request
Projects
None yet
Development

No branches or pull requests

4 participants