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

Re-approve when another review is requested #209

Closed
wants to merge 2 commits into from
Closed

Conversation

hmarr
Copy link
Owner

@hmarr hmarr commented Oct 20, 2022

When a review has already been left for the current commit, but the review is dismissed and re-requested, we should leave another approving review.

This also switches uses Promise.all to fetch the PR, review, and review requests concurrently.

Closes #206.

const commit = pull_request.data.head.sha;
const { owner, repo } = context.repo;
const queryParams = { owner, repo, pull_number: prNumber };
const [pullRequest, reviews, requestedReviewers] = await Promise.all([
Copy link
Contributor

@vincejv vincejv Oct 20, 2022

Choose a reason for hiding this comment

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

Hi @hmarr, while doing the fixes, I realized we can get requested_reviewers from pull_request, I was about to PR this, so no additional rest calls should be necessary, if my understanding is correct with how Github API works

for (const review of reviews.data) {
      if (
        review.user?.login == login &&
        review.commit_id == commit &&
        review.state == "APPROVED" &&
        pull_request.data
        .requested_reviewers?.filter(
          reviewer => reviewer.login == login
        ).length == 0
      ) {
        core.info(
          `Current user already approved pull request #${prNumber}, nothing to do`
        );
        return;
      }
    }

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh great point! Please do change that and submit the PR. I'd definitely rather avoid the extra request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh great point! Please do change that and submit the PR. I'd definitely rather avoid the extra request.

The code is vastly different now, so I recommend for you to do the change 😁 on how you like it, but just suggesting that we can get the field without the additional rest call.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hmarr nevermind I PRd #210, let me know your thoughts...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great, I'll close this out in favour of your pull request. Thanks for contributing!

@hmarr hmarr closed this Oct 20, 2022
@hmarr hmarr deleted the reapprove-on-request branch October 20, 2022 19:21
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.

Action does not perform review when requesting for re-review
2 participants