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 issue auto-assignment and improve PR reviewer assignment #3045

Merged
merged 4 commits into from
May 2, 2023

Conversation

na--
Copy link
Member

@na-- na-- commented Apr 28, 2023

I've tested some of the things in a throwaway repo, but this might require a few subsequent PRs to fix any bugs...

@na--
Copy link
Member Author

na-- commented Apr 28, 2023

This PR doesn't actually use the new PR auto-assignment, since pull_request_target is used as the action. We basically need to merge this PR to see if the changes in it work... 😞

@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2023

Codecov Report

Merging #3045 (729bfe5) into master (6d5cabf) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 729bfe5 differs from pull request most recent head a016ca3. Consider uploading reports for the commit a016ca3 to get more accurate results

@@            Coverage Diff             @@
##           master    #3045      +/-   ##
==========================================
+ Coverage   77.01%   77.02%   +0.01%     
==========================================
  Files         229      229              
  Lines       17065    17065              
==========================================
+ Hits        13143    13145       +2     
+ Misses       3080     3079       -1     
+ Partials      842      841       -1     
Flag Coverage Δ
ubuntu 77.02% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/executor/base_config.go 86.48% <ø> (ø)

... and 5 files with indirect coverage changes

@na-- na-- requested a review from mstoykov April 28, 2023 13:28
mstoykov
mstoykov previously approved these changes Apr 28, 2023
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

I think there are a couple of crucial typos, but LGTM otherwise.

I'm not familiar with GitHub's API or the github-script action to know if this will work as we want it to, but it looks like it 😄

.github/workflows/pr-auto-assign.yml Outdated Show resolved Hide resolved
Comment on lines -11 to +14
- uses: kentaro-m/auto-assign-action@6d966a1f3bd73adfe44dd19dc42cc5845e39ebd3 # v1.2.4
- uses: actions/github-script@v6
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice that this is a relatively trivial dependency to remove 👍

.github/workflows/pr-auto-assign.yml Outdated Show resolved Hide resolved
Comment on lines 21 to 25
github.rest.pulls.requestReviewers({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.payload.pull_request.number,
reviewers: assigneesWithoutAuthor.sort(() => 0.5 - Math.random()).slice(0, reviewerCount),
Copy link
Contributor

@imiric imiric Apr 28, 2023

Choose a reason for hiding this comment

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

Do you know if this will only do the assignment if the PR is marked as "ready for review", i.e. not for draft PRs? If not, we should ignore the assignment for draft PRs.

Ah, the on configuration above should handle it, right?

on:
  pull_request_target:
    types: [opened, ready_for_review, reopened]

Though does opened include draft PRs? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I am not sure, https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request doesn't seem to document this explicitly. I suggest we try it like this and adjust if needed.

My guess is that just the ready_for_review event won't trigger if I open a new non-draft PR, so we need all 3. The actual solution might be to add some if JavaScript check in the job body to short-circuit it and return early if the PR is in a draft state 🤔

Co-authored-by: Ivan Mirić <ivan.miric@grafana.com>
mstoykov
mstoykov previously approved these changes Apr 28, 2023
Co-authored-by: Ivan Mirić <ivan.miric@grafana.com>
@na-- na-- merged commit 4c148a7 into master May 2, 2023
@na-- na-- deleted the auto-assign-ci-actions branch May 2, 2023 06:43
@mstoykov mstoykov added this to the v0.45.0 milestone Jun 5, 2023
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