batch-changes: ensure PR is only approved after code owner review by davejrt · Pull Request #53601 · sourcegraph/sourcegraph-public-snapshot

davejrt · 2023-06-16T12:24:39Z

Attempts to address https://github.com/sourcegraph/sourcegraph/issues/50874

By getting the "ReviewDecision" from a PR which is only "APPROVED" once a codeowner (if required) has reviewed the PR, otherwise this field returns "REVIEW_REQUIRED" or "CHANGES_REQUSTED".

Given there is a no longer a requirement for events on github PRs, we don't need to calculate if the changeset is older than events before computing the state.

Test plan

CI passing, tested locally with a repo on ghe.sgdev.org with a codeowners file

davejrt · 2023-06-16T12:32:06Z

Current issue is, the MergableState field on the pullrequest type is empty, despite it being populated when querying the pull request via the github api:

  "BaseRepository": {
    "ID": "MDEwOlJlcG9zaXRvcnk5Nzg=",
    "Name": "JetBrains-ideavim",
    "Owner": {
      "Login": "sourcegraph"
    }
  },
  "HeadRepository": {
    "ID": "MDEwOlJlcG9zaXRvcnk5Nzg=",
    "Name": "JetBrains-ideavim",
    "Owner": {
      "Login": "sourcegraph"
    }
  },
  "MergeableState": ""
}

  "author_association": "MEMBER",
  "auto_merge": null,
  "active_lock_reason": null,
  "merged": false,
  "mergeable": true,
  "rebaseable": true,
  "mergeable_state": "blocked",
  "merged_by": null,
  "comments": 0,
  "review_comments": 0,
  "maintainer_can_modify": false,
  "commits": 1,
  "additions": 1,
  "deletions": 0,
  "changed_files": 1
}```

davejrt · 2023-06-16T15:37:19Z

Example of what is returned from createPullRequest where mergablestate is empty

pr: &{RepoWithOwner: 
ID:MDExOlB1bGxSZXF1ZXN0OTg1 
Title:Hello World 
Body:My first batch change!
[_Created by Sourcegraph batch change `admin/hello-world`._]
(https://sourcegraph.test:3443/users/admin/batch-changes/hello-world) 
State:OPEN URL:https://ghe.sgdev.org/sourcegraph/JetBrains-ideavim/pull/15 
HeadRefOid:d08b7c73844d4d97beafdba5def4b0cea6fd1639 
BaseRefOid:e44ea7ae691ee9004d0b0c9167a58af044e94ba9 
HeadRefName:hello-world-2 
BaseRefName:master 
Number:15 
MergeableState: 
Author:{AvatarURL:https://ghe.sgdev.org/avatars/u/3 
Login:milton 
URL:https://ghe.sgdev.org/milton} 
BaseRepository:{ID:MDEwOlJlcG9zaXRvcnk5Nzg= 
Name:JetBrains-ideavim Owner:{Login:sourcegraph}} 
HeadRepository:{ID:MDEwOlJlcG9zaXRvcnk5Nzg= 
Name:JetBrains-ideavim 
Owner:{Login:sourcegraph}} 
Participants:[] 
Labels:{Nodes:[]} 
TimelineItems:[] 
Commits:{Nodes:[{Commit:{OID:d08b7c73844d4d97beafdba5def4b0cea6fd1639 
CheckSuites:{Nodes:[]} 
Status:{State: 
Contexts:[]} 
CommittedDate:2023-06-16 15:00:13 +0000 UTC}}]} 
IsDraft:false 
CreatedAt:2023-06-16 15:00:33 +0000 UTC 
UpdatedAt:2023-06-16 15:00:33 +0000 UTC}

sourcegraph-bot · 2023-06-22T01:32:36Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 935a989...8c61612.

Notify	File(s)
@eseliger	enterprise/internal/batches/sources/github_test.go enterprise/internal/batches/sources/testdata/golden/GithubSource_CloseChangeset_DeleteSourceBranch_success enterprise/internal/batches/sources/testdata/golden/GithubSource_CloseChangeset_success enterprise/internal/batches/sources/testdata/golden/GithubSource_CreateChangeset_already_exists enterprise/internal/batches/sources/testdata/golden/GithubSource_CreateChangeset_success enterprise/internal/batches/sources/testdata/golden/GithubSource_LoadChangeset_found enterprise/internal/batches/sources/testdata/golden/GithubSource_ReopenChangeset_success enterprise/internal/batches/sources/testdata/golden/GithubSource_UpdateChangeset_success enterprise/internal/batches/sources/testdata/sources/GithubSource_CloseChangeset_DeleteSourceBranch_success.yaml enterprise/internal/batches/sources/testdata/sources/GithubSource_CloseChangeset_success.yaml enterprise/internal/batches/sources/testdata/sources/GithubSource_CreateChangeset_already_exists.yaml enterprise/internal/batches/sources/testdata/sources/GithubSource_CreateChangeset_success.yaml enterprise/internal/batches/sources/testdata/sources/GithubSource_LoadChangeset_found.yaml enterprise/internal/batches/sources/testdata/sources/GithubSource_LoadChangeset_not-found.yaml enterprise/internal/batches/sources/testdata/sources/GithubSource_ReopenChangeset_success.yaml enterprise/internal/batches/sources/testdata/sources/GithubSource_UpdateChangeset_success.yaml enterprise/internal/batches/state/state.go enterprise/internal/batches/state/state_test.go enterprise/internal/batches/types/changeset.go internal/extsvc/github/common.go internal/extsvc/github/testdata/golden/ClosePullRequest-0 internal/extsvc/github/testdata/golden/ClosePullRequest-1 internal/extsvc/github/testdata/golden/CreatePullRequest-0 internal/extsvc/github/testdata/golden/CreatePullRequest-3 internal/extsvc/github/testdata/golden/LoadPullRequest-2 internal/extsvc/github/testdata/golden/LoadPullRequest-3 internal/extsvc/github/testdata/golden/MarkPullRequestReadyForReview-0 internal/extsvc/github/testdata/golden/MarkPullRequestReadyForReview-1 internal/extsvc/github/testdata/golden/MergePullRequest-error internal/extsvc/github/testdata/golden/MergePullRequest-success internal/extsvc/github/testdata/golden/ReopenPullRequest-0 internal/extsvc/github/testdata/golden/ReopenPullRequest-1 internal/extsvc/github/testdata/vcr/ClosePullRequest.yaml internal/extsvc/github/testdata/vcr/CreatePullRequest.yaml internal/extsvc/github/testdata/vcr/CreatePullRequestComment.yaml internal/extsvc/github/testdata/vcr/CreatePullRequest_Archived.yaml internal/extsvc/github/testdata/vcr/LoadPullRequest.yaml internal/extsvc/github/testdata/vcr/MarkPullRequestReadyForReview.yaml internal/extsvc/github/testdata/vcr/ReopenPullRequest.yaml internal/extsvc/github/testdata/vcr/TestMergePullRequest.yaml internal/extsvc/github/v4_test.go
@sashaostrikov	internal/extsvc/github/common.go internal/extsvc/github/testdata/golden/ClosePullRequest-0 internal/extsvc/github/testdata/golden/ClosePullRequest-1 internal/extsvc/github/testdata/golden/CreatePullRequest-0 internal/extsvc/github/testdata/golden/CreatePullRequest-3 internal/extsvc/github/testdata/golden/LoadPullRequest-2 internal/extsvc/github/testdata/golden/LoadPullRequest-3 internal/extsvc/github/testdata/golden/MarkPullRequestReadyForReview-0 internal/extsvc/github/testdata/golden/MarkPullRequestReadyForReview-1 internal/extsvc/github/testdata/golden/MergePullRequest-error internal/extsvc/github/testdata/golden/MergePullRequest-success internal/extsvc/github/testdata/golden/ReopenPullRequest-0 internal/extsvc/github/testdata/golden/ReopenPullRequest-1 internal/extsvc/github/testdata/vcr/ClosePullRequest.yaml internal/extsvc/github/testdata/vcr/CreatePullRequest.yaml internal/extsvc/github/testdata/vcr/CreatePullRequestComment.yaml internal/extsvc/github/testdata/vcr/CreatePullRequest_Archived.yaml internal/extsvc/github/testdata/vcr/LoadPullRequest.yaml internal/extsvc/github/testdata/vcr/MarkPullRequestReadyForReview.yaml internal/extsvc/github/testdata/vcr/ReopenPullRequest.yaml internal/extsvc/github/testdata/vcr/TestMergePullRequest.yaml internal/extsvc/github/v4_test.go

BolajiOlajide

Thanks for the fix @davejrt !

BolajiOlajide · 2023-06-22T11:46:42Z

 	HeadRefName    string
 	BaseRefName    string
 	Number         int64
+	ReviewDecision string


I noticed that the ReviewDecision field only exists in the GraphQL response for loading a pull request and not the response from the REST API.

I've checked and confirmed we only load pull requests using the v4 client.

…3601) Attempts to address https://github.com/sourcegraph/sourcegraph/issues/50874 By getting the "ReviewDecision" from a PR which is only "APPROVED" once a codeowner (if required) has reviewed the PR, otherwise this field returns "REVIEW_REQUIRED" or "CHANGES_REQUSTED". Given there is a no longer a requirement for events on github PRs, we don't need to calculate if the changeset is older than events before computing the state. ## Test plan CI passing, tested locally with a repo on ghe.sgdev.org with a codeowners file (cherry picked from commit 6ea307e)

Add changelog entry for https://github.com/sourcegraph/sourcegraph/pull/53601 ## Test plan CI

Add changelog entry for https://github.com/sourcegraph/sourcegraph/pull/53601 ## Test plan CI (cherry picked from commit c2478af)

…wner review (#54174) Attempts to address https://github.com/sourcegraph/sourcegraph/issues/50874 By getting the "ReviewDecision" from a PR which is only "APPROVED" once a codeowner (if required) has reviewed the PR, otherwise this field returns "REVIEW_REQUIRED" or "CHANGES_REQUSTED". Given there is a no longer a requirement for events on github PRs, we don't need to calculate if the changeset is older than events before computing the state. ## Test plan CI passing, tested locally with a repo on ghe.sgdev.org with a codeowners file <br> Backport 6ea307e from #53601 Co-authored-by: Dave Try <davetry@gmail.com>

davejrt added 2 commits June 15, 2023 16:13

add mergeable_state

e761b9d

Merge branch 'main' into dt/batch_codeowners

a5ea52d

cla-bot Bot added the cla-signed label Jun 16, 2023

check mergable state

dc59ea9

davejrt force-pushed the dt/batch_codeowners branch from 4a3db93 to dc59ea9 Compare June 16, 2023 12:27

davejrt requested review from a team and eseliger June 16, 2023 13:38

davejrt added 3 commits June 21, 2023 12:45

add reviewDecision

3768f48

compute gh changeset state from reviewDecision

e13201c

ensure we check github metadata as source of truth

eca1ca1

davejrt marked this pull request as ready for review June 22, 2023 01:30

Merge branch 'main' into dt/batch_codeowners

082ffc4

davejrt force-pushed the dt/batch_codeowners branch from d411fad to 082ffc4 Compare June 22, 2023 11:29

BolajiOlajide approved these changes Jun 22, 2023

View reviewed changes

davejrt added 2 commits June 22, 2023 07:29

use new function for github

8a57cdf

consolidate functions, fix tests

d09823b

davejrt force-pushed the dt/batch_codeowners branch from 9e21cb3 to ebbbb58 Compare June 23, 2023 19:32

fix tests

74a5a60

davejrt force-pushed the dt/batch_codeowners branch from ebbbb58 to 74a5a60 Compare June 23, 2023 19:45

Merge branch 'main' into dt/batch_codeowners

8c61612

davejrt merged commit 6ea307e into main Jun 26, 2023

davejrt deleted the dt/batch_codeowners branch June 26, 2023 14:41

davejrt added EE/paper cuts customer issues that will be worked on as part of EE rotation backport 5.1 labels Jun 26, 2023

github-actions Bot mentioned this pull request Jun 26, 2023

[Backport 5.1] batch-changes: ensure PR is only approved after code owner review #54174

Merged

davejrt mentioned this pull request Jun 26, 2023

update changelog #54176

Merged

davejrt mentioned this pull request Jun 26, 2023

Batch Changes: Approved Status #50874

Closed

davejrt referenced this pull request Jun 26, 2023

update changelog (#54176)

c2478af

Add changelog entry for https://github.com/sourcegraph/sourcegraph/pull/53601 ## Test plan CI

github-actions Bot mentioned this pull request Jun 26, 2023

[Backport 5.1] update changelog #54180

Closed

github-actions Bot referenced this pull request Jun 26, 2023

update changelog (#54176)

fc6efb8

Add changelog entry for https://github.com/sourcegraph/sourcegraph/pull/53601 ## Test plan CI (cherry picked from commit c2478af)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batch-changes: ensure PR is only approved after code owner review#53601

batch-changes: ensure PR is only approved after code owner review#53601
davejrt merged 11 commits into
mainfrom
dt/batch_codeowners

davejrt commented Jun 16, 2023 •

edited

Loading

Uh oh!

davejrt commented Jun 16, 2023

Uh oh!

davejrt commented Jun 16, 2023 •

edited

Loading

Uh oh!

sourcegraph-bot commented Jun 22, 2023 •

edited

Loading

Uh oh!

BolajiOlajide left a comment

Uh oh!

Uh oh!

BolajiOlajide Jun 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

davejrt commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

davejrt commented Jun 16, 2023

Uh oh!

davejrt commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcegraph-bot commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BolajiOlajide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BolajiOlajide Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davejrt commented Jun 16, 2023 •

edited

Loading

davejrt commented Jun 16, 2023 •

edited

Loading

sourcegraph-bot commented Jun 22, 2023 •

edited

Loading