-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
issue checker is confused by PRs referencing other PRs #5483
Comments
As far as GitHub is concerned PRs are still issues which is likely the cause of this bug. /cc @cjwagner |
So many questions, here's just a start (I'm heading up the release note effort for 1.9, so bear with me):
|
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 |
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.
|
/assign |
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 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 |
If we are going to adopt the new release note tool (kubernetes/release#434, kubernetes/release#435), there might be a corner case here. PR title may also be used as release note. See: kubernetes/release#434 (comment) As for the |
BTW, are we going to adopt the new release note tool(s) for 1.9, or still use the script one? |
I just used anago. |
@enisoc the original discussion is here: Each PR should have an associated issue |
@dashpole then it will invoke the script version release notes tool: https://github.com/kubernetes/release/blob/master/anago#L425 |
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? |
I guess so. cc @roycaihw |
@xiangpengzhao @enisoc The new hierarchical release note function assumes PR authors following the PR template and looks for specific "fixes #<issue number>" in PR body. The tool doesn't use the |
That tool will have the same problem the approval-handler has now if I say |
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) ***@***.***> wrote:
That tool will have the same problem the approval-handler has now if I say
fixes #5 <#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
<#5483 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAnglloBXanG4-31ngYYzIzAQs0DNFypks5s2gUZgaJpZM4QcltY>
.
|
@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"? |
this is still a bit of a pain point. kubernetes/kubernetes#57126 (comment) |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/reopen
…On Sat, May 19, 2018 at 5:47 PM k8s-ci-robot ***@***.***> wrote:
Closed #5483 <#5483>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5483 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAnglkUtZv4mTgwcIcV1j_ecL4aP6GaWks5t0L0QgaJpZM4QcltY>
.
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/reopen
/lifecycle frozen
…On Wed, Jun 20, 2018 at 10:12 AM k8s-ci-robot ***@***.***> wrote:
Closed #5483 <#5483>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5483 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAnglkFF-H1ew6f1g-GDxLdw84Yr5aQGks5t-oKDgaJpZM4QcltY>
.
|
/remove-lifecycle frozen |
See kubernetes/kubernetes#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.
The text was updated successfully, but these errors were encountered: