Skip to content
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.

Sign up for GitHub

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

Jump to bottom

k8s-ci-robot closes PR in different repo without right to reopen it #5032

Closed
loburm opened this issue Oct 17, 2017 · 31 comments
Closed

k8s-ci-robot closes PR in different repo without right to reopen it #5032

loburm opened this issue Oct 17, 2017 · 31 comments
Labels
area/prow Issues or PRs related to prow kind/bug Categorizes issue or PR as related to a bug.

Comments

@loburm
Copy link
Contributor

loburm commented Oct 17, 2017

This has happened in kubernetes-retired/heapster#1844. Author has referenced in one of his comments PR from the test-infra. Once PR in test-infra was merged original PR was closed by the k8s-ci-robot, and looks like no one has a permission to reopen it.
@BenTheElder BenTheElder added the kind/bug Categorizes issue or PR as related to a bug. label Oct 17, 2017
@BenTheElder
Copy link
Member

/cc @cjwagner

@krzyzacy
Copy link
Member

is the original comment text something like fixes kubernetes/heapster#1844? This is a github thing afaik

@BenTheElder
Copy link
Member

regardless of why it was closed, they cannot reopen the PR, which is itself a bug

@cjwagner
Copy link
Member

I just checked the logs and it doesn't look like Prow closed the PR, I think its because the issue was linked in the PR body.
As to why the /reopen command isn't working:

"status code 422 not one of [200], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","field":"state","message":"state cannot be changed. The bump_kubernetes_to_1.8 branch was force-pushed or recreated."}],"documentation_url":"https://developer.github.com/v3/pulls/#update-a-pull-request"}"

The relevant bit:

state cannot be changed. The bump_kubernetes_to_1.8 branch was force-pushed or recreated.

@chrislovecnm
Copy link
Contributor

This nailed us as well ... :(

It is reading all of the linked issues and closing them

@spxtr
Copy link
Contributor

spxtr commented Oct 18, 2017

This is just an annoying GitHub feature. If a PR body or commit message says "fixes someorg/somerepo#12345" then when that PR is merged GitHub will close the thing. This is particularly annoying in commit messages, because sometimes when I sync my k/k fork I'll close random issues in other repos.

@BenTheElder
Copy link
Member

@cjwagner ah, I think I have an open issue about catching that error and reporting it to the user.

@BenTheElder
Copy link
Member

#4786

@k8s-ci-robot
Copy link
Contributor

I did not do that, don't blame me 😞 How about I tell you a joke
/joke

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot: I bought shoes from a drug dealer once. I don't know what he laced them with, but I was tripping all day.

In response to this:

I did not do that, don't blame me 😞 How about I tell you a joke
/joke

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.

@chrislovecnm
Copy link
Contributor

This is just an annoying GitHub feature. If a PR body or commit message says "fixes someorg/somerepo#12345" then when that PR is merged GitHub will close the thing. This is particularly annoying in commit messages, because sometimes when I sync my k/k fork I'll close random issues in other repos.

Yah this may be different. The issue was just mentioned, and the merge bot closed the issue NOT GitHub.

@spxtr
Copy link
Contributor

spxtr commented Oct 18, 2017

the merge bot closed the issue NOT GitHub.

GitHub closes it as whoever merged the PR or pushed the commit. See this example where I supposedly closed an issue. Actually all I did was sync my fork of kubernetes.

The issue was just mentioned

That's strange. Maybe the author edited the PR body?

@loburm
Copy link
Contributor Author

loburm commented Oct 19, 2017

I'm ok with closing issues by PR, but why PR A is closed when PR B is merged, considering that PR B was just mentioned in PR A. By mentioned I mean no one used keyword as fixes, closes, etc.

@BenTheElder
Copy link
Member

BenTheElder commented Oct 19, 2017 via email

@BenTheElder
Copy link
Member

If the robot had just closed it on its own you would see something like "closed by k8s-ci-robot" whereas "@user closed this in " means merging the linked PR caused GitHub to close it.

@loburm
Copy link
Contributor Author

loburm commented Oct 19, 2017

Wow, that's really terrible feature. I'm surprised that I can't actually mention other PR's from the comments of different PR.

@BenTheElder Thanks for the explanation, I think that we can close it now.

@krzyzacy
Copy link
Member

@loburm I think the problem is mention fix|fixes #some-pr, mention PR only is definitely fine. That original PR comment from #5009 (comment) might contain fix before author edited it?

@BenTheElder
Copy link
Member

@krzyzacy @loburm I'm pretty sure if you mention a PR in a comment it will be fine if you don't say fix|fixes, but I think you also might need to not link to it in the PR's opening comment.

@fejta
Copy link
Contributor

fejta commented Oct 19, 2017

We should disallow commit messages with fixes #foo

@spxtr
Copy link
Contributor

spxtr commented Oct 19, 2017

We should disallow commit messages with fixes #foo

Heh, I wish.

@cjwagner
Copy link
Member

We could block PRs with commits that contain messages like that. It could just manage a new do-not-merge/ label.

@spiffxp
Copy link
Member

spiffxp commented Oct 20, 2017

The "annoying feature" https://help.github.com/articles/closing-issues-using-keywords/

When the pull request is merged into your repository's default branch, the corresponding issue is automatically closed.

@chrislovecnm
Copy link
Contributor

The coresponding issue? This is not corresponding. This smells like a github bug

@0xmichalis 0xmichalis added the area/prow Issues or PRs related to prow label Oct 31, 2017
@BenTheElder
Copy link
Member

kubernetes/enhancements#383 :(
Can we re-raise this with GitHub? Does anyone have an existing point of contact with them?

@BenTheElder BenTheElder mentioned this issue Dec 13, 2017
Closed
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 13, 2018
@BenTheElder BenTheElder mentioned this issue Mar 23, 2018
Closed
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 12, 2018
@chrislovecnm
Copy link
Contributor

/lifecycle frozen
/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Apr 12, 2018
@fejta fejta removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jun 20, 2018
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 18, 2018
@BenTheElder
Copy link
Member

Recently we've had people looking at blocking merges that would trigger this behavior cc @nikhita
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 18, 2018
@spiffxp
Copy link
Member

spiffxp commented Sep 18, 2018

/close
Closing in favor of #9360 which aims to prevent this from happening

@k8s-ci-robot
Copy link
Contributor

@spiffxp: Closing this issue.

In response to this:

/close
Closing in favor of #9360 which aims to prevent this from happening

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.

@BenTheElder BenTheElder mentioned this issue Oct 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/prow Issues or PRs related to prow kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests