Page MenuHomePhabricator

Allow builds to have parameters
ClosedPublic

Authored by epriestley on Oct 2 2015, 12:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 8:22 AM
Unknown Object (File)
Tue, Mar 19, 1:06 PM
Unknown Object (File)
Tue, Mar 12, 6:52 PM
Unknown Object (File)
Tue, Mar 12, 6:37 PM
Unknown Object (File)
Tue, Mar 12, 6:15 PM
Unknown Object (File)
Tue, Mar 12, 5:54 PM
Unknown Object (File)
Tue, Mar 12, 5:38 PM
Unknown Object (File)
Tue, Mar 12, 5:31 PM
Subscribers

Details

Summary

Ref T9352. See D13635. Build targets can have variables already, but let builds have them too. This mostly enables future use cases (sub-builds, more sophisticated build triggers).

Test Plan

With a custom Herald rule + action like the one in T9352, updated a revision and saw it generate multiple builds with varying parameters.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Allow builds to have parameters.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: chad, hach-que.
  • Allow "/" to appear in build variables, so build/x.y.z works.

I noticed this doesn't enforce uniqueness over build parameters, does this mean multiple requests with the same parameter will result in multiple builds?

Yes. At least for now, that's intentional.

Does this change mean that builds with no parameters will also be instantiated twice? (At the moment I believe they're forced to be unique across buildPlanPHID and buildablePHID)

Yeah -- if you have two Herald rules which run the same build plan, you'll now get two builds out of it.

There was no existing constraint that prevented this before, per se, but it was very hard (impossible in the upstream?) to get into that state because:

  • Herald implicitly de-duplicated by plan PHID within a single run of the rules.
  • Commit rules only run when the commit is pushed.
  • Revision rules only run when the revision is updated, which always creates a new buildable.
  • Manually running builds also creates a new buildable.

I did trigger multiple copies of a build earlier in T9352, but it required a custom Herald action to do it (P1860).

My assumption is that it will be reasonable to adjust Herald rules to not trigger duplicate builds, but maybe this won't hold up in practice.

I think we're reasonably likely to end up with some kind of deduplication eventually, I'm just not sold on "all parameters are the same" being the best ruleset. I don't think it's a bad ruleset, but it feels like it's a little too implicit/magical to me.

In the case of T9352, for example, I might want to add a "triggering herald rule PHID" or "triggering package PHID" parameter. Those could be pretty useful diagnostically, but would affect the "all parameters the same" deduplication rule. It seems questionable to choose a rule which prevents this kind of thing.

hach-que edited edge metadata.
This revision is now accepted and ready to land.Oct 2 2015, 1:25 PM
This revision was automatically updated to reflect the committed changes.