Test-infra: Each PR should have an associated issue

[crimsonfaith91]

The release team has a feature request:

It is difficult to figure out from PRs exactly what's being addressed, and this is especially bad for tracking (as has been brought up before in retrospectives).
They want to require that new PRs have associated issues in order for the approval process to be complete.

Having a PR to be associated with at least an issue will:
(1) allow better visibility into the release (what features and bugfixes exist)
(2) facilitate tracking of feature and bugfix progress (e.g., this issue was fixed by these 4 PRs)
(3) facilitate categorization of PRs based on sig groups (i.e., each sig group will know which PRs are relevant to them)

/cc @foxish @grodrigues3 @dchen1107

[foxish]

cc @kubernetes/sig-contributor-experience-feature-requests

[k8s-ci-robot]

@foxish: These labels do not exist in this repository: sig/contributor-experience.

In response to this:

cc @kubernetes/sig-contributor-experience-feature-requests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[rmmh]

Why should every PR have an issue?

[grodrigues3]

Release team decided that issues:

• allow visibility into the release (what features bugfixes are present) and this was commonly complained about in the OSS retrospectives
• Having an associated issue makes tracking it's progress (i.e this issue was fixed by these 4 PRs)

[foxish]

It is an ongoing effort to improve the quality of the release, and currently, it appears that the release team has a hard time figuring out from PRs exactly what's being addressed.
Requiring that PRs have associated issues ensures that the release team has visibility into the underlying issue that it closes (partially or completely), and use that data for tracking.

One might argue that relnotes are one way to do this, but not every PR is fixing something that belongs in a relnote.

[crimsonfaith91]

/cc @apelisse

[spxtr]

I would be okay with requiring all PRs that request a release note to have an associated issue. However, requiring all PRs to have an issue would just be super annoying, and I think actually most PRs shouldn't. Here is the most recent PR to k/k. We should not require the author to open an issue for this.

[foxish]

The release team's biggest concern is that there is no single source of truth covering all the things that went into a release, and the PRs themselves are often badly described. Not many of them have a release-note even, especially if they are smaller bug fixes. Given the effort a person goes through to create most PRs, creating a corresponding issue wouldn't be too hard IMO.

However, for typo fixing PRs and the like, it does seem like it would be annoying to have a person describe that.
So, an escape hatch, to allow /approve no-issue will need to be provided.

[crimsonfaith91]

IMO, in addtion to it, perhaps we can introduce a trivial label for the typo fixing PRs and the like that can be approved without any issue.

[spxtr]

So, an escape hatch, to allow /approve no-issue will need to be provided.

I'm not sure I understand how this is supposed to work. Can you be more precise in what exactly happens when a PR such as the one I linked is opened and moves through the ordinary flow?

[apelisse]

I'll link Jeff Bezos on this one :-)

It sounds like we do a bad job giving good descriptions/labels to PRs. I completely agree with that and argued for better PR description for as long as I've worked on this project. We also fail to shepherd PRs consistently.

To fix that, you suggest creating issues as a proxy. Can you explain me:

• How issues are going to be easier to manage than PRs (while we clearly have much less control over issues currently than we have on PRs),
• Why people would give issues a better descriptions than they do give to their PRs, and/or label them properly,
• How a more involved process will simplify people's life, improve the velocity of the project, or even improve the quality of anything?

Both (1) and (3) can be addressed within PRs themselves. I don't disagree that (2) can be useful for complex features that requires multiple steps.

We need to find a good balance between velocity and quality/verification, but I really believe the issue/proxy would just hurt both.

[dchen1107]

I received a tons of questions from the EngProd team related to this issue. Hope I can answer those questions through this thread.

First let me start with the new policy which triggers this issue was file at the first place. We (the release managemen team) announced a new policy at kubernetes community meeting at:
https://docs.google.com/a/google.com/presentation/d/1a5XSJRsFg-F-f_V5Yn24BxrKirbH_pMk3VfYM27w9GY/edit?usp=sharing (slide 3)

No objections from the community. Later, we sent out the announcement to kubernetes-dev@ at https://groups.google.com/forum/#!topic/kubernetes-dev/NNnbHAhzuVI after consulting some sig group leads.

The purpose of the new policy is to we, the maintainer of Kubernetes can answer what are included in each release. Today we have

• Tons of issues and tons of prs. Many issues are staled and should be closed, but no easy way to find which pr(s) resolve that issue and in which release since a lot of engineers do not provide a reference to the issue for their prs.
• If an issue was filed as an umbrella which requires a bunch of prs (sometimes even from multiple assignees, across multiple releases), there is no easy way to track or aggregate all progress. The feature repo's issues have the same problem, especially a contributor normally won't refer the prs to the feature repo.
• If multiple prs are involved for resolving an issue, it is very hard to generate a user-friendly release notes. When I scanned through the auto-generated release notes each time, I found many duplicated release note entries referring to the same features / issues, but from different prs. Because there is no xref issue policy enforced, it is very hard for the release management team / documentation team to clean those mess.
• Also each sig group to manage their features / issues / bugs, they have to look at both issues and prs, and apply the same set of labels and milestones to both. That is an extra overhead too.
  ...

To answer above question, 1.7 release team proposed the state machine for every issue (including both feature repo and main repo), and associated policy / workflow. This issue along with https://github.com/kubernetes/test-infra/issues/2869 were filed to ensure the workflow and policy installed.

Why each pr requires an issue for the new policy? If we don't install this policy, the workflow / policy proposed above cannot be enforced since it is very easy for the developers to start pr without an issue especially the new policy imposes extra overhead.

Why not simply apply the same workflow / policy to prs? Couple reasons and the below are the top two in my head:

• The new policy impose extra overhead to each sig group, especially the leads. Applying the same workflow to the pr could potentially double that overhead.
• The pr won't be authored by several engineers, won't across several release, but issue can. To track the incremental progress of a feature / problem, the issue serves better here.

How do we link a PR to an issue and vice versa? It is very easy. In your pr's description include the one of following strings:

• Fixed #XXXX
• Partially fixed #YYYY
• Related to #WWW
• Per comment at #ZZZZ
  ...

Again the issue can be the one in feature repo or the one in the main repo.

What happens to PRs that don't link an issue when they are opened, approved, lgtmed? That is why we filed this issue at the first place. If bot didn't detect an issue associated with that pr, even approver approve that pr, the pr won't be merged. Approver bot will be updated with proper message on why. Same with lgtm bot.

How do we ensure this doesn't stop trivial PRs like typo fixes? If the approver agreed this pr is trivial, he / she can simply type "/approve no-issue".

At last, the policy / workflow is subject to change. We want to give it a try, then iterate later. But mungen scripts / k8s-bots are the key to start this process. Thanks!

[xiangpengzhao]

Partially fixed #YYYY

@dchen1107 IIUC, k8s-merge-robot will close the referenced issue #XXXX after PR is merged if it has words fixed #XXXX (or fixes, etc. code here) Do we want the issue be closed if it is just partially fixed?

[rmmh]

That's not done by k8s-merge-robot, that's a github feature to close with commit/PR messages.

[xiangpengzhao]

@rmmh Thanks for pointing out that!

However, we still have to decide if we want the issue be closed if it is just partially fixed, right? IMO, we should keep the issue open until it is completely solved.

[thockin]

The process is not the objective, the result is the objective. The process is a tool.

[apelisse]

Implemented by #2930. About to deploy.

[rmmh]

So, most of the PRs in the queue that were approved will be kicked out by this new requirement just after code freeze?

[thockin]

@dchen1107 How do you want to handle existing open PRs?

[dchen1107]

Re: https://github.com/kubernetes/test-infra/issues/2868#issuecomment-306016634

@rmmh and I discussed this offline: For the existing open PRs with /lgtm & /approved, we plan to take them as is since the process is so late for 1.7 release. For any new opened prs, we should enforce the policy.

[grodrigues3]

Sounds like the right policy to me. Balances our visibility goals with project velocity. Thanks @dchen1107

[apelisse]

I ran in dry-mode, here is the list of affected pull-requests:

I0605 10:46:24.679590   94597 github.go:969] Removing label "approved" to PR 31399
I0605 10:46:29.785462   94597 github.go:969] Removing label "approved" to PR 32460
I0605 10:46:33.300363   94597 github.go:969] Removing label "approved" to PR 33440
I0605 10:46:40.871361   94597 github.go:969] Removing label "approved" to PR 35113
I0605 10:46:45.399338   94597 github.go:969] Removing label "approved" to PR 36325
I0605 10:46:46.337407   94597 github.go:969] Removing label "approved" to PR 36376
I0605 10:46:54.460682   94597 github.go:969] Removing label "approved" to PR 38184
I0605 10:46:55.280054   94597 github.go:969] Removing label "approved" to PR 38351
I0605 10:46:59.663423   94597 github.go:969] Removing label "approved" to PR 38950
I0605 10:47:00.647074   94597 github.go:969] Removing label "approved" to PR 39054
I0605 10:47:06.194361   94597 github.go:969] Removing label "approved" to PR 39620
I0605 10:47:07.214589   94597 github.go:969] Removing label "approved" to PR 39728
I0605 10:47:10.189225   94597 github.go:969] Removing label "approved" to PR 39901
I0605 10:47:13.722219   94597 github.go:969] Removing label "approved" to PR 40284
I0605 10:47:21.800616   94597 github.go:969] Removing label "approved" to PR 40963
I0605 10:47:22.636073   94597 github.go:969] Removing label "approved" to PR 40970
I0605 10:47:24.053446   94597 github.go:969] Removing label "approved" to PR 41016
I0605 10:47:29.032818   94597 github.go:969] Removing label "approved" to PR 41499
I0605 10:47:30.156779   94597 github.go:969] Removing label "approved" to PR 41622
I0605 10:47:30.812438   94597 github.go:969] Removing label "approved" to PR 41634
I0605 10:47:33.976378   94597 github.go:969] Removing label "approved" to PR 41698
I0605 10:47:43.795756   94597 github.go:969] Removing label "approved" to PR 42142
I0605 10:47:52.195254   94597 github.go:969] Removing label "approved" to PR 42340
I0605 10:47:58.529114   94597 github.go:969] Removing label "approved" to PR 42540
I0605 10:47:59.290820   94597 github.go:969] Removing label "approved" to PR 42546
I0605 10:48:03.618652   94597 github.go:969] Removing label "approved" to PR 42595
I0605 10:48:06.571412   94597 github.go:969] Removing label "approved" to PR 42684
I0605 10:48:09.451003   94597 github.go:969] Removing label "approved" to PR 42783
I0605 10:48:10.348318   94597 github.go:969] Removing label "approved" to PR 42829
I0605 10:48:11.824486   94597 github.go:969] Removing label "approved" to PR 42943
I0605 10:48:14.221947   94597 github.go:969] Removing label "approved" to PR 43004
I0605 10:48:27.757923   94597 github.go:969] Removing label "approved" to PR 43420
I0605 10:48:32.400804   94597 github.go:969] Removing label "approved" to PR 43558
I0605 10:48:45.245644   94597 github.go:969] Removing label "approved" to PR 44048
I0605 10:48:47.607380   94597 github.go:969] Removing label "approved" to PR 44125
I0605 10:48:52.834599   94597 github.go:969] Removing label "approved" to PR 44245
I0605 10:48:53.532182   94597 github.go:969] Removing label "approved" to PR 44249
I0605 10:48:56.143675   94597 github.go:969] Removing label "approved" to PR 44282
I0605 10:48:58.856634   94597 github.go:969] Removing label "approved" to PR 44358
I0605 10:49:05.441925   94597 github.go:969] Removing label "approved" to PR 44464
I0605 10:49:13.346432   94597 github.go:969] Removing label "approved" to PR 44653
I0605 10:49:14.089857   94597 github.go:969] Removing label "approved" to PR 44654
I0605 10:49:20.051756   94597 github.go:969] Removing label "approved" to PR 44786
I0605 10:49:28.516781   94597 github.go:969] Removing label "approved" to PR 44900
I0605 10:49:31.368621   94597 github.go:969] Removing label "approved" to PR 44936
I0605 10:49:44.388072   94597 github.go:969] Removing label "approved" to PR 45268
I0605 10:50:04.156609   94597 github.go:969] Removing label "approved" to PR 45532
I0605 10:50:08.277971   94597 github.go:969] Removing label "approved" to PR 45552
I0605 10:50:10.854959   94597 github.go:969] Removing label "approved" to PR 45603
I0605 10:50:15.791496   94597 github.go:969] Removing label "approved" to PR 45644
I0605 10:50:17.119613   94597 github.go:969] Removing label "approved" to PR 45651
I0605 10:50:18.558068   94597 github.go:969] Removing label "approved" to PR 45660
I0605 10:50:20.632954   94597 github.go:969] Removing label "approved" to PR 45666
I0605 10:50:23.143245   94597 github.go:969] Removing label "approved" to PR 45697
I0605 10:50:28.868207   94597 github.go:969] Removing label "approved" to PR 45784
I0605 10:50:34.864991   94597 github.go:969] Removing label "approved" to PR 45877
I0605 10:50:36.563675   94597 github.go:969] Removing label "approved" to PR 45918
I0605 10:50:41.372763   94597 github.go:969] Removing label "approved" to PR 45984
I0605 10:50:49.954624   94597 github.go:969] Removing label "approved" to PR 46082
I0605 10:50:55.222344   94597 github.go:969] Removing label "approved" to PR 46112
I0605 10:50:57.367871   94597 github.go:969] Removing label "approved" to PR 46135
I0605 10:50:59.969670   94597 github.go:969] Removing label "approved" to PR 46157
I0605 10:51:05.557338   94597 github.go:969] Removing label "approved" to PR 46181
I0605 10:51:11.270496   94597 github.go:969] Removing label "approved" to PR 46228
I0605 10:51:11.964790   94597 github.go:969] Removing label "approved" to PR 46235
I0605 10:51:15.195128   94597 github.go:969] Removing label "approved" to PR 46260
I0605 10:51:17.766317   94597 github.go:969] Removing label "approved" to PR 46276
I0605 10:51:22.882989   94597 github.go:969] Removing label "approved" to PR 46314
I0605 10:51:25.501148   94597 github.go:969] Removing label "approved" to PR 46318
I0605 10:51:33.075888   94597 github.go:969] Removing label "approved" to PR 46360
I0605 10:51:36.182160   94597 github.go:969] Removing label "approved" to PR 46382
I0605 10:51:38.511146   94597 github.go:969] Removing label "approved" to PR 46401
I0605 10:51:49.390854   94597 github.go:969] Removing label "approved" to PR 46512
I0605 10:51:53.186769   94597 github.go:969] Removing label "approved" to PR 46539
I0605 10:51:54.533797   94597 github.go:969] Removing label "approved" to PR 46542
I0605 10:52:05.913305   94597 github.go:969] Removing label "approved" to PR 46604
I0605 10:52:11.650924   94597 github.go:969] Removing label "approved" to PR 46642
I0605 10:52:14.168747   94597 github.go:969] Removing label "approved" to PR 46660
I0605 10:52:15.047499   94597 github.go:969] Removing label "approved" to PR 46662
I0605 10:52:16.360166   94597 github.go:969] Removing label "approved" to PR 46665
I0605 10:52:18.486378   94597 github.go:969] Removing label "approved" to PR 46669
I0605 10:52:24.767669   94597 github.go:969] Removing label "approved" to PR 46699
I0605 10:52:25.608723   94597 github.go:969] Removing label "approved" to PR 46700
I0605 10:52:29.111706   94597 github.go:969] Removing label "approved" to PR 46717
I0605 10:52:29.888659   94597 github.go:969] Removing label "approved" to PR 46718
I0605 10:52:30.704099   94597 github.go:969] Removing label "approved" to PR 46727
I0605 10:52:32.561419   94597 github.go:969] Removing label "approved" to PR 46743
I0605 10:52:34.131806   94597 github.go:969] Removing label "approved" to PR 46745
I0605 10:52:38.970416   94597 github.go:969] Removing label "approved" to PR 46755
I0605 10:52:40.267338   94597 github.go:969] Removing label "approved" to PR 46757
I0605 10:52:44.298163   94597 github.go:969] Removing label "approved" to PR 46775
I0605 10:52:46.877534   94597 github.go:969] Removing label "approved" to PR 46787
I0605 10:52:48.613829   94597 github.go:969] Removing label "approved" to PR 46796
I0605 10:52:49.955014   94597 github.go:969] Removing label "approved" to PR 46799
I0605 10:52:50.637341   94597 github.go:969] Removing label "approved" to PR 46800
I0605 10:52:51.299921   94597 github.go:969] Removing label "approved" to PR 46802
I0605 10:52:54.404679   94597 github.go:969] Removing label "approved" to PR 46811
I0605 10:52:55.100681   94597 github.go:969] Removing label "approved" to PR 46812
I0605 10:52:59.149741   94597 github.go:969] Removing label "approved" to PR 46819
I0605 10:53:01.592951   94597 github.go:969] Removing label "approved" to PR 46824
I0605 10:53:05.487227   94597 github.go:969] Removing label "approved" to PR 46829
I0605 10:53:07.106837   94597 github.go:969] Removing label "approved" to PR 46833
I0605 10:53:08.901816   94597 github.go:969] Removing label "approved" to PR 46836
I0605 10:53:18.862758   94597 github.go:969] Removing label "approved" to PR 46864
I0605 10:53:22.951291   94597 github.go:969] Removing label "approved" to PR 46878
I0605 10:53:23.560911   94597 github.go:969] Removing label "approved" to PR 46879
I0605 10:53:24.818515   94597 github.go:969] Removing label "approved" to PR 46881
I0605 10:53:30.028963   94597 github.go:969] Removing label "approved" to PR 46902
I0605 10:53:32.569096   94597 github.go:969] Removing label "approved" to PR 46907
I0605 10:53:35.468260   94597 github.go:969] Removing label "approved" to PR 46916
I0605 10:53:35.980035   94597 github.go:969] Removing label "approved" to PR 46921
I0605 10:53:36.620794   94597 github.go:969] Removing label "approved" to PR 46923
I0605 10:53:39.589969   94597 github.go:969] Removing label "approved" to PR 46930
I0605 10:53:41.441733   94597 github.go:969] Removing label "approved" to PR 46938
I0605 10:53:43.090918   94597 github.go:969] Removing label "approved" to PR 46944
I0605 10:53:50.887488   94597 github.go:969] Removing label "approved" to PR 46967
I0605 10:53:53.183597   94597 github.go:969] Removing label "approved" to PR 46973

That's 117.

[dchen1107]

@apelisse I just applied /approve no-issue to PR: https://github.com/kubernetes/kubernetes/pull/46539 Can we dry-run one more time to see if that pr is removed from above list.

The release team should manually scan through above list anyway. For example
PR https://github.com/kubernetes/kubernetes/pull/41622 requires a rebase since Feb, but no action. But now it is blindly marked with 1.7 release.

[apelisse]

PR is still approved in dry-run, with the following message:

Associated issue requirement bypassed by: dchen1107

[apelisse]

Dawn will re-approve issues. Deploying now.

[timstclair]

The regex doesn't match issues that are referenced in URL format, even though they look the same from github's perspective. Ex:

https://github.com/kubernetes/test-infra/issues/2868: https://github.com/kubernetes/test-infra/issues/2868
#2868: #2868

Or:

https://github.com/kubernetes/features/issues/22: https://github.com/kubernetes/features/issues/22
kubernetes/features#22: kubernetes/features#22

[grodrigues3]

Reran with the fix that @apelisse wrote. Looks like the bot readded the approved label to
60+ PRs.

https://pastebin.com/ffnnCjCF

[xiangpengzhao]

Another concern. Someone might find a bug and be working on it. Before he/she has a PR for it, he/she may file an issue first. Should we let he/she point out that he/she is working on it in the issue description? So that other guys don't need to spend time trying to fix it.

[apelisse]

People usually assign to their issue as a sign that they are working on it.

[xiangpengzhao]

SGTM. Hope that all contributors know /assign :)

[eparis]

We expect users to open an issue for every single PR ever? I guess I get it if a PR is fixing a bug/issue. Or if we are in release lockdown where we really want extra tracking. But every single PR for 1.8 needs an issue or a magic command? I'm not in love. (Though also not saying this is wrong, just making sure I understand)

[grodrigues3]

@eparis Visibility into the releases has historically been one of the challenges of the project. The release team was trying to address this, but if this isn't the best way, we should look into it at the 1.7 retrospective. (Hopefully by magic command you meant that you can approve an issue with no issue so that comment fixes, markdown changes and tiny bugfixes don't cause ridiculous overhead)

[wongma7]

I'm extremely late to this, so sorry for reviving an old discussion, but having just been bit by it I'd like to voice my displeasure with how it's is implemented :).

The PR template needs to be updated, because it doesn't mention this requirement and in fact has confusing wording: it says linking to which issue the PR fixes is "optional," but it's only the "fixes/fix" part that's optional.

k8s-merge-bot does remind me if I forget to link an issue, but its reminder is buried in the middle of its [APPROVAL NOTIFIER] comment, which is long and looks almost identical in every PR, meaning the reminder is extremely easy to overlook, for both the PR author and the approver who probably wants to use approve no-issue. So I suggest we bold the "No associated issue" message instead of italicizing it, or better yet, put it in a new comment, kind of like how 'needs-rebase' or 'release-note-needed' are handled.

Now I'm pretty ignorant of the release team's work & the stuff that motivated this change but:
(1) I thought that was what release notes are for? i.e., a good bugfix release note should say "fixed an issue where..." I suppose the idea is that it's easier to get people to simply link to an issue than write a good release note. But if no issue exists then we're asking them to do the bigger task of writing a good issue.
(3) Why don't we require PRs to have a sig label instead of using issues as a proxy.
edit: I found I'm basically echoing @apelisse's earlier comment here, read his https://github.com/kubernetes/test-infra/issues/2868#issuecomment-304932573 if mine is unintelligible

[fejta]

We now enforce this. Lots of people do not like it.