1514807 - Migrate checkin-needed to Phabricator

Status: RESOLVED FIXED

(Conduit :: General, enhancement, P2)

Description

[Mark Côté [:mcote]]

We should move the handling of checkin-needed to Phabricator, likely as a tag.  The sheriffs should be consulted as part of the plan.

Comment 1

[Steven MacLeod [:smacleod]]

Current sheriff documentation for checkin-needed used with Lando is https://wiki.mozilla.org/Sheriffing/How_To/Landing_checkin-needed_patches#Landing_patches_via_Lando_-_Autoland (The screenshots are already out of date, so we should definitely work with the Sheriffs to update the process/documentation and keep it up to date).

Going to brain dump a little on how this should work and what questions we need to answer:

• A #Check-in Needed "tag" type project[1] should be added to Phabricator.
  • Name: Check-in Needed
  • Description: <description explaining purpose and use (possibly linking to docs)>
  • Icon: Tag
  • Color: Not red[2]. Maybe start a new convention for tags?
  • Additional Hashtags: checkin-needed [3]
  • Visible To: "Public (No Login Required)", so that anyone can see it tagged on things, and query with it.
    • Note that this will not expose confidential revision's marked with checkin-needed, the Phabricator docs on projects[1] have more details on the security model.
  • Editable By: Administrators
  • Joinable By: No One
• The new tag can be added to revisions by any user with edit-bugs.
• Sheriffs may query for revisions with the tag, and trigger landings using Lando.
• Some process or automation must remove the tag either at landing time, or backout time.

Some details we need to figure out:

• Should sheriffs land revisions that have warnings in Lando?
  • If so, what warnings are acceptable vs unacceptable?
• Should a series of revisions be landable with #Check-in Needed?
  • Is #Check-in Needed added only to the tip of the series, since Lando will land the dependencies too?[4]
• The #Check-in Needed tag needs to be removed so that backed out changes aren't immediately re-landed.
  • Could be removed at landing time, or backout time.
  • Sheriff client tooling/Sheriff backout tooling or Lando could remove it[5].
• Where should the documentation for this process live?

[1] https://secure.phabricator.com/book/phabricator/article/projects/
[2] https://mana.mozilla.org/wiki/display/EW/Phabricator+-+Adding+a+Project
[3] So that we also match old Bugzilla convention.
[4] IMO yes... Not supporting series would be a pain and I think this model makes the most sense.
[5] Lando removing at landing time makes most sense to me, will require some Lando work and maybe a change in the security properties such as giving Lando a powerful API key, or using the sheriffs Phabricator API key when landing confidential revisions.

Comment 2

[Steven MacLeod [:smacleod]]

Hey :dkl, any thoughts on the plan I've proposed here? We're going to need to get this started soon so that we can move all landings to Lando. Let me know if you have any concerns or suggestions.

Comment 3

[David Lawrence [:dkl]]

(In reply to Steven MacLeod [:smacleod] from comment #2)

Hey :dkl, any thoughts on the plan I've proposed here? We're going to need to get this started soon so that we can move all landings to Lando. Let me know if you have any concerns or suggestions.

This seems fine to me in general. We can improve up on it as we go if we see any issues. Most of the complication will be in the
automation of removing the tag when landed but doesn't seem too difficult. And of course educating the reviewers/authors to not forget to add the tag to allow it to show up on the sheriffs radar.

dkl

Comment 4

[Kim Moir [:kmoir] ET]

Notes are here

https://docs.google.com/document/d/1vNDXqzKe1jh6BUvcbgij01i2nuvvkRvKpHL53X-lmQs/edit

Comment 5

[Kim Moir [:kmoir] ET]

smacleod - what's the status of this bug? Can it be closed or is there additional work that needs to be completed?

Comment 6

[Landry Breuil (:gaston)]

Fwiw, i havent found anywhere in the contributor docs (https://firefox-source-docs.mozilla.org/contributing/how_to_submit_a_patch.html or https://moz-conduit.readthedocs.io/en/latest/) that once you had a revision accepted by a reviewer, 'someone' had to edit the revision and put the 'Check-in Needed' tag on the revision for a sheriff to land it.

There's https://wiki.mozilla.org/Phabricator/Bugzilla_Equivalents#Request_checkin-needed_to_land_your_changes but it is only leading here without details.

Comment 7

[:glob ✱]

(In reply to Landry Breuil (:gaston) from comment #6)

Fwiw, i havent found anywhere in the contributor docs (https://firefox-source-docs.mozilla.org/contributing/how_to_submit_a_patch.html or https://moz-conduit.readthedocs.io/en/latest/) that once you had a revision accepted by a reviewer, 'someone' had to edit the revision and put the 'Check-in Needed' tag on the revision for a sheriff to land it.

The check-in needed flag is no longer part of our workflow. The reviewer should be landing code for you.

There's https://wiki.mozilla.org/Phabricator/Bugzilla_Equivalents#Request_checkin-needed_to_land_your_changes but it is only leading here without details.

https://wiki.mozilla.org/Phabricator/Bugzilla_Equivalents#Request_checkin-needed_to_land_your_changes is out of date; I'll update it tomorrow.

Comment 8

[Landry Breuil (:gaston)]

(In reply to :glob 🎈 from comment #7)

(In reply to Landry Breuil (:gaston) from comment #6)

Fwiw, i havent found anywhere in the contributor docs (https://firefox-source-docs.mozilla.org/contributing/how_to_submit_a_patch.html or https://moz-conduit.readthedocs.io/en/latest/) that once you had a revision accepted by a reviewer, 'someone' had to edit the revision and put the 'Check-in Needed' tag on the revision for a sheriff to land it.

The check-in needed flag is no longer part of our workflow. The reviewer should be landing code for you.

oh ? all the reviewers are aware that it's part of their duty to land the code they review, if the submitter doesn't have the necessary access level ? That doesnt look obvious at all nor clearly underlined in the contributing docs..

Comment 9

[:glob ✱]

https://groups.google.com/g/firefox-dev/c/3JGrgQMM6lM has the announcement (we made some noise about it at the time). I've referenced this from the wiki page.

The work to make checkin-needed fully redundant is progressing slower than expected. The end goal is to allow any patch author to land their patches with Lando once they have been approved by an appropriately "blessed" reviewer.