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

Add auto approve workflow for dependency PRs #3184

Merged
merged 9 commits into from
Jan 6, 2022
Merged

Conversation

gsheni
Copy link
Contributor

@gsheni gsheni commented Jan 5, 2022

  • This new workflow runs every 30 minutes.
  • It will auto-approve and set auto-merge on dependency PRs.
  • Added workflow_dispatch so unit tests, and latest dep check can be kickoff off manually.

@gsheni gsheni changed the title Create auto_approve_dependency_PRs.yml Add auto approve workflow for dependency PRs Jan 5, 2022
@gsheni gsheni self-assigned this Jan 5, 2022
@gsheni gsheni marked this pull request as ready for review January 5, 2022 19:00
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #3184 (ca9e6a3) into main (37f2734) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3184   +/-   ##
=====================================
  Coverage   99.7%   99.7%           
=====================================
  Files        326     326           
  Lines      31444   31444           
=====================================
  Hits       31340   31340           
  Misses       104     104           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37f2734...ca9e6a3. Read the comment docs.

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

@gsheni Looks good to me! Left some non-blocking questions

echo ${{ steps.find_prs.outputs.dep_pull_request }}
gh pr review --repo "${{ github.repository }}" --comment --body "auto approve" ${{ steps.find_prs.outputs.dep_pull_request }}
gh pr review --repo "${{ github.repository }}" --approve ${{ steps.find_prs.outputs.dep_pull_request }}
gh pr merge --repo "${{ github.repository }}" --auto --squash ${{ steps.find_prs.outputs.dep_pull_request }}
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will error out if any of the CI checks are red? What if some are still running?

If this doesn't error out for pending checks, should we change the cron to run every 5 minutes? That way this gets merged as soon as CI finishes + passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The status:success review:required means that it will only auto approve if all CI passes.

I can change it to run every 5 minutes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this auto-approve and merge if a reviewer approves but leaves comments on nit-picks/suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only for dependency update PRs, such as this:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My hope would be that EvalML reviewers are no longer touching those types of PRs (when they pass all CI checks).

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

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

This is really cool! Left a nit and a question that would be useful to address

@@ -11,7 +11,7 @@ Release Notes
* Fixed ``mean_cv_data`` and ``validation_score`` values in AutoMLSearch.rankings to reflect cv score or ``NaN`` when appropriate :pr:`3162`
* Documentation Changes
* Testing Changes

* Add workflow to auto-merge dependency PRs if status checks pass (:pr:`3184`)
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit, but we don't put parentheses around the PR number.

echo ${{ steps.find_prs.outputs.dep_pull_request }}
gh pr review --repo "${{ github.repository }}" --comment --body "auto approve" ${{ steps.find_prs.outputs.dep_pull_request }}
gh pr review --repo "${{ github.repository }}" --approve ${{ steps.find_prs.outputs.dep_pull_request }}
gh pr merge --repo "${{ github.repository }}" --auto --squash ${{ steps.find_prs.outputs.dep_pull_request }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this auto-approve and merge if a reviewer approves but leaves comments on nit-picks/suggestions?

Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for doing this!

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

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

🎉

@gsheni gsheni enabled auto-merge (squash) January 6, 2022 17:05
@gsheni gsheni merged commit e383f24 into main Jan 6, 2022
@gsheni gsheni deleted the add_auto_approve_dep branch January 6, 2022 18:39
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