1239798 - Carry forward approval for autolanding

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Dan Minor [:dminor]]

When we fixed Bug 1175166 we never made the corresponding changes to the approval calculation, so they still expect another "ship-it" before changes can be autolanded.

Comment 1

[Steven MacLeod [:smacleod]]

This was purposeful. I'd like to see at least an obvious warning in the autoland UI for the triggerer if they are landing a patch where the author isn't L3, and any of the ship-its are carry forwards.

Comment 2

[Dan Minor [:dminor]]

Ultimately the L3 person doing the landing is responsible for what they land, but there's no harm in calling out the fact that the commits were amended after they were last reviewed in case they want to take a closer look at them. L3 people are trusted to commit to the tree, so we should not do anything more than warn.

Comment 3

[Steven MacLeod [:smacleod]]

(In reply to Dan Minor [:dminor] from comment #2)
> Ultimately the L3 person doing the landing is responsible for what they
> land, but there's no harm in calling out the fact that the commits were
> amended after they were last reviewed in case they want to take a closer
> look at them. L3 people are trusted to commit to the tree, so we should not
> do anything more than warn.

Totally agree with you. I just want to make this case have a clear warning so that someone doesn't go to land a change without even sanity checking that either a) I really trust the person who posted this, I won't bother looking at the new diff at all or b) I've quickly glanced at the new diff and it doesn't seem problematic (whether through malice or accident)

Comment 4

[Dan Minor [:dminor]]

Attachment: MozReview Request: mozreview: carry forward approval for autolanding (Bug 1239798) r?smacleod

Currently we require re-review on commits made by non-L3 users which have
been amended. This limits the utility of autoland as a replacement for the
manual Bugzilla "check-in needed" process. Instead, we will apply the same
criteria we use for L3 users for non-L3 users and create a warning in the
UI during the autoland approval process that commits have been amended and
may need further examination prior to landing.

Review commit: https://reviewboard.mozilla.org/r/31755/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31755/

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: mozreview: warn when autolanding commits that have changed since last review(Bug 1239798) r?smacleod

This causes an warning to be displayed during autoland for each commit
that has been amended since last review.

Review commit: https://reviewboard.mozilla.org/r/31757/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31757/

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8710435 [details]
MozReview Request: mozreview: carry forward approval for autolanding (Bug 1239798) r?smacleod

https://reviewboard.mozilla.org/r/31755/#review28941

::: pylib/mozreview/mozreview/hooks.py:97
(Diff revision 1)
> -        if author_mrp.has_scm_ldap_group('scm_level_3'):
> -            # In the case of a level 3 user we really only care that they've
> -            # received a single ship-it, which is still current, from any
> -            # user. If they need to wait for reviews from other people
> -            # before landing we trust them to wait.
> +
> +        # We check whether review request has received a single "ship-it",
> +        # which is still current, from any user. We trust L3 users to only
> +        # land commits which should be landed. We will warn in the UI if
> +        # the commit has been amended since the "ship-it" was granted and
> +        # may need to be examined again.
> -            if not has_valid_shipit(review_request):
> +        if not has_valid_shipit(review_request):
> -                return False, 'A suitable reviewer has not given a "Ship It!"'
> +            return False, 'A suitable reviewer has not given a "Ship It!"'
> -        else:
> -            if not has_l3_shipit(review_request):
> -                return False, 'A suitable reviewer has not given a "Ship It!"'

This change has the side effect of allowing non L3 reviewers to grant landing approval to non L3 authors - meaning the only place L3 is considered is for who can actually press the "Land Commits" button.

While this does follow the policy for the level system, we've thrown away our use of the level system as a proxy for the module system (which wasn't the greatest scheme, but it's all we had). While this might be something we want to do, I don't think we should make this decision hastily.

The other issue is it's an important change in behaviour which we aren't indicating in any way. EX: An L3 user might act in a checkin-needed sort of capacity and click the land button for a non L3 author, assuming that the approval means someone with the authority to review the change has given a ship-it.

I'd prefer if we add in the carryforward and warn without this extra change first and give this some more discussion.

The way I'd suggest doing is making the change inside of `has_l3_shipit` to not actually check the diffset timestamp, so that the l3 shipits are carried forward. (It would actually be nice though if the diffset checking functionality was programmed in some general way and left around so your next commit can callout to review_helpers)

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8710436 [details]
MozReview Request: mozreview: warn when autolanding commits that have changed since last review(Bug 1239798) r?smacleod

https://reviewboard.mozilla.org/r/31757/#review28945

::: pylib/mozreview/mozreview/resources/commit_rewrite.py:71
(Diff revision 1)
> +            # Detect if the commit has been changed since the last review.
> +            approval_carryforward = False
> +            last_review = child_request.get_public_reviews().order_by('-timestamp')[0]
> +            diffset_history = child_request.diffset_history
> +            if diffset_history and diffset_history.last_diff_updated:
> +                if last_review.timestamp <= diffset_history.last_diff_updated:
> +                    approval_carryforward = True

I'd really like to see this logic centralized in review_helpers.py

::: pylib/mozreview/mozreview/resources/commit_rewrite.py:78
(Diff revision 1)
> +            else:
> +                logging.warning('Review request missing diffset history '
> +                                'when performing commit rewrite %s' %
> +                                child_request.id)

When were you seeing this happen? This seems like a pretty serioues failure case where we should stop the landing from happening.

Comment 8

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/31755/#review28941

> This change has the side effect of allowing non L3 reviewers to grant landing approval to non L3 authors - meaning the only place L3 is considered is for who can actually press the "Land Commits" button.
> 
> While this does follow the policy for the level system, we've thrown away our use of the level system as a proxy for the module system (which wasn't the greatest scheme, but it's all we had). While this might be something we want to do, I don't think we should make this decision hastily.
> 
> The other issue is it's an important change in behaviour which we aren't indicating in any way. EX: An L3 user might act in a checkin-needed sort of capacity and click the land button for a non L3 author, assuming that the approval means someone with the authority to review the change has given a ship-it.
> 
> I'd prefer if we add in the carryforward and warn without this extra change first and give this some more discussion.
> 
> The way I'd suggest doing is making the change inside of `has_l3_shipit` to not actually check the diffset timestamp, so that the l3 shipits are carried forward. (It would actually be nice though if the diffset checking functionality was programmed in some general way and left around so your next commit can callout to review_helpers)

While I think we need to change the current behaviour, I agree we should do this as part of a separate bug. Filed Bug 1242681.

Comment 9

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/31757/#review28945

> When were you seeing this happen? This seems like a pretty serioues failure case where we should stop the landing from happening.

Never saw it happen, but with the refactoring to move this to review_helpers.py, this code isn't relevant anymore anyway.

Comment 10

[Dan Minor [:dminor]]

Comment on attachment 8710435 [details]
MozReview Request: mozreview: carry forward approval for autolanding (Bug 1239798) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31755/diff/1-2/

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 8710436 [details]
MozReview Request: mozreview: warn when autolanding commits that have changed since last review(Bug 1239798) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31757/diff/1-2/

Comment 12

[Steven MacLeod [:smacleod]]

Comment on attachment 8710435 [details]
MozReview Request: mozreview: carry forward approval for autolanding (Bug 1239798) r?smacleod

https://reviewboard.mozilla.org/r/31755/#review29179

::: pylib/mozreview/mozreview/review_helpers.py:121
(Diff revision 2)
> +    for review in gen_latest_reviews(review_request):

There is potential to unify all these code paths which look over the reviews into a single one which returns metadata rather than just a bool - we should probably do that eventually.

Comment 13

[Steven MacLeod [:smacleod]]

Comment on attachment 8710436 [details]
MozReview Request: mozreview: warn when autolanding commits that have changed since last review(Bug 1239798) r?smacleod

https://reviewboard.mozilla.org/r/31757/#review29183