fix: do not check if issue has affects label and refine status desc (#…

…888)

docs/en/plugins/issue-triage.md

@@ -26,7 +26,7 @@ For critical or major severity bug issues.
 
 For pull requests that fix related bug issues and linked with them, we add a check named `check-issue-triage-complete` that will ensure that the bug issues are triaged before the pull request is merged.
 
-By default, the plugin will trigger the check at the right time, and if it doesn't, contributors can trigger it manually with the `/run-check-issue-triage-complete` command.
+By default, the plugin will trigger the check at the right time, and if it doesn't, contributors can trigger it manually with the `/run-check-issue-triage-complete` or `/check-issue-triage-complete` command.
 
 The plugin determines whether a pull request is triaged according to the following rules.
 

docs/plugins/issue-triage.md

@@ -26,7 +26,7 @@ ti-community-issue-triage 插件将被设计来对以上流程进行管控和自
 
 对于修复相关 bug issue 并与之关联的 pull request 而言，我们会添加一个名为 `check-issue-triage-complete` 的检查项，该检查将会确保 bug issue 在 pull request 合并之前完成 triage。
 
-默认情况下，插件会在合适的时机来触发该检查，如果没有触发成功，Contributor 可以通过 `/run-check-issue-triage-complete` 命令进行手动触发。
+默认情况下，插件会在合适的时机来触发该检查，如果没有触发成功，Contributor 可以通过 `/run-check-issue-triage-complete` 或 `/check-issue-triage-complete` 命令进行手动触发。
 
 插件会根据如下规则来判断 pull request 是否完成 triaged：
 

internal/pkg/externalplugins/issuetriage/issuetriage.go

@@ -26,19 +26,30 @@ import (
 const PluginName = "ti-community-issue-triage"
 
 const (
-	bugTypeLabel          = "type/bug"
+	typeLabelPrefix = "type/"
+	bugTypeLabel    = "type/bug"
+
+	severityLabelPrefix   = "severity/"
 	majorSeverityLabel    = "severity/major"
 	criticalSeverityLabel = "severity/critical"
-	severityLabelPrefix   = "severity/"
 
-	issueNeedTriagedContextName      = "check-issue-triage-complete"
-	issueTriageContextMessageSuccess = "All linked bug issues have been triaged complete."
-	issueTriageContextMessagePending = "Bug issues need to triage completed before merge."
+	maxStatusDescriptionLength = 140
+)
+
+const (
+	issueNeedTriagedContextName = "check-issue-triage-complete"
+
+	statusDescAllBugIssueTriaged     = "All linked bug issues have been triaged complete."
+	statusDescNoLinkedIssue          = "No linked issue found."
+	statusDescFailedToGetIssue       = "Failed to get issue %s/%s#%d."
+	statusDescTypeLabelNotFound      = "Can not find any type label on issue %s/%s#%d."
+	statusDescSeverityLabelNotFound  = "Can not find any severity label on bug issue %s/%s#%d."
+	statusDescMayAffectsLabelBeFound = "Found may-affects label on bug issue %s/%s#%d."
 )
 
 var (
 	IssueNumberLineRe   = regexp.MustCompile("(?im)^Issue Number:.+")
-	checkIssueTriagedRe = regexp.MustCompile(`(?mi)^/(run-)*check-issue-triage-complete\s*$`)
+	checkIssueTriagedRe = regexp.MustCompile(`(?mi)^/(run-)?check-issue-triage-complete\s*$`)
 )
 
 type githubClient interface {
@@ -345,6 +356,11 @@ func (s *Server) handleIssueEvent(ie *github.IssueEvent, log *logrus.Entry) erro
 		if err != nil {
 			return err
 		}
+		if len(prs) == 0 {
+			log.Infof("Can not find any reference PRs.")
+			return nil
+		}
+		log.Infof("Found %d reference PRs.", len(prs))
 
 		// Notice: wait a few seconds to reduce API consumption in case of batched label operations.
 		time.Sleep(5 * time.Second)
@@ -359,27 +375,34 @@ func (s *Server) handleIssueEvent(ie *github.IssueEvent, log *logrus.Entry) erro
 
 		var errs []error
 		for _, pr := range prs {
-			issueNumberLine := IssueNumberLineRe.FindString(string(pr.Body))
-			linkedIssueNumbers := utils.NormalizeIssueNumbers(issueNumberLine, org, repo)
-
 			defaultBranch := string(pr.Repository.DefaultBranchRef.Name)
 			prOrg := string(pr.Repository.Owner.Login)
 			prRepo := string(pr.Repository.Name)
 			prNum := int(pr.Number)
 			prState := string(pr.State)
 			prSHA := string(pr.HeadRefOid)
 			prBranch := string(pr.BaseRefName)
+			log = log.WithFields(logrus.Fields{
+				"org":  prOrg,
+				"repo": prRepo,
+				"num":  prNum,
+			})
 
 			if !isPRNeedToCheck(prState, prBranch, defaultBranch) {
 				log.Debugf("Skipping the check, because it is not a open PR on the default branch.")
 				continue
 			}
+			log.Infof("Checking triggered by issue %s/%s#%d.", org, repo, num)
 
 			prLabels := sets.NewString()
 			for _, node := range pr.Labels.Nodes {
 				prLabels.Insert(string(node.Name))
 			}
 
+			issueNumberLine := IssueNumberLineRe.FindString(string(pr.Body))
+			linkedIssueNumbers := utils.NormalizeIssueNumbers(issueNumberLine, org, repo)
+			log.Infof("Found %d linked issues.", len(linkedIssueNumbers))
+
 			err := s.handle(log, cfg, prOrg, prRepo, prSHA, prNum, prLabels, linkedIssueNumbers, issues)
 			if err != nil {
 				errs = append(errs, err)
@@ -403,11 +426,17 @@ func (s *Server) handlePullRequestEvent(pe *github.PullRequestEvent, log *logrus
 	prRepo := pe.Repo.Name
 	prSHA := pr.Head.SHA
 	prNum := pr.Number
+	log = log.WithFields(logrus.Fields{
+		"org":  prOrg,
+		"repo": prRepo,
+		"num":  prNum,
+	})
 
 	if !isPRNeedToCheck(pr.State, pr.Base.Ref, pe.Repo.DefaultBranch) {
-		log.Debugf("Skipping the check, because it is not a open PR on the default branch.")
+		log.Debug("Skipping the check, because it is not a open PR on the default branch.")
 		return nil
 	}
+	log.Infof("Checking triggered by PR %s/%s#%d.", prOrg, prRepo, prNum)
 
 	prLabels := sets.NewString()
 	for _, label := range pr.Labels {
@@ -417,6 +446,7 @@ func (s *Server) handlePullRequestEvent(pe *github.PullRequestEvent, log *logrus
 	cfg := s.ConfigAgent.Config().IssueTriageFor(prOrg, prRepo)
 	issueNumberLine := IssueNumberLineRe.FindString(pr.Body)
 	linkedIssueNumbers := utils.NormalizeIssueNumbers(issueNumberLine, prOrg, prRepo)
+	log.Infof("Found %d linked issues.", len(linkedIssueNumbers))
 
 	return s.handle(log, cfg, prOrg, prRepo, prSHA, prNum, prLabels, linkedIssueNumbers, issueCache{})
 }
@@ -432,9 +462,15 @@ func (s *Server) handleIssueCommentEvent(ice *github.IssueCommentEvent, log *log
 	prOrg := ice.Repo.Owner.Login
 	prRepo := ice.Repo.Name
 	prNum := issue.Number
+	log = log.WithFields(logrus.Fields{
+		"org":        prOrg,
+		"repo":       prRepo,
+		"num":        prNum,
+		"comment_id": ice.Comment.ID,
+	})
 
 	if !checkIssueTriagedRe.MatchString(comment) {
-		log.Debugf("Skipping the check, because no command comment.")
+		log.Debug("Skipping the check, because no command comment.")
 		return nil
 	}
 
@@ -444,9 +480,10 @@ func (s *Server) handleIssueCommentEvent(ice *github.IssueCommentEvent, log *log
 	}
 
 	if !isPRNeedToCheck(pr.State, pr.Base.Ref, ice.Repo.DefaultBranch) {
-		log.Debugf("Skipping the check because it is not a open PR on the default branch.")
+		log.Debug("Skipping the check because it is not a open PR on the default branch.")
 		return nil
 	}
+	log.Infof("Checking triggered by comment %s/%s#%d:%d.", prOrg, prRepo, prNum, ice.Comment.ID)
 
 	prSHA := pr.Head.SHA
 	prLabels := sets.NewString()
@@ -457,6 +494,7 @@ func (s *Server) handleIssueCommentEvent(ice *github.IssueCommentEvent, log *log
 	cfg := s.ConfigAgent.Config().IssueTriageFor(prOrg, prRepo)
 	issueNumberLine := IssueNumberLineRe.FindString(pr.Body)
 	linkedIssueNumbers := utils.NormalizeIssueNumbers(issueNumberLine, prOrg, prRepo)
+	log.Infof("Found %d linked issues.", len(linkedIssueNumbers))
 
 	return s.handle(log, cfg, prOrg, prRepo, prSHA, prNum, prLabels, linkedIssueNumbers, issueCache{})
 }
@@ -480,26 +518,29 @@ func (s *Server) handle(log *logrus.Entry, cfg *tiexternalplugins.TiCommunityIss
 	lock.Lock()
 	defer lock.Unlock()
 
-	existingStatus, err := s.checkExistingStatus(log, prOrg, prRepo, prSHA)
+	existingStatus, existingDesc, existingURL, err := s.checkExistingStatus(log, prOrg, prRepo, prSHA)
 	if err != nil {
 		return err
 	}
 
-	allTriaged, affectsVersionLabels, err := s.checkLinkedIssues(cfg, issueKeys, issueCache)
+	wantStatus, wantDesc, affectsVersionLabels, err := s.checkLinkedIssues(cfg, issueKeys, issueCache)
 	if err != nil {
 		return err
 	}
+	log.Infof("Checking result: %s, found affects labels: %v", strings.ToLower(wantDesc), affectsVersionLabels)
 
-	// Notice: all triaged means all the linked issues needed to be triaged have triaged complete, if a PR
-	// has no issues that require triaged, the check will pass too.
-	if allTriaged {
+	// Notice: check pass when all the linked issues needed to be triaged have triaged complete,
+	// if a PR has no linked bug issues, the check will pass too.
+	if wantStatus == github.StatusSuccess {
+		// Remove the need triaged label.
 		if len(cfg.NeedTriagedLabel) != 0 && prLabels.Has(cfg.NeedTriagedLabel) {
 			err := s.GitHubClient.RemoveLabel(prOrg, prRepo, prNum, cfg.NeedTriagedLabel)
 			if err != nil {
 				return err
 			}
 		}
 
+		// Add needs-cherry-pick label.
 		labelsNeedToAdd := make([]string, 0)
 		for _, affectsVersionLabel := range affectsVersionLabels.List() {
 			affectVersion := strings.TrimPrefix(affectsVersionLabel, cfg.AffectsLabelPrefix)
@@ -509,30 +550,26 @@ func (s *Server) handle(log *logrus.Entry, cfg *tiexternalplugins.TiCommunityIss
 			}
 		}
 
-		err := s.createStatus(log, prOrg, prRepo, prSHA, existingStatus,
-			github.StatusSuccess, issueTriageContextMessageSuccess, cfg.StatusTargetURL)
-		if err != nil {
-			return err
-		}
-
 		if len(labelsNeedToAdd) > 0 {
 			err := s.GitHubClient.AddLabels(prOrg, prRepo, prNum, labelsNeedToAdd...)
 			if err != nil {
 				return err
 			}
 		}
-		return nil
-	}
-
-	if len(cfg.NeedTriagedLabel) != 0 && !prLabels.Has(cfg.NeedTriagedLabel) {
-		err := s.GitHubClient.AddLabel(prOrg, prRepo, prNum, cfg.NeedTriagedLabel)
-		if err != nil {
-			return err
+	} else if wantStatus == github.StatusError || wantStatus == github.StatusFailure {
+		// Add the need triaged label.
+		if len(cfg.NeedTriagedLabel) != 0 && !prLabels.Has(cfg.NeedTriagedLabel) {
+			err := s.GitHubClient.AddLabel(prOrg, prRepo, prNum, cfg.NeedTriagedLabel)
+			if err != nil {
+				return err
+			}
 		}
 	}
 
-	return s.createStatus(log, prOrg, prRepo, prSHA, existingStatus,
-		github.StatusPending, issueTriageContextMessagePending, cfg.StatusTargetURL)
+	// Update status.
+	wantDesc = truncatedStatusDesc(log, wantDesc)
+	return s.createStatus(log, prOrg, prRepo, prSHA, existingStatus, existingDesc, existingURL,
+		wantStatus, wantDesc, cfg.StatusTargetURL)
 }
 
 // isPRNeedToCheck used to determine if PR needs to be checked.
@@ -543,13 +580,22 @@ func isPRNeedToCheck(state, prBranch, defaultBranch string) bool {
 
 // checkLinkedIssues used to check if a given set of bug issues have all been triaged.
 func (s *Server) checkLinkedIssues(cfg *tiexternalplugins.TiCommunityIssueTriage,
-	issueKeys []utils.IssueNumberData, issueCache issueCache) (bool, sets.String, error) {
+	issueKeys []utils.IssueNumberData, issueCache issueCache) (string, string, sets.String, error) {
 	affectsVersionLabels := sets.NewString()
 
+	if len(issueKeys) == 0 {
+		return github.StatusSuccess, statusDescNoLinkedIssue, affectsVersionLabels, nil
+	}
+
 	for _, issueKey := range issueKeys {
-		issue, err := s.getIssueWithCache(issueCache, issueKey.Org, issueKey.Repo, issueKey.Number)
+		org := issueKey.Org
+		repo := issueKey.Repo
+		num := issueKey.Number
+
+		issue, err := s.getIssueWithCache(issueCache, org, repo, num)
 		if err != nil {
-			return false, sets.String{}, err
+			desc := fmt.Sprintf(statusDescFailedToGetIssue, org, repo, num)
+			return github.StatusFailure, desc, sets.String{}, err
 		}
 
 		labels := sets.NewString()
@@ -560,63 +606,75 @@ func (s *Server) checkLinkedIssues(cfg *tiexternalplugins.TiCommunityIssueTriage
 			}
 		}
 
+		// Issue without type label is not triaged.
+		if !hasAnyTypeLabel(issue) {
+			desc := fmt.Sprintf(statusDescTypeLabelNotFound, org, repo, num)
+			return github.StatusError, desc, sets.String{}, nil
+		}
+
 		// Only bug issues need to be checked.
 		if !labels.Has(bugTypeLabel) {
 			continue
 		}
 
 		// Bug issue must have severity label, if not, it will be considered to triage.
 		if !hasAnySeverityLabel(issue) {
-			return false, sets.String{}, nil
+			desc := fmt.Sprintf(statusDescSeverityLabelNotFound, org, repo, num)
+			return github.StatusError, desc, sets.String{}, nil
 		}
 
 		// Only major or critical bug issues need to be checked.
 		if !labels.HasAny(majorSeverityLabel, criticalSeverityLabel) {
 			continue
 		}
 
-		// Check if issue has any may-affects labels or no affects label, if any issue have
+		// Check if issue has any may-affects labels, if any issue have
 		// not yet been triaged, the checker will fail.
-		if !hasAnyAffectsLabel(issue, cfg.AffectsLabelPrefix) ||
-			hasAnyMayAffectsLabel(issue, cfg.MayAffectsLabelPrefix) {
-			return false, sets.String{}, nil
+		if hasAnyMayAffectsLabel(issue, cfg.MayAffectsLabelPrefix) {
+			desc := fmt.Sprintf(statusDescMayAffectsLabelBeFound, org, repo, num)
+			return github.StatusError, desc, sets.String{}, nil
 		}
 	}
-	return true, affectsVersionLabels, nil
+
+	return github.StatusSuccess, statusDescAllBugIssueTriaged, affectsVersionLabels, nil
 }
 
 // checkExistingStatus will retrieve the current status of the linked issue need triaged context
 // for the provided SHA.
-func (s *Server) checkExistingStatus(l *logrus.Entry, org, repo, sha string) (string, error) {
+func (s *Server) checkExistingStatus(l *logrus.Entry, org, repo, sha string) (string, string, string, error) {
 	combinedStatus, err := s.GitHubClient.GetCombinedStatus(org, repo, sha)
 	if err != nil {
-		return "", fmt.Errorf("error listing pull request combined statuses: %w", err)
+		return "", "", "", fmt.Errorf("error listing pull request combined statuses: %w", err)
 	}
 
 	existingStatus := ""
+	existingDesc := ""
+	existingURL := ""
 	for _, status := range combinedStatus.Statuses {
 		if status.Context != issueNeedTriagedContextName {
 			continue
 		}
 		existingStatus = status.State
+		existingDesc = status.Description
+		existingURL = status.TargetURL
 		break
 	}
 	l.Debugf("Existing linked issue need triaged status context status is %q", existingStatus)
-	return existingStatus, nil
+	return existingStatus, existingDesc, existingURL, nil
 }
 
-func hasAnySeverityLabel(issue *github.Issue) bool {
+func hasAnyTypeLabel(issue *github.Issue) bool {
 	for _, label := range issue.Labels {
-		if strings.HasPrefix(label.Name, severityLabelPrefix) {
+		if strings.HasPrefix(label.Name, typeLabelPrefix) {
 			return true
 		}
 	}
 	return false
 }
 
-func hasAnyAffectsLabel(issue *github.Issue, affectsLabelPrefix string) bool {
+func hasAnySeverityLabel(issue *github.Issue) bool {
 	for _, label := range issue.Labels {
-		if strings.HasPrefix(label.Name, affectsLabelPrefix) {
+		if strings.HasPrefix(label.Name, severityLabelPrefix) {
 			return true
 		}
 	}
@@ -673,21 +731,30 @@ func (s *Server) getReferencePRList(log *logrus.Entry, org, repo string, issueNu
 	return ret, nil
 }
 
-func (s *Server) createStatus(log *logrus.Entry, org, repo, sha, existingState,
-	targetState, message, targetURL string) error {
-	if existingState != targetState {
-		log.Debugf("Setting check-issue-triage-complete status context to %s.", targetState)
+func (s *Server) createStatus(log *logrus.Entry, org, repo, sha, existingState, existingDesc, existingURL,
+	wantState, wantDesc, wantURL string) error {
+	if existingState != wantState || existingDesc != wantDesc || existingURL != wantURL {
+		log.Debugf("Setting check-issue-triage-complete status context to %s.", wantState)
 		if err := s.GitHubClient.CreateStatus(org, repo, sha, github.Status{
 			Context:     issueNeedTriagedContextName,
-			State:       targetState,
-			TargetURL:   targetURL,
-			Description: message,
+			State:       wantState,
+			TargetURL:   wantURL,
+			Description: wantDesc,
 		}); err != nil {
 			return fmt.Errorf("error setting pull request status: %w", err)
 		}
 	} else {
-		log.Debugf("The check-issue-triage-complete status context is already %s.", targetState)
+		log.Debugf("The check-issue-triage-complete status context is already %s.", wantState)
 	}
 
 	return nil
 }
+
+func truncatedStatusDesc(log *logrus.Entry, wantDesc string) string {
+	if len(wantDesc) > maxStatusDescriptionLength {
+		original := wantDesc
+		wantDesc = fmt.Sprintf("%s...", wantDesc[0:(maxStatusDescriptionLength-3)])
+		log.WithField("original-desc", original).Warn("GitHub status description needed to be truncated to fit GH API limit")
+	}
+	return wantDesc
+}