Autoland should warn about if issues marked unsolved, rather than be unavailable in the UI

NEW
Assigned to

Status

MozReview
Autoland
P1
normal
2 years ago
10 months ago

People

(Reporter: Gijs, Assigned: glob)

Tracking

Details

(Assignee)

Comment 1

2 years ago
autoland already warns if there's an r+ on an earlier revision of a patch, but you can still land it.
updating summary (we shouldn't allow autolanding patches that completely lack an r+).

i kinda see the need to allow landing with open issues, however i think the crux of this problem is the issue workflow, and allowing autolanding with open issues is a bandaid solution.

an alternative bandaid fix for this would be to add the ability to mark all issues as fixed with a single click.
Summary: Autoland should warn about lack of r+ for latest patch / issues marked unsolved, rather than be unavailable in the UI → Autoland should warn about if issues marked unsolved, rather than be unavailable in the UI
(Reporter)

Comment 2

2 years ago
(In reply to Byron Jones ‹:glob› from comment #1)
> we shouldn't allow autolanding patches that completely lack an r+

Why not? This restriction makes it impossible to autoland patches that have been reviewed outside of reviewboard.

> i kinda see the need to allow landing with open issues, however i think the
> crux of this problem is the issue workflow

What is the problem with the issue workflow and what would be a more thorough fix?
Flags: needinfo?(glob)
(Assignee)

Comment 3

2 years ago
(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Byron Jones ‹:glob› from comment #1)
> > we shouldn't allow autolanding patches that completely lack an r+
> 
> Why not? This restriction makes it impossible to autoland patches that have
> been reviewed outside of reviewboard.

because right now autoland is part of the mozreview workflow.
changing that is outside the scope of this bug.

even if that wasn't the case, we need confirmation in a trusted system that the code has been appropriately reviewed before it can be landed; so i stand by "we shouldn't allow autolanding patches that completely lack an r+" :)

> > i kinda see the need to allow landing with open issues, however i think the
> > crux of this problem is the issue workflow
> 
> What is the problem with the issue workflow and what would be a more
> thorough fix?

i think a core issue is it isn't clear who has the responsibility for marking an issue as fixed - the developer or the reviewer?  review board is agnostic on this front - it's up to the site to decide; however that doesn't help discoverability.  a fix for this could be add a 'verified' state to issues, and ensure the labelling of the issues are clear enough to guide the expected workflow (devs mark them as fixed as they fix them, reviewers have the option to mark them as verified).  fwiw i wouldn't issues being in an unverified state to block autolanding.
Flags: needinfo?(glob)
(Reporter)

Comment 4

2 years ago
(In reply to Byron Jones ‹:glob› from comment #3)
> even if that wasn't the case, we need confirmation in a trusted system that
> the code has been appropriately reviewed before it can be landed; so i stand
> by "we shouldn't allow autolanding patches that completely lack an r+" :)

I disagree. Autoland is only available to people with L3 commit access - such people could already push to m-c and all the release branches, and it's very very very rare that we have to back people out when they have purposefully (rather than accidentally, because they pushed a combination of patches including some without r+) landed something that didn't have review. To the point where I cannot recall it happening in all the time I've been involved with the Mozilla project. If you get L3 commit access, we trust you to make those decisions. I don't think it's necessary to have the system enforce this - it's rather a hindrance, because the mozreview review state gets out of sync with bugzilla really easily, and it already doesn't make the right decisions (e.g. I've just added a reviewer to a commit I was asked to review, and then gave r+ myself, and now I can autoland even though one of the review requests is still outstanding, and I wouldn't want the original dev to land without that review being finalized, which I'm sure they wouldn't.).

> > > i kinda see the need to allow landing with open issues, however i think the
> > > crux of this problem is the issue workflow
> > 
> > What is the problem with the issue workflow and what would be a more
> > thorough fix?
> 
> i think a core issue is it isn't clear who has the responsibility for
> marking an issue as fixed - the developer or the reviewer?  review board is
> agnostic on this front - it's up to the site to decide; however that doesn't
> help discoverability.  a fix for this could be add a 'verified' state to
> issues, and ensure the labelling of the issues are clear enough to guide the
> expected workflow (devs mark them as fixed as they fix them, reviewers have
> the option to mark them as verified).  fwiw i wouldn't issues being in an
> unverified state to block autolanding.

Sorry to be blunt, but from the perspective of this issue, I don't see how having more states for issues is going to help streamline the workflow here. Marking issues as fixed/dropped is just a nuisance for me at the moment. Marking them verified doesn't really help that, nor do I think a post-landing "verification" by reviewers of whether people have fixed the nits you asked about correctly would be a useful investment of the reviewer's time.

Updated

2 years ago
Priority: -- → P1
(Assignee)

Updated

a year ago
Assignee: nobody → glob
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1357534
You need to log in before you can comment on or make changes to this bug.