reviews: use timeline and ignore reviews prior to last push

common/gitea_utils.go

@@ -67,6 +67,10 @@ const (
 	ReviewStateUnknown models.ReviewStateType = ""
 )
 
+type GiteaTimelineFetcher interface {
+	GetTimeline(org, repo string, idx int64) ([]*models.TimelineComment, error)
+}
+
 type GiteaComment interface {
 	AddComment(pr *models.PullRequest, comment string) error
 }
@@ -81,6 +85,10 @@ type GiteaPRFetcher interface {
 	GetAssociatedPrjGitPR(prjGitOrg, prjGitRepo, refOrg, refRepo string, Index int64) (*models.PullRequest, error)
 }
 
+type GiteaCommitFetcher interface {
+	GetCommit(org, repo, sha string) (*models.Commit, error)
+}
+
 type GiteaReviewFetcher interface {
 	GetPullRequestReviews(org, project string, PRnum int64) ([]*models.PullReview, error)
 }
@@ -138,8 +146,10 @@ type Gitea interface {
 	GiteaReviewRequester
 	GiteaReviewer
 	GiteaPRFetcher
+	GiteaCommitFetcher
 	GiteaReviewFetcher
 	GiteaCommentFetcher
+	GiteaTimelineFetcher
 	GiteaMaintainershipReader
 	GiteaFileContentReader
 	GiteaCommitStatusGetter
@@ -290,6 +300,25 @@ func (gitea *GiteaTransport) GetPullRequestReviews(org, project string, PRnum in
 	return allReviews, nil
 }
 
+func (gitea *GiteaTransport) GetCommit(org, repo, sha string) (*models.Commit, error) {
+	f := false
+
+	r, err := gitea.client.Repository.RepoGetSingleCommit(
+		repository.NewRepoGetSingleCommitParams().
+			WithOwner(org).
+			WithRepo(repo).
+			WithSha(sha).
+			WithStat(&f).
+			WithFiles(&f).
+			WithVerification(&f),
+		gitea.transport.DefaultAuthentication)
+
+	if err != nil {
+		return nil, err
+	}
+	return r.Payload, nil
+}
+
 func (gitea *GiteaTransport) GetIssueComments(org, project string, issueNo int64) ([]*models.Comment, error) {
 	//	limit := int64(20)
 	//	var page int64
@@ -625,6 +654,41 @@ func (gitea *GiteaTransport) AddComment(pr *models.PullRequest, comment string)
 	return nil
 }
 
+func (gitea *GiteaTransport) GetTimeline(org, repo string, idx int64) ([]*models.TimelineComment, error) {
+	limit := int64(20)
+	page := int64(1)
+	resCount := limit
+
+	retData := []*models.TimelineComment{}
+
+	for resCount == limit {
+		res, err := gitea.client.Issue.IssueGetCommentsAndTimeline(
+			issue.NewIssueGetCommentsAndTimelineParams().
+				WithOwner(org).
+				WithRepo(repo).
+				WithIndex(idx).
+				WithPage(&page).
+				WithLimit(&limit),
+			gitea.transport.DefaultAuthentication,
+		)
+
+		if err != nil {
+			return nil, err
+		}
+
+		resCount = int64(len(res.Payload))
+		page++
+
+		retData = append(retData, res.Payload...)
+	}
+
+	slices.SortFunc(retData, func(a, b *models.TimelineComment) int {
+		return time.Time(a.Created).Compare(time.Time(b.Created))
+	})
+
+	return retData, nil
+}
+
 func (gitea *GiteaTransport) GetRepositoryFileContent(org, repo, hash, path string) ([]byte, string, error) {
 	params := repository.NewRepoGetContentsParams().WithOwner(org).WithRepo(repo).WithFilepath(path)
 	if len(hash) > 0 {

common/listen.go

@@ -149,7 +149,7 @@ func (l *ListenDefinitions) processRabbitMQ(msgCh chan<- RabbitMessage) error {
 	}
 	//	log.Printf("queue: %s:%d", q.Name, q.Consumers)
 
-	LogInfo(" -- listening to topics:")
+	LogDebug(" -- listening to topics:")
 	l.topicSubChanges = make(chan string)
 	defer close(l.topicSubChanges)
 	go l.processTopicChanges(ch, q.Name)

common/log.go

@@ -52,18 +52,38 @@ type LogLevel int
 
 const (
 	LogLevelNone  = 0
+	LogLevelError = 2
 	LogLevelInfo  = 5
 	LogLevelDebug = 10
 )
 
-var logLevel LogLevel
+var logLevel LogLevel = LogLevelInfo
 
 func SetLoggingLevel(ll LogLevel) {
 	logLevel = ll
 }
 
+func SetLoggingLevelFromString(ll string) error {
+	switch ll {
+	case "info":
+		SetLoggingLevel(LogLevelInfo)
+	case "debug":
+		SetLoggingLevel(LogLevelDebug)
+	case "error":
+		SetLoggingLevel(LogLevelError)
+	case "none":
+		SetLoggingLevel(LogLevelNone)
+	default:
+		return fmt.Errorf("Unknown logging level: %s", ll)
+	}
+
+	return nil
+}
+
 func LogError(params ...any) {
+	if logLevel >= LogLevelError {
 		log.Println(append([]any{"[E]"}, params...)...)
+	}
 }
 
 func LogDebug(params ...any) {

group-review/main.go

@@ -1,10 +1,13 @@
 package main
 
 import (
+	"encoding/json"
 	"flag"
+	"fmt"
 	"log"
 	"net/url"
 	"regexp"
+	"runtime/debug"
 	"slices"
 	"strconv"
 	"strings"
@@ -57,89 +60,191 @@ func ReviewRejected(reviewText string) bool {
 func ProcessNotifications(notification *models.NotificationThread, gitea common.Gitea) {
 	defer func() {
 		if r := recover(); r != nil {
-			log.Println("--- resovered")
+			common.LogInfo("panic cought --- recovered")
+			common.LogError(string(debug.Stack()))
 		}
 	}()
 
-	rx := regexp.MustCompile(`^/?api/v\d+/repos/(?<org>[_a-zA-Z0-9-]+)/(?<project>[_a-zA-Z0-9-]+)/issues/(?<num>[0-9]+)$`)
+	rx := regexp.MustCompile(`^/?api/v\d+/repos/(?<org>[_a-zA-Z0-9-]+)/(?<project>[_a-zA-Z0-9-]+)/(?:issues|pulls)/(?<num>[0-9]+)$`)
 	subject := notification.Subject
 	u, err := url.Parse(notification.Subject.URL)
 	if err != nil {
-		log.Panicln("Invalid format of notification: %s", subject.URL)
+		common.LogError("Invalid format of notification:", subject.URL, err)
+		return
 	}
+
 	match := rx.FindStringSubmatch(u.Path)
 	if match == nil {
-		log.Panicf("** Unexpected format of notification: %s", subject.URL)
+		common.LogError("** Unexpected format of notification:", subject.URL)
+		return
 	}
 
 	org := match[1]
 	repo := match[2]
 	id, _ := strconv.ParseInt(match[3], 10, 64)
 
-	log.Printf("processing: %s/%s#%d\n", org, repo, id)
+	common.LogInfo("processing:", fmt.Sprintf("%s/%s#%d", org, repo, id))
 	pr, err := gitea.GetPullRequest(org, repo, id)
 	if err != nil {
-		log.Println("  ** Cannot fetch PR associated with review:", subject.URL, "Error:", err)
+		common.LogError("  ** Cannot fetch PR associated with review:", subject.URL, "Error:", err)
+		return
+	}
+
+	commit, err := gitea.GetCommit(org, repo, pr.Head.Sha)
+	if err != nil {
+		common.LogError("  ** Cannot fetch head commit:", pr.Head.Sha, err)
 		return
 	}
 
 	config := configs.GetPrjGitConfig(org, repo, pr.Base.Name)
 	if pr.State == "closed" {
 		// dismiss the review
-		log.Println("  -- closed request, so nothing to review")
+		common.LogInfo("  -- closed request, so nothing to review")
+		if !common.IsDryRun {
 			gitea.SetNotificationRead(notification.ID)
+		}
 		return
 	}
 
 	reviews, err := gitea.GetPullRequestReviews(org, repo, id)
 	if err != nil {
-		log.Println("  ** No reviews associated with request:", subject.URL, "Error:", err)
+		common.LogInfo("  ** No reviews associated with request:", subject.URL, "Error:", err)
 		return
 	}
 
+	timeline, err := gitea.GetTimeline(org, repo, id)
+	if err != nil {
+		common.LogError(err)
+		return
+	}
+
+	idx := len(timeline) - 1
+
+	//{"is_force_push":true,"commit_ids":["36e43509be1b13a1a8fc63a4361405de04cc621ab16935f88968c46193221bb6","732246a48fbc6bac9df16c0b0ca23ce0f6fbabd9990795863b6d1f0ef3f242c8"]}
+	type PullPushData struct {
+		IsForcePush bool     `json:"is_force_push"`
+		CommitIds   []string `json:"commit_ids"`
+	}
+
+	for ; idx > 0; idx-- {
+		e := timeline[idx]
+		if e.Type == "pull_push" {
+			var push PullPushData
+			if err := json.Unmarshal([]byte(e.Body), &push); err != nil {
+				common.LogError(err)
+			}
+
+			if slices.Contains(push.CommitIds, pr.Head.Sha) {
+				break
+			}
+		}
+	}
+
+	timeline = timeline[idx:]
+
+	for _, t := range timeline {
+		common.LogDebug(t.Created, "->", t.Updated, ":", t.Body)
+		common.LogDebug("   Type:", t.Type)
+	}
+
 	requestReviewers, err := config.GetReviewGroupMembers(groupName)
 	if err != nil {
-		log.Println(err)
+		common.LogError(err)
 		return
 	}
 
+	valid_review_ids := make([]int64, 0, len(timeline))
+	for _, t := range timeline {
+		if t.Type == "review" {
+			valid_review_ids = append(valid_review_ids, t.ReviewID)
+		}
+	}
+
 	// submitter cannot be reviewer
 	requestReviewers = slices.DeleteFunc(requestReviewers, func(u string) bool { return u == pr.User.UserName })
+	//	pr.Head.Sha
+
+	common.LogDebug("   PR update time:", pr.Updated)
 
 	for _, review := range reviews {
 		user := review.User.UserName
+
+		if !slices.Contains(valid_review_ids, review.ID) {
+			continue
+		}
+
+		submit_time := time.Time(review.Submitted)
+		/*	pr_update := time.Time(pr.Updated)
+		 */
 		if !review.Stale && !review.Dismissed &&
 			slices.Contains(requestReviewers, user) {
+			/*
+				if review.CommitID != pr.Head.Sha {
+					common.LogDebug("review commitid", review.CommitID, "is not up-to-date with HEAD", pr.Head.Sha)
+					common.LogDebug("ignoring review by", review.User.UserName, "at", review.Submitted)
+					continue
+				}
+			*/
+			common.LogDebug("Created: ", commit.Created)
+			common.LogDebug("Submit:  ", submit_time)
+			if submit_time.Compare(time.Time(commit.Created)) < 0 {
+				common.LogDebug("   ignoring. Review predates commit time. Review Submit:", submit_time, "created:", commit.Created)
+				continue
+			}
 
 			if review.State == common.ReviewStateApproved && ReviewAccepted(review.Body) {
+				if !common.IsDryRun {
 					gitea.AddReviewComment(pr, common.ReviewStateApproved, "Signed off by: "+user)
+					if !common.IsDryRun {
 						if err := gitea.SetNotificationRead(notification.ID); err != nil {
-					log.Println("  Cannot set notification as read", err)
+							common.LogDebug("  Cannot set notification as read", err)
 						}
-				log.Println("  -> approved by", user)
+					}
+				}
+				common.LogInfo("  -> approved by", user)
+				common.LogInfo("     review at", submit_time)
 				return
 			} else if review.State == common.ReviewStateRequestChanges && ReviewRejected(review.Body) {
+				if !common.IsDryRun {
 					gitea.AddReviewComment(pr, common.ReviewStateRequestChanges, "Request changes. See review by: "+user)
 					if err := gitea.SetNotificationRead(notification.ID); err != nil {
-					log.Println("  Cannot set notification as read", err)
+						common.LogDebug("  Cannot set notification as read", err)
 					}
-				log.Println("  -> declined by", user)
+				}
+				common.LogInfo("  -> declined by", user)
 				return
 			}
 		}
 	}
 
 	// request group member reviews, if missing
-	log.Println("  Requesting reviews for:", requestReviewers)
+	common.LogDebug("  Review incomplete...")
+	requestReviewers = slices.DeleteFunc(requestReviewers, func(user string) bool {
+		for _, r := range pr.RequestedReviewers {
+			if r.UserName == user {
+				return true
+			}
+		}
+		return false
+	})
+	if len(requestReviewers) > 0 {
+		common.LogDebug("  Requesting reviews for:", requestReviewers)
+		if !common.IsDryRun {
 			if _, err := gitea.RequestReviews(pr, requestReviewers...); err != nil {
-		log.Println("   -> err:", err)
+				common.LogDebug("   -> err:", err)
+			} else {
+				common.LogDebug("   ^^^ not done")
+			}
+		}
+	} else {
+		common.LogDebug("  Not requesting additional reviewers:", requestReviewers)
 	}
 }
 
 func PeriodReviewCheck(gitea common.Gitea) {
 	notifications, err := gitea.GetPullNotifications(nil)
 	if err != nil {
-		log.Println(" EEE  Error fetching unread notifications: %w", err)
+		common.LogError("   Error fetching unread notifications: %w", err)
 		return
 	}
 
@@ -154,6 +259,8 @@ func main() {
 	rabbitMqHost := flag.String("rabbit-host", "rabbit.opensuse.org", "RabbitMQ instance where Gitea webhook notifications are sent")
 	interval := flag.Int64("interval", 5, "Notification polling interval in minutes (min 1 min)")
 	configFile := flag.String("config", "", "PrjGit listing config file")
+	logging := flag.String("logging", "info", "Logging level: [none, error, info, debug]")
+	flag.BoolVar(&common.IsDryRun, "dry", false, "Dry run, no effect. For debugging")
 	flag.Parse()
 
 	args := flag.Args()
@@ -166,29 +273,43 @@ func main() {
 	}
 	groupName = args[0]
 
+	if *configFile == "" {
+		common.LogError("Missing config file")
+		return
+	}
+
 	configData, err := common.ReadConfigFile(*configFile)
 	if err != nil {
-		log.Println("Failed to read config file", err)
+		common.LogError("Failed to read config file", err)
 		return
 	}
 
 	if err := common.RequireGiteaSecretToken(); err != nil {
-		log.Panicln(err)
+		common.LogError(err)
+		return
 	}
 
 	if err := common.RequireRabbitSecrets(); err != nil {
-		log.Panicln(err)
+		common.LogError(err)
+		return
 	}
 
 	gitea := common.AllocateGiteaTransport(*giteaUrl)
 	configs, err = common.ResolveWorkflowConfigs(gitea, configData)
 	if err != nil {
-		log.Panicln(err)
+		common.LogError("Cannot parse workflow configs:", err)
+		return
 	}
 
 	reviewer, err := gitea.GetCurrentUser()
 	if err != nil {
-		log.Panicln("Cannot fetch review user: %w", err)
+		common.LogError("Cannot fetch review user:", err)
+		return
+	}
+
+	if err := common.SetLoggingLevelFromString(*logging); err != nil {
+		common.LogError(err.Error())
+		return
 	}
 
 	if *interval < 1 {
@@ -197,31 +318,66 @@ func main() {
 
 	InitRegex(groupName)
 
-	log.Println(" ** processing group reviews for group:", groupName)
+	common.LogInfo(" ** processing group reviews for group:", groupName)
-	log.Println(" ** username in Gitea:", reviewer.UserName)
+	common.LogInfo(" ** username in Gitea:", reviewer.UserName)
-	log.Println(" ** polling internval:", *interval, "min")
+	common.LogInfo(" ** polling interval:", *interval, "min")
-	log.Println(" ** connecting to RabbitMQ:", *rabbitMqHost)
+	common.LogInfo(" ** connecting to RabbitMQ:", *rabbitMqHost)
 
 	if groupName != reviewer.UserName {
-		log.Println(" ***** Reviewer does not match group name. Aborting. *****")
+		common.LogError(" ***** Reviewer does not match group name. Aborting. *****")
 		return
 	}
 
 	u, err := url.Parse("amqps://" + *rabbitMqHost)
 	if err != nil {
-		log.Panicln("Cannot parse RabbitMQ host:", err)
+		common.LogError("Cannot parse RabbitMQ host:", err)
+		return
 	}
 
-	configUpdates := &common.ListenDefinitions {
+	config_update := ConfigUpdatePush{
+		config_modified: make(chan *common.AutogitConfig),
+	}
+
+	configUpdates := &common.ListenDefinitions{
 		RabbitURL: u,
 		Orgs:      []string{},
-		Handlers: map[string]common.RequestProcessor {
+		Handlers: map[string]common.RequestProcessor{
-			common.RequestType_Push: &ConfigUpdatePush{},
+			common.RequestType_Push: &config_update,
 		},
 	}
+	for _, c := range configs {
+		if org, _, _ := c.GetPrjGit(); !slices.Contains(configUpdates.Orgs, org) {
+			configUpdates.Orgs = append(configUpdates.Orgs, org)
+		}
+	}
 	go configUpdates.ProcessRabbitMQEvents()
 
 	for {
+	config_update_loop:
+		for {
+			select {
+			case configTouched, ok := <-config_update.config_modified:
+				if ok {
+					for idx, c := range configs {
+						if c == configTouched {
+							org, repo, branch := c.GetPrjGit()
+							prj := fmt.Sprintf("%s/%s#%s", org, repo, branch)
+							common.LogInfo("Detected config update for", prj)
+
+							new_config, err := common.ReadWorkflowConfig(gitea, prj)
+							if err != nil {
+								common.LogError("Failed parsing Project config for", prj, err)
+							} else {
+								configs[idx] = new_config
+							}
+						}
+					}
+				}
+			default:
+				break config_update_loop
+			}
+		}
+
 		PeriodReviewCheck(gitea)
 		time.Sleep(time.Duration(*interval * int64(time.Minute)))
 	}

group-review/rabbit.go

@@ -7,8 +7,8 @@ import (
 	"src.opensuse.org/autogits/common"
 )
 
-type ConfigUpdatePush struct{
+type ConfigUpdatePush struct {
-
+	config_modified chan *common.AutogitConfig
 }
 
 func (s *ConfigUpdatePush) ProcessFunc(req *common.Request) error {
@@ -44,7 +44,13 @@ func (s *ConfigUpdatePush) ProcessFunc(req *common.Request) error {
 			slices.Contains(commit.Removed, common.ProjectConfigFile)
 	}
 
-	s.config_modified <- 
+	if modified_config {
+		for _, config := range configs {
+			if o, r, _ := config.GetPrjGit(); o == org && r == repo {
+				s.config_modified <- config
+			}
+		}
+	}
 
 	return nil
 }