git-workflow/autogits
SHA256
22
2
Fork 15
Code Issues 44 Pull Requests 14 Actions Projects 2 Activity

pending-reviews #142

Closed
adamm wants to merge 2 commits from pending-reviews into main
pull from: pending-reviews
merge into: git-workflow:main
Conversation 3 Commits 2 Files Changed 8 +112 -47
8 changed files with 112 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
common/pr.go
View File

@@ -468,7 +468,7 @@ func (rs *PRSet) IsApproved(gitea GiteaPRChecker, maintainers MaintainershipData
LogError("Cannot fetch gita reaviews for PR:", err)
return false
}
r.RequestedReviewers = reviewers
r.SetRequiredReviewers(reviewers)
prjgit.Reviews = r
if prjgit.Reviews.IsManualMergeOK() {
is_manually_reviewed_ok = true
@@ -489,7 +489,7 @@ func (rs *PRSet) IsApproved(gitea GiteaPRChecker, maintainers MaintainershipData
LogError("Cannot fetch gita reaviews for PR:", err)
return false
}
r.RequestedReviewers = reviewers
r.SetRequiredReviewers(reviewers)
pr.Reviews = r
if !pr.Reviews.IsManualMergeOK() {
LogInfo("Not approved manual merge. PR:", pr.PR.URL)
@@ -530,7 +530,7 @@ func (rs *PRSet) IsApproved(gitea GiteaPRChecker, maintainers MaintainershipData
LogError("Cannot fetch gitea reaviews for PR:", err)
return false
}
r.RequestedReviewers = reviewers
r.SetRequiredReviewers(reviewers)
is_manually_reviewed_ok = r.IsApproved()
LogDebug("PR to", pr.PR.Base.Repo.Name, "reviewed?", is_manually_reviewed_ok)

32
common/pr_test.go
View File

@@ -807,9 +807,8 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
Base: &models.PRBranchInfo{Name: "main", Repo: &models.Repository{Name: "pkg", Owner: &models.User{UserName: "org"}}},
},
Reviews: &common.PRReviews{
Reviews: []*models.PullReview{{State: common.ReviewStateRequestReview, User: &models.User{UserName: "m1"}}},
RequestedReviewers: []string{"m1"},
FullTimeline: []*models.TimelineComment{
Reviews: []*models.PullReview{{State: common.ReviewStateRequestReview, User: &models.User{UserName: "m1"}}},
RequestedReviewers: []*models.TimelineComment{
{User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "m1"}, Type: common.TimelineCommentType_ReviewRequested},
},
},
@@ -919,8 +918,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
},
Reviews: &common.PRReviews{
Reviews: []*models.PullReview{{State: common.ReviewStateRequestReview, User: &models.User{UserName: "reviewer"}}},
RequestedReviewers: []string{"reviewer"},
FullTimeline: []*models.TimelineComment{{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "reviewer"}}},
RequestedReviewers: []*models.TimelineComment{{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "reviewer"}}},
},
},
{
@@ -930,8 +928,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
},
Reviews: &common.PRReviews{
Reviews: []*models.PullReview{{State: common.ReviewStateRequestReview, User: &models.User{UserName: "reviewer"}}},
RequestedReviewers: []string{"reviewer"},
FullTimeline: []*models.TimelineComment{{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "reviewer"}}},
RequestedReviewers: []*models.TimelineComment{{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "reviewer"}}},
},
},
},
@@ -966,8 +963,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
{State: common.ReviewStateApproved, User: &models.User{UserName: "pkgmaintainer"}},
{State: common.ReviewStatePending, User: &models.User{UserName: "prjmaintainer"}},
},
RequestedReviewers: []string{"user2", "pkgmaintainer", "prjmaintainer"},
FullTimeline: []*models.TimelineComment{
RequestedReviewers: []*models.TimelineComment{
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "user2"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "pkgmaintainer"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prjmaintainer"}},
@@ -985,8 +981,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
{State: common.ReviewStateRequestChanges, User: &models.User{UserName: "user1"}},
{State: common.ReviewStateRequestReview, User: &models.User{UserName: "autogits_obs_staging_bot"}},
},
RequestedReviewers: []string{"user1", "autogits_obs_staging_bot"},
FullTimeline: []*models.TimelineComment{
RequestedReviewers: []*models.TimelineComment{
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "user1"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "autogits_obs_staging_bot"}},
},
@@ -1026,8 +1021,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
{State: common.ReviewStatePending, User: &models.User{UserName: "prj2"}},
{State: common.ReviewStatePending, User: &models.User{UserName: "someother"}},
},
RequestedReviewers: []string{"user2", "pkgmaintainer", "prjmaintainer", "pkgm1", "pkgm2", "someother", "prj1", "prj2"},
FullTimeline: []*models.TimelineComment{
RequestedReviewers: []*models.TimelineComment{
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "pkgmaintainer"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prjmaintainer"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prj1"}},
@@ -1050,8 +1044,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
{State: common.ReviewStatePending, User: &models.User{UserName: "prj1"}},
{State: common.ReviewStatePending, User: &models.User{UserName: "prj2"}},
},
RequestedReviewers: []string{"user1", "autogits_obs_staging_bot", "prj1", "prj2"},
FullTimeline: []*models.TimelineComment{
RequestedReviewers: []*models.TimelineComment{
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "autogits_obs_staging_bot"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prj1"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prj2"}},
@@ -1090,8 +1083,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
{State: common.ReviewStateRequestReview, User: &models.User{UserName: "prj1"}},
{State: common.ReviewStateRequestReview, User: &models.User{UserName: "someother"}},
},
RequestedReviewers: []string{"user2", "pkgmaintainer", "prjmaintainer", "pkgm1", "someother", "prj1"},
FullTimeline: []*models.TimelineComment{
RequestedReviewers: []*models.TimelineComment{
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "pkgm1"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "pkgmaintainer"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prjmaintainer"}},
@@ -1112,8 +1104,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
{State: common.ReviewStateRequestReview, User: &models.User{UserName: "autogits_obs_staging_bot"}},
{State: common.ReviewStateRequestReview, User: &models.User{UserName: "prj1"}},
},
RequestedReviewers: []string{"user1", "autogits_obs_staging_bot", "prj1"},
FullTimeline: []*models.TimelineComment{
RequestedReviewers: []*models.TimelineComment{
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "autogits_obs_staging_bot"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prj1"}},
{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "!bot"}, Assignee: &models.User{UserName: "user1"}},
@@ -1199,6 +1190,9 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
test.prset.HasAutoStaging = !test.noAutoStaging
for idx, pr := range test.prset.PRs {
if pr.Reviews != nil {
pr.Reviews.SetRequiredReviewers(test.prset.Config.Reviewers)
}
missing, extra := test.prset.FindMissingAndExtraReviewers(test.maintainers, idx)
// avoid nil dereference below, by adding empty array elements

64
common/reviews.go
View File

@@ -8,12 +8,28 @@ import (
"src.opensuse.org/autogits/common/gitea-generated/models"
)
type ReviewInterface interface {
IsManualMergeOK() bool
IsApproved() bool
MisingReviews() []string
FindReviewRequester(reviewer string) *models.TimelineComment
HasPendingReviewBy(reviewer string) bool
IsReviewedBy(reviewer string) bool
IsReviewedByOneOf(reviewers ...string) bool
SetRequiredReviewers(reviewers []string)
}
type PRReviews struct {
Reviews []*models.PullReview
RequestedReviewers []string
RequestedReviewers []*models.TimelineComment
Comments []*models.TimelineComment
FullTimeline []*models.TimelineComment
RequiredReviewers []string
}
func (r *PRReviews) SetRequiredReviewers(reviewers []string) {
r.RequiredReviewers = reviewers
}
func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64) (*PRReviews, error) {
@@ -28,11 +44,11 @@ func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64
}
reviews := make([]*models.PullReview, 0, 10)
needNewReviews := []string{}
var comments []*models.TimelineComment
var foundUsers []string
alreadyHaveUserReview := func(user string) bool {
if slices.Contains(needNewReviews, user) {
if slices.Contains(foundUsers, user) {
return true
}
for _, r := range reviews {
@@ -48,20 +64,24 @@ func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64
LogDebug("Number of items in timeline:", len(timeline))
cutOffIdx := len(timeline)
var PendingRequestedReviews []*models.TimelineComment
for idx, item := range timeline {
if item.Type == TimelineCommentType_Review || item.Type == TimelineCommentType_ReviewRequested {
if item.Type == TimelineCommentType_Review {
for _, r := range rawReviews {
if r.ID == item.ReviewID && r.User != nil {
if !alreadyHaveUserReview(r.User.UserName) {
if item.Type == TimelineCommentType_Review && idx > cutOffIdx {
needNewReviews = append(needNewReviews, r.User.UserName)
} else {
if idx < cutOffIdx {
reviews = append(reviews, r)
}
foundUsers = append(foundUsers, r.User.UserName)
}
break
}
}
} else if item.Type == TimelineCommentType_ReviewRequested && item.Assignee != nil && !alreadyHaveUserReview(item.Assignee.UserName) {
PendingRequestedReviews = append(PendingRequestedReviews, item)
} else if item.Type == TimelineCommentType_DismissReview && item.Assignee != nil && !alreadyHaveUserReview(item.Assignee.UserName) {
foundUsers = append(foundUsers, item.Assignee.UserName)
} else if item.Type == TimelineCommentType_Comment && cutOffIdx > idx {
comments = append(comments, item)
} else if item.Type == TimelineCommentType_PushPull && cutOffIdx == len(timeline) {
@@ -74,9 +94,9 @@ func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64
LogDebug("num comments:", len(comments), "timeline:", len(reviews))
return &PRReviews{
Reviews: reviews,
Comments: comments,
FullTimeline: timeline,
Reviews: reviews,
Comments: comments,
RequestedReviewers: PendingRequestedReviews,
}, nil
}
@@ -104,7 +124,7 @@ func (r *PRReviews) IsManualMergeOK() bool {
continue
}
LogDebug("comment:", c.User.UserName, c.Body)
if slices.Contains(r.RequestedReviewers, c.User.UserName) {
if slices.Contains(r.RequiredReviewers, c.User.UserName) {
if bodyCommandManualMergeOK(c.Body) {
return true
}
@@ -115,7 +135,7 @@ func (r *PRReviews) IsManualMergeOK() bool {
if c.Updated != c.Submitted {
continue
}
if slices.Contains(r.RequestedReviewers, c.User.UserName) {
if slices.Contains(r.RequiredReviewers, c.User.UserName) {
if bodyCommandManualMergeOK(c.Body) {
return true
}
@@ -131,7 +151,7 @@ func (r *PRReviews) IsApproved() bool {
}
goodReview := true
for _, reviewer := range r.RequestedReviewers {
for _, reviewer := range r.RequiredReviewers {
goodReview = false
for _, review := range r.Reviews {
if review.User.UserName == reviewer && review.State == ReviewStateApproved && !review.Stale && !review.Dismissed {
@@ -155,7 +175,7 @@ func (r *PRReviews) MissingReviews() []string {
return missing
}
for _, reviewer := range r.RequestedReviewers {
for _, reviewer := range r.RequiredReviewers {
if !r.IsReviewedBy(reviewer) {
missing = append(missing, reviewer)
}
@@ -168,12 +188,11 @@ func (r *PRReviews) FindReviewRequester(reviewer string) *models.TimelineComment
return nil
}
for _, r := range r.FullTimeline {
if r.Type == TimelineCommentType_ReviewRequested && r.Assignee.UserName == reviewer {
return r
for _, t := range r.RequestedReviewers {
if t.Assignee.UserName == reviewer {
return t
}
}
return nil
}
@@ -193,6 +212,13 @@ func (r *PRReviews) HasPendingReviewBy(reviewer string) bool {
}
}
// at this point, we do not have actual review by user. Check if we have a pending review
for _, t := range r.RequestedReviewers {
if t.Assignee != nil && t.Assignee.UserName == reviewer {
return true
}
}
return false
}

49
common/reviews_test.go
View File

@@ -145,6 +145,53 @@ func TestReviews(t *testing.T) {
reviewers: []string{"user1"},
isApproved: false,
},
{
name: "ReviewRequested predates PushPull should be seen as pending",
reviews: []*models.PullReview{},
timeline: []*models.TimelineComment{
{Type: common.TimelineCommentType_PushPull},
{Type: common.TimelineCommentType_ReviewRequested, Assignee: &models.User{UserName: "user1"}},
},
reviewers: []string{"user1"},
isPendingByTest1: true,
},
{
name: "ReviewRequested postdates PushPull but blocked by older dismiss",
reviews: []*models.PullReview{},
timeline: []*models.TimelineComment{
{Type: common.TimelineCommentType_ReviewRequested, Assignee: &models.User{UserName: "user1"}},
{Type: common.TimelineCommentType_PushPull},
{Type: common.TimelineCommentType_ReviewDismissed, Assignee: &models.User{UserName: "user1"}},
},
reviewers: []string{"user1"},
isPendingByTest1: true,
},
{
name: "ReviewRequested predates PushPull should be seen as pending",
reviews: []*models.PullReview{
{ID: 101, State: common.ReviewStateRequestReview, User: &models.User{UserName: "user1"}},
},
timeline: []*models.TimelineComment{
{Type: common.TimelineCommentType_PushPull},
{Type: common.TimelineCommentType_ReviewRequested, Assignee: &models.User{UserName: "user1"}},
},
reviewers: []string{"user1"},
isPendingByTest1: true,
},
{
name: "Review requested, review, then push needs re-requesting",
reviews: []*models.PullReview{
{ID: 100, State: common.ReviewStateRequestChanges, User: &models.User{UserName: "user1"}},
},
timeline: []*models.TimelineComment{
{Type: common.TimelineCommentType_PushPull},
{Type: common.TimelineCommentType_Review, ReviewID: 100},
{Type: common.TimelineCommentType_ReviewRequested, Assignee: &models.User{UserName: "user1"}},
},
reviewers: []string{"user1"},
isReviewedByTest1: false, // Should be stale
isPendingByTest1: false, // Should be stale
},
}
for _, test := range tests {
@@ -166,7 +213,7 @@ func TestReviews(t *testing.T) {
}
return
}
reviews.RequestedReviewers = test.reviewers
reviews.SetRequiredReviewers(test.reviewers)
if r := reviews.IsApproved(); r != test.isApproved {
t.Fatal("Unexpected IsReviewed():", r, "vs. expected", test.isApproved)

1
common/timeline.go
View File

@@ -8,6 +8,7 @@ import (
)
const (
TimelineCommentType_ReviewDismissed = "dismiss_review"
TimelineCommentType_ReviewRequested = "review_request"
TimelineCommentType_Review = "review"
TimelineCommentType_PushPull = "pull_push"

4
integration/test-plan.md
View File

@@ -65,11 +65,11 @@ The testing will be conducted in a dedicated test environment that mimics the pr
| **TC-REVIEW-001** | P | **Add mandatory reviewers** | 1. Create a new PackageGit PR. | 1. All mandatory reviewers are added to both the PackageGit and ProjectGit PRs. | High |
| **TC-REVIEW-002** | - | **Add advisory reviewers** | 1. Create a new PackageGit PR with advisory reviewers defined in the configuration. | 1. Advisory reviewers are added to the PR, but their approval is not required for merging. | Medium |
| **TC-REVIEW-003** | - | **Re-add reviewers** | 1. Push a new commit to a PackageGit PR after it has been approved. | 1. The original reviewers are re-added to the PR. | Medium |
| **TC-REVIEW-004** | x | **Package PR created by a maintainer** | 1. Create a PackageGit PR from the account of a package maintainer. | 1. No review is requested from other package maintainers. | High |
| **TC-REVIEW-004** | P | **Package PR created by a maintainer** | 1. Create a PackageGit PR from the account of a package maintainer. | 1. No review is requested from other package maintainers. | High |
| **TC-REVIEW-005** | P | **Package PR created by an external user (approve)** | 1. Create a PackageGit PR from the account of a user who is not a package maintainer.<br>2. One of the package maintainers approves the PR. | 1. All package maintainers are added as reviewers.<br>2. Once one maintainer approves the PR, the other maintainers are removed as reviewers. | High |
| **TC-REVIEW-006** | P | **Package PR created by an external user (reject)** | 1. Create a PackageGit PR from the account of a user who is not a package maintainer.<br>2. One of the package maintainers rejects the PR. | 1. All package maintainers are added as reviewers.<br>2. Once one maintainer rejects the PR, the other maintainers are removed as reviewers. | High |
| **TC-REVIEW-007** | P | **Package PR created by a maintainer with ReviewRequired=true** | 1. Set `ReviewRequired = true` in `workflow.config`.<br>2. Create a PackageGit PR from the account of a package maintainer. | 1. A review is requested from other package maintainers if available. | High |
| **TC-MERGE-001** | x | **Automatic Merge** | 1. Create a PackageGit PR.<br>2. Ensure all mandatory reviews are completed on both project and package PRs. | 1. The PR is automatically merged. | High |
| **TC-MERGE-001** | P | **Automatic Merge** | 1. Create a PackageGit PR.<br>2. Ensure all mandatory reviews are completed on both project and package PRs. | 1. The PR is automatically merged. | High |
| **TC-MERGE-002** | - | **ManualMergeOnly with Package Maintainer** | 1. Create a PackageGit PR with `ManualMergeOnly` set to `true`.<br>2. Ensure all mandatory reviews are completed on both project and package PRs.<br>3. Comment "merge ok" on the package PR from the account of a package maintainer for that package. | 1. The PR is merged. | High |
| **TC-MERGE-003** | - | **ManualMergeOnly with unauthorized user** | 1. Create a PackageGit PR with `ManualMergeOnly` set to `true`.<br>2. Ensure all mandatory reviews are completed on both project and package PRs.<br>3. Comment "merge ok" on the package PR from the account of a user who is not a maintainer for that package. | 1. The PR is not merged. | High |
| **TC-MERGE-004** | - | **ManualMergeOnly with multiple packages** | 1. Create a ProjectGit PR that references multiple PackageGit PRs with `ManualMergeOnly` set to `true`.<br>2. Ensure all mandatory reviews are completed on both project and package PRs.<br>3. Comment "merge ok" on each package PR from the account of a package maintainer. | 1. The PR is merged only after "merge ok" is commented on all associated PackageGit PRs. | High |

1
integration/tests/workflow_pr_merge_test.py
View File

@@ -5,7 +5,6 @@ from pathlib import Path
from tests.lib.common_test_utils import GiteaAPIClient
@pytest.mark.t001
@pytest.mark.xfail(reason="The bot sometimes re-request reviews despite having all the approvals")
def test_001_automerge(automerge_env, test_user_client):
"""
Test scenario:

2
integration/tests/workflow_pr_review_test.py
View File

@@ -43,7 +43,6 @@ index 0000000..e69de29
@pytest.mark.t004
@pytest.mark.xfail(reason="the bot sometimes re-requests review from autogits_obs_staging_bot despite having the approval")
def test_004_maintainer(maintainer_env, ownerA_client):
"""
Test scenario:
@@ -152,7 +151,6 @@ index 0000000..e69de29
@pytest.mark.t005
# @pytest.mark.xfail(reason="TBD troubleshoot")
def test_005_any_maintainer_approval_sufficient(maintainer_env, ownerA_client, ownerBB_client):
"""
Test scenario: