Navigation Menu

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.

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

proposal: gerrit: allow approvers to re-run trybots and fix commit messages #29285

Closed
mvdan opened this issue Dec 15, 2018 · 10 comments
Closed

proposal: gerrit: allow approvers to re-run trybots and fix commit messages #29285

mvdan opened this issue Dec 15, 2018 · 10 comments
Labels
DevExp anything around developer experience ExpertNeeded FrozenDueToAge Proposal Proposal-Accepted
Milestone
Unreleased

Comments

@mvdan
Copy link
Member

mvdan commented Dec 15, 2018

A few weeks ago, I sent this message to golang-dev https://groups.google.com/d/msg/golang-dev/tuvyX9j0cQg/bSzVRyeEBwAJ):

I'd like to suggest that all members of the Gerrit "approvers" team, who can already +2 CLs and submit them, gain the following permission:

https://gerrit-review.googlesource.com/Documentation/access-control.html#category_forge_author

This is useful in a couple of common scenarios when reviewing a CL to submit it:

  • To start the trybots again, if they encountered an unrelated flake or failure
  • To fix small typos in commit messages before submission, without waiting to hear back from them

I realise that the permission can be abused to impersonate others, but it seems about as dangerous as approving and submitting any patch :)

I've also seen some Go team members removing flaky "Trybot-Result-1" labels to kick the trybots again without a rebase, but I don't know what kind of permission allows that. A rebase should be enough in any case.

@andybons pointed out that we may be able to accomplish the two example use-cases without giving all these people powers to impersonate other contributors. That would be great, if it's possible.

This proposal is first to decide if we'd want all approvers to be able to restart the trybots and edit commit messages. I think those are reasonable. Once approved, we can use the issue to track exactly how we could accomplish that without giving full "forge author" powers.

/cc @bradfitz @dmitshur @ianlancetaylor
@mvdan mvdan added ExpertNeeded Proposal DevExp anything around developer experience labels Dec 15, 2018
@mvdan mvdan added this to the Proposal milestone Dec 15, 2018
@andybons
Copy link
Member

I think it’s definitely reasonable. It’s just figuring out the mechanism within Gerrit to abide by the principle of least privilege. @FiloSottile since I’m on holiday until Jan 7.

@ianlancetaylor
Copy link
Contributor

This proposal is first to decide if we'd want all approvers to be able to restart the trybots and edit commit messages.

Fine with me.

@rsc
Copy link
Contributor

rsc commented Dec 19, 2018

If we can do it, then great. It's unclear to me that the "commit message tweak" can be separated from "code file tweak". That seems important.

@rsc
Copy link
Contributor

rsc commented Dec 19, 2018

Everyone agrees we should do this. So approving. But @andybons still needs to figure out if we can.

@rsc rsc modified the milestones: Proposal, Unreleased Dec 19, 2018
@mvdan
Copy link
Member Author

mvdan commented Dec 19, 2018

If we can do it, then great. It's unclear to me that the "commit message tweak" can be separated from "code file tweak". That seems important.

Seems like Gerrit understands this, since updating a commit message only shows "Commit message updated" on the Gerrit UI, while any other change shows "Uploaded patch set N".

If it's not possible today in Gerrit, it would be fine to allow restarting the trybots now, and leave commit message editing blocked until Gerrit adds that feature. Neither is terribly urgent, they'd just save reviewers a bit of time.

@andybons
Copy link
Member

We can give go-approvers the remove reviewer permission which would allow them to remove not only reviewers/ccs but labels as well. This seems fine to me.

From talking to the Gerrit team there is no way to allow editing of only the commit message.

@mvdan
Copy link
Member Author

mvdan commented Jan 16, 2019

We can give go-approvers the remove reviewer permission which would allow them to remove not only reviewers/ccs but labels as well. This seems fine to me.

That's great! Looking forward to having that.

From talking to the Gerrit team there is no way to allow editing of only the commit message.

That's a pity. Is it planned, or can we open a feature request about it?

@andybons
Copy link
Member

I've enabled remove reviewer permissions for everyone who can +2 changes. Let me know if you run into any issues with this.

@andybons
Copy link
Member

That's a pity. Is it planned, or can we open a feature request about it?

You can file a bug at https://bugs.chromium.org/p/gerrit/issues/list. I'm not certain whether it's planned or not.

@andybons
Copy link
Member

Closing out for now since the remove reviewers permissions was added for the appropriate people and we can't achieve what we want for commit messages without allowing everyone forge author permissions. If Gerrit's behavior changes, feel free to open a new issue.

@dmitshur dmitshur mentioned this issue Jan 21, 2019
Closed
@dmitshur dmitshur changed the title proposal: Gerrit: allow approvers to re-run trybots and fix commit messages proposal: gerrit: allow approvers to re-run trybots and fix commit messages Mar 18, 2019
@golang golang locked and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
DevExp anything around developer experience ExpertNeeded FrozenDueToAge Proposal Proposal-Accepted
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@rsc @andybons @ianlancetaylor @mvdan @gopherbot