Test-infra: issue checker is confused by PRs referencing other PRs

[lavalamp]

See https://github.com/kubernetes/kubernetes/pull/55533.

Note the "approved" comment has decided that the associated issue is 55127, which is not even an issue but a PR this one is rebased on top of. (note, I just edited to add the "Ref:" link, it wasn't there when the bot made that comment.)

Seems like whatever does that should at least check that the thing is an issue and not a PR.

This likely means that our release note generating stuff is going to be confused or generate wrong output, but I don't know how widespread this is.

[BenTheElder]

As far as GitHub is concerned PRs are still issues which is likely the cause of this bug.

/cc @cjwagner

[Bradamant3]

So many questions, here's just a start (I'm heading up the release note effort for 1.9, so bear with me):

• the example (#55533) looks as though it's a test? We don't and shouldn't provide release notes for tests, including e2e tests. If the script for generating the release notes is iterating over PRs for tests, then we should fix the script.
• we should also not be generating release notes at all for PRs that don't have release note content (which should take care of the issue the reporter noted).

[lavalamp]

Yeah, I'm sure that is the cause of the bug.

The test is part of a bigger feature which has a release note. The PR either needs to be associated with that issue, or say NONE for its release note to make the bot happy.

[BenTheElder]

the example (#55533) looks as though it's a test? We don't and shouldn't provide release notes for tests, including e2e tests. If the script for generating the release notes is iterating over PRs for tests, then we should fix the script.

The reviewer should make sure PRs that shouldn't have release notes don't and that PRs that should, do. The GitHub bot just makes sure you've actually set these values as far as I know.

we should also not be generating release notes at all for PRs that don't have release note content (which should take care of the issue the reporter noted).

/release-note none / putting NONE in the PR body should cover this (?), I don't think we are generating release note for PRs without release note content. If we are that definitely seems like a bug.

[cjwagner]

/assign

[enisoc]

I haven't verified this, but my impression has always been that the "Associated issue" is not used for anything except deciding whether to require /approve no-issue.

I'm not aware of any automation that follows "associated issue" links at this time. For release notes in particular, we don't look at non-PR issues at all. We list PRs with the release-note label, and get the text directly from the PR. We then link to the PR (not an issue) in the generated release notes.

[xiangpengzhao]

We list PRs with the release-note label, and get the text directly from the PR.

If we are going to adopt the new release note tool (https://github.com/kubernetes/release/pull/434, https://github.com/kubernetes/release/pull/435), there might be a corner case here. PR title may also be used as release note. See: https://github.com/kubernetes/release/pull/434#discussion_r145884721

As for the associated issue, the Hierarchical release note feature functions will try to extract sig/area info from PRs' associated issues.

[xiangpengzhao]

BTW, are we going to adopt the new release note tool(s) for 1.9, or still use the script one?
Seems like v1.9.0-alpha.2 changelog was updated by @dashpole (https://github.com/kubernetes/kubernetes/commit/e0cac2a2f0359f8908076ad92fcd33fb3bdfcb13), @dashpole which release tool were you using?

[dashpole]

I just used anago.

[xiangpengzhao]

...but my impression has always been that the "Associated issue" is not used for anything except deciding whether to require /approve no-issue.

@enisoc the original discussion is here: Each PR should have an associated issue

[xiangpengzhao]

I just used anago.

@dashpole then it will invoke the script version release notes tool: https://github.com/kubernetes/release/blob/master/anago#L425

[enisoc]

Thanks for the info @xiangpengzhao. So to summarize, our current tooling should not be impaired by this problem, but the new hierarchical release note function might get confused by it?

[xiangpengzhao]

but the new hierarchical release note function might get confused by it?

I guess so. cc @roycaihw

[cjwagner]

That tool will have the same problem the approval-handler has now if I say fixes #5 and 5 is a PR.

[roycaihw]

That tool will have the same problem the approval-handler has now if I say fixes #5 and 5 is a PR.

@cjwagner Ah I see. Yes you're right. I will add a check to that tool to make sure the "issue" is not a PR.

[lavalamp]

Just looking for "fixes" is not enough. It should also look for "ref" or
"references"; sometimes it takes many PRs to "fix" an issue, in fact that
is the only case where it is useful to have an associated issue: when you
want to be able to group multiple PRs.

On Tue, Nov 14, 2017 at 1:24 PM, Haowei Cai (Roy) [e-mail]
wrote:

That tool will have the same problem the approval-handler has now if I say
fixes #5 https://github.com/kubernetes/test-infra/pull/5 and 5 is a PR.

@cjwagner https://github.com/cjwagner Ah I see. Yes you're right. I
will add a check to that tool to make sure the "issue" is not a PR.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/5483#issuecomment-344403650,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglloBXanG4-31ngYYzIzAQs0DNFypks5s2gUZgaJpZM4QcltY
.

[roycaihw]

@lavalamp Totally agree. One concern is that I see many PR referring other PRs. We can filter out the associated PRs and keep only the issues, but maybe we should provide some guideline in PR template about how to use "ref"?

[BenTheElder]

this is still a bit of a pain point. https://github.com/kubernetes/kubernetes/pull/57126#issuecomment-351283215
/cc @mindprince see above: https://github.com/kubernetes/test-infra/issues/5483#issuecomment-344335315
edit and more here: https://github.com/kubernetes/test-infra/issues/3737

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[lavalamp]

/reopen

On Sat, May 19, 2018 at 5:47 PM k8s-ci-robot [e-mail]
wrote:

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

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/5483#event-1635742459,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglkUtZv4mTgwcIcV1j_ecL4aP6GaWks5t0L0QgaJpZM4QcltY
.

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[lavalamp]

/reopen
/lifecycle frozen

On Wed, Jun 20, 2018 at 10:12 AM k8s-ci-robot [e-mail]
wrote:

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

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/5483#event-1691742604,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglkFF-H1ew6f1g-GDxLdw84Yr5aQGks5t-oKDgaJpZM4QcltY
.

[fejta]

/remove-lifecycle frozen

[spiffxp]

/close
@lavalamp The associated issue requirement for PR's was disabled in January (thread) so we're not inclined to work on this unless there's an urgent need. I'll admit, I haven't kept up to date on how release notes are being generated these days, but it's not dependent on this.