Page MenuHomePhabricator

Make the new Build Plan "Runnable" behavior work

Authored by epriestley on Feb 28 2019, 7:41 PM.



Ref T13258. Fixes T11415. This makes "Runnable" actually do something:

  • With "Runnable" set to "If Editable" (default): to manually run, pause, resume, abort, or restart a build, you must normally be able to edit the associated build plan.
  • If you toggle "Runnable" to "If Viewable", anyone who can view the build plan may take these actions.

This is pretty straightforward since T9614 already got us pretty close to this ruleset a while ago.

Test Plan
  • Created a Build Plan, set "Can Edit" to just me, toggled "Runnable" to "If Viewable"/"If Editable", tried to take actions as another user.
  • With "If Editable", unable to run, pause, resume, abort, or restart as another user.
  • With "If Viewable", those actions work.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Owners added a subscriber: Restricted Owners Package.Feb 28 2019, 7:41 PM
amckinley added inline comments.

"You must have edit permission on this build plan to pause, abort, resume, restart, or run it manually."

This revision is now accepted and ready to land.Mar 5 2019, 7:38 PM

My phrasing is weird, but I think this replacement phrasing allows a reading as " (x, y, or z) it manually". That is, it could suggest we're talking about the ability to pause the build "manually", versus pause the build in general.

Although this is technically true today (pauses are manual), there's no other type of pause, while there are other types of "run". Notably, you can still write a Herald global rule to run a plan you can't run manually. So the "manual" part is an important qualifier for "run", but a possibly misleading qualifier for "pause/restart/etc".

I'm just going to break this into two rules/sentences, since that's probably clearer and we might refine these rules eventually anyway.

  • Describe "run" and "pause/abort/resume/restart" in two different rules to get rid of weird compound clauses.
This revision was automatically updated to reflect the committed changes.