Test-infra: /reopen should have wider ACLs
This is pretty ridiculous:
After @fejta-bot closes a PR/issue if you are not the author or assigned to it you must do the following to reopen without leveraging your own write access:
- comment
/assign, the robot will assign you - comment
/reopen, the robot will reopen it - comment
/unassign, the robot will unassign you
There are plenty of reasons to keep an issue open without being assigned... we should probably allow a maintainers team or at least those that already have write access to the repo to /reopen without jumping through the assign/unassign hoop.
/area prow
/kind bug
/cc @cjwagner @stevekuznetsov @kargakis
BenTheElder
All 21 comments
examples:
https://github.com/kubernetes/test-infra/issues/4761#event-1697744103
https://github.com/kubernetes/test-infra/issues/4425#issuecomment-399781854
https://github.com/kubernetes/test-infra/issues/4046#issuecomment-399781899
BenTheElder
on 25 Jun 2018
Funny example from the impl of the feature way back when ... https://github.com/kubernetes/test-infra/issues/2237#issuecomment-294075659
stevekuznetsov
on 26 Jun 2018
Can't find any explanation of why we use the ACL we do in the initial implementation of /close here https://github.com/kubernetes/test-infra/pull/997 which was copied into /reopen. Unless we have a real need to block people who cannot be assigned from closing and opening issues, maybe let's just not do the ACL check? Is it really useful?
stevekuznetsov
on 26 Jun 2018
We need to use some kind of gate. It wouldn't be great if anyone on the internet could show up and start closing all of our PRs and issues. I think requiring that the commenter is an organization collaborator would be most consistent with other plugins.
cjwagner
on 26 Jun 2018
being a collaborator / org member SGTM, if we can't trust collaborators / org members to close / open issues responsibly we have bigger issues 😬
@cblecker contribex bat signal 🦇🔦
/sig contributor-experience
BenTheElder
on 26 Jun 2018
IsCollaborator is the effect right now, yes? You need to be able to be assigned an issue (minimum read access to repo) to be able to close/reopen. I'm okay with that. What I don't like about the above is that I'm guessing @kargakis put in other comments to assign himself, then reopen, then close, then deleted the comments.. if someone can close/reopen issues without us having a visible trail, that's no good.
cblecker
on 26 Jun 2018
I think we're in violent agreement about what the behavior should be -- if you can be assigned you can reopen/close -- but we should not actually require someone to assign themselves.
fejta
on 26 Jun 2018
Yes exactly what Erick said.
If we need a trail the robot can comment:
Reopening issue.
In response to:
On Tue, Jun 26, 2018, 11:16 Erick Fejta notifications@github.com wrote:
I think we're in violent agreement about what the behavior should be -- if
you can be assigned you can reopen/close -- but we should not actually
require someone to assign themselves.—
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/8450#issuecomment-400412827,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA4Bq0KV8b7Byp-4T-f724oow7FECTF7ks5uAnpqgaJpZM4U20no
.
BenTheElder
on 26 Jun 2018
+1 to what Erick said, and +1 to having the trail.. preferably on both close and reopen.
cblecker
on 26 Jun 2018
if someone can close/reopen issues without us having a visible trail, that's no good
That is really a problem common to all slash commands implemented by Prow. We use the pattern Ben described to indicate why the robot took certain actions, but we only do so when we are already leaving a comment.
This is probably the right path to take, but it will increase comment spam from the bot. 🤷♂️
cjwagner
on 26 Jun 2018
Bumping, I just danced through the "/assign", "/reopen", "/unassign" again, this 3Xes the noise from reopening an issue https://github.com/kubernetes/kubernetes/issues/30784#event-1707527553 ...
Are we at consensus to expand to collaborators and leave a comment? Or do we need a formal proposal etc...
BenTheElder
on 29 Jun 2018
I am +1 on expanding to collaborators
Please make commenting configurable (if we are talking about changing the
bot quote every command in a new comment though I guess that's a separate
issue).
On Fri, Jun 29, 2018, 01:21 Benjamin Elder notifications@github.com wrote:
Bumping, I just danced through the "/assign", "/reopen", "/unassign"
again, this 3Xes the noise from reopening an issue kubernetes/kubernetes#30784
(comment)
https://github.com/kubernetes/kubernetes/issues/30784#event-1707527553
...Are we at consensus to expand to collaborators and leave a comment? Or do
we need a formal proposal etc...—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/8450#issuecomment-401201671,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADuFfw5D1Fui-acSdMIqQF-kzH3hf6qbks5uBWTYgaJpZM4U20no
.
kargakis
on 29 Jun 2018
Er the commenting could be configurable, but what we're talking about is
having the bot comment something like: "Ok, I reopened the PR." along with
the standard
comments so that there's some trace regarding on whose behalf the bot
reopened the issue.
Edit: I think? this is reasonable and shouldn't be project specific. The only problem I see with the comments is just more comment spam..
On Thu, Jun 28, 2018 at 5:01 PM Michalis Kargakis notifications@github.com
wrote:
I am +1 on expanding to collaborators
Please make commenting configurable (if we are talking about changing the
bot quote every command in a new comment though I guess that's a separate
issue).On Fri, Jun 29, 2018, 01:21 Benjamin Elder notifications@github.com
wrote:Bumping, I just danced through the "/assign", "/reopen", "/unassign"
again, this 3Xes the noise from reopening an issue
kubernetes/kubernetes#30784
(comment)
https://github.com/kubernetes/kubernetes/issues/30784#event-1707527553
...Are we at consensus to expand to collaborators and leave a comment? Or do
we need a formal proposal etc...—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<
https://github.com/kubernetes/test-infra/issues/8450#issuecomment-401201671
,
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/ADuFfw5D1Fui-acSdMIqQF-kzH3hf6qbks5uBWTYgaJpZM4U20no.
—
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/8450#issuecomment-401208103,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA4Bq-MtbCr4QtXtWS1HjaXtLtsQGaJOks5uBW5bgaJpZM4U20no
.
BenTheElder
on 29 Jun 2018
Closing and reopening an issue or PR must have an audit trail (it's what I'd consider a more privileged action). Currently closing works without more commands, but it forces assigning (like /lgtm does).
We could either force assign, or we could comment that we are taking the action. Either way, I'm okay with not requiring actual assignment to do it.
cblecker
on 29 Jun 2018
an audit trail
There's still an audit trail, even if someone deletes the comment.
fejta
on 29 Jun 2018
bumping this. I'd really love to not use three commands to reopen an issue. :)
There's still an audit trail, even if someone deletes the comment.
Interesting - if I comment /close, the bot closes the issue and I delete my comment, is there still an audit trail of me deleting the comment? I could be totally wrong here :) but I always thought there was an audit trail if _someone else_ deleted _my_ comment.
nikhita
on 5 Jul 2018
bumping this. I'd really love to not use three commands to reopen an issue. :)
yeah, and three _comments_ (!) unless you want to roll the dice on winning plugin races.
Interesting - if I comment /close, the bot closes the issue and I delete my comment, is there still an audit trail of me deleting the comment? I could be totally wrong here :) but I always thought there was an audit trail if someone else deleted my comment.
At minimum I think we have Prow logs covering this, but that's not public (and probably shouldn't be unless we can split it off from k-s at least) and I'm not sure how long we retain enough info to track this. I'm pretty sure if you delete your own comment we won't have a way to find out who other than those.
That said I think we can go ahead and fix this to check for "is assignable / is collaborator" while we figure out what kind of auditing / commenting we want out of the robot. That ought to be a small change.
/assign
BenTheElder
on 5 Jul 2018
@BenTheElder Are you still working on this? If no, can I take this up?
I just used three commands to open an issue again so would love to see this fixed. :)
nikhita
on 27 Aug 2018
This fell off my radar, I'd love someone to take it up! :-)
BenTheElder
on 27 Aug 2018
This fell off my radar, I'd love someone to take it up! :-)
Awesome, I'll get to this (my time) tomorrow morning! :tada:
nikhita
on 27 Aug 2018
nikhita
on 28 Aug 2018
Related issues
sjenning
·
4Comments
xiangpengzhao
·
3Comments
cblecker
·
4Comments
fejta
·
4Comments
spzala
·
4Comments
Most helpful comment
being a collaborator / org member SGTM, if we can't trust collaborators / org members to close / open issues responsibly we have bigger issues 😬
@cblecker contribex bat signal 🦇🔦
/sig contributor-experience