Proposal: add issue number to PR title

ichn-hu · September 18, 2021, 9:49am

I already have that, see my previous issue tracker

ichn-hu/issue-tracker/blob/master/main.go#L244


				log.Fatal(err)
			}
		}


		// insert PRs
		if _, err := db.Exec("delete from close where issue_id = ?", issueId); err != nil {
			log.Fatal(err)
		}
		for _, edge := range issue.TimelineItems.Edges {
			event := edge.Node.CrossReferencedEvent
			if !event.WillCloseTarget {
				continue
			}
			pr := event.Source.PullRequest
			res, err := db.Exec("insert into pull_request (owner, repository, number, title, author, created_at, updated_at, state, url) values (?, ?, ?, ?, ?, ?, ?, ?, ?) on duplicate key update updated_at=?, title=?, state=?", pr.Repository.Owner.Login, pr.Repository.Name, pr.Number, pr.Title, pr.Author.Login, pr.CreatedAt.Time, pr.UpdatedAt.Time, pr.State, pr.Url, pr.UpdatedAt.Time, pr.Title, pr.State)
			if err != nil {
				log.Fatal("failed on insert PR", err)
			}
			prId, err := res.LastInsertId()
			if err != nil {
				log.Fatal(err)

Mini256 · September 18, 2021, 1:10pm

We can get the PR reference event on the timeline of an issue through graphsql query, but what’s disturbing is that we can’t get the referenced issue by querying a PR conversely.

Tide (the component responsible for merging PR) obtains some PR information through a graphsql query before generating the commit message. We can use the information to fill in the commit message template, but the current problem is that we cannot get the issue associated with PR.

github.com

kubernetes/test-infra/blob/224fa04eb34923d5c384d32c617fa65e0622ccb8/prow/tide/tide.go#L1810


		if err != nil {
			return nil, fmt.Errorf("failed to list jobs for subpool %s: %w", subpoolkey, err)
		}
		c.logger.WithField("subpool", subpoolkey).Debugf("Found %d prowjobs.", len(pjs.Items))
		sps[subpoolkey].pjs = pjs.Items
	}
	return sps, nil
}


// PullRequest holds graphql data about a PR, including its commits and their contexts.
type PullRequest struct {
	Number githubql.Int
	Author struct {
		Login githubql.String
	}
	BaseRef struct {
		Name   githubql.String
		Prefix githubql.String
	}
	HeadRefName githubql.String `graphql:"headRefName"`
	HeadRefOID  githubql.String `graphql:"headRefOid"`

Mini256 · September 18, 2021, 1:25pm

Amazing! It seems that GitHub has added this feature.

query { 
  repository (owner:"pingcap" name:"tidb") { 
    pullRequest(number: 28204) {
      closingIssuesReferences(first: 10) {
        nodes {
          number
        }
      }
    }
  }
}

tison · September 21, 2021, 7:14am

Can tide populate issue number from PR body in commit message in your opinion?

Mini256 · September 22, 2021, 2:56am

One problem is that the closingIssuesReferences field outputs the closing issue. I don’t think all PRs will close an issue.

Will there be some issues that cannot be directly closed by one PR, but need to pass multiple PRs before they can be closed?

I submitted a related PR, but the upstream reviewer will consider the issues about the increased API points.

Mini256 · September 22, 2021, 6:30am

@SunRunAway, @ichn-hu

I think it’s not a good way to use a bot to automatically determine which issue number should be filled in the commit message, even though GitHub provides the closingIssuesReferences field so that we can easily get the number of issues that will be closed by the PR, but in the actual situation, some PRs will be associated with an issue, but this issue may not be closed by it.

tison · September 22, 2021, 7:15am

Another question is that can we use a pattern with less change to current style, see also:

… and just issue-x, issue-y: as pkg-a, pkg-b:. If someone still want pkg-a as issue-x: pkg-a: it is ok. But not required.

ichn-hu · September 22, 2021, 7:54am

I think pkg: is intuitive and informative, issue-x requires more typing and is not intuitive, btw issue number is machine friendly, but never human friendly.

ichn-hu · September 22, 2021, 7:55am

if we really want the issue number in title, maybe

test: migrate test-infra to testify for types/etc_test.go; close #28002

is more eye friendly

tison · September 24, 2021, 3:06am

I’m ok with either style. @Mini256 is there any technical blocker?

disksing · September 24, 2021, 3:35am

Will the issue number in commit message create an extra reverse link to the issue?

tison · September 24, 2021, 10:05am

May or may not depend on the format.

If #{number}, it will; if issue-number or other bare way, it will not.

disksing · September 24, 2021, 12:17pm

I guess issue- would be a better idea.

tison · September 24, 2021, 2:13pm

I create another demo title.

@Mini256 please see whether we can implement checker with this format.

@disksing @ichn-hu please see whether it is in your style.

@SunRunAway for associate with multiple issues, one can use ; issue-1, issue-2 IMO.

If the length is a concern, we can shorten issue as TI or anything. We forbid cross repos mapping, just create a subtasks or mirror in the specific repo.

disksing · September 24, 2021, 5:59pm

I think the use : and ; creates too many fields in the title and it is hard for me to interpret. I would personally prefer [issue-28076] or (issue-28076) or [28076] or (28076).

gregweber · September 24, 2021, 6:45pm

It would be nice to be able to accept documentation updates and without requiring an issue ahead of time. The logic would be to not validate if all files changed are markdown files.

tison · September 26, 2021, 12:48am

Thanks for your comment. I agree that we can create exceptions later.

For TiDB / TiKV and other repositories except pingcap/docs*, there isn’t a significant percentage of such changes. Thus, let’s force every PR associated with issue(s). You can create an issue for tiny change, like https://github.com/tikv/tikv/issues/10962 . When we overcome the case now there are too many dangling PRs as well as release note sucks, we can bring this topic as an enhancement to our developers with better practices and behaviors.

For pingcap/docs, I’d delegate the decision to its maintainers.

tison · September 26, 2021, 12:49am

Could you create a completed example? Actually I don’t have a strong feeling, but associated with an issue so that we don’t omit it.

Mini256 · September 26, 2021, 5:51am

Our PR number has already used (, ), so it is not recommended to use (, ) for the issue number.

In order to avoid ambiguity, I think it is necessary to add a prefix, whether it is issue-, #, or TI-.

disksing · September 26, 2021, 7:01am

pkg: fix something [123], after merged it becomes pkg: fix something [123] (#124)