Search builds based on who kicked them off
ClosedPublic
Actions

Authored by yelirekim on Jul 31 2016, 4:22 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rP4865dbdff199: Search builds based on who kicked them off

Summary

It's only natural for users to be interested their own builds. We are also building in support for other sources of builds, the only formally supported way to run a build right now is via Herald.

In our third party codebase, we designate an application as the "thing" that started builds which are scheduled and managed automatically by phabricator. I believe this is a common practice elsewhere in the codebase when you're at a loss for a real human identity and you need to apply some transactions.

Test Plan

Ran some builds manually and saw them show up under the list of things I've run. Looking up builds based on those that had been started by a herald rule.

Diff Detail

Repository

rP Phabricator

Branch

initiator_search (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 13203
Build 16912: Run Core Tests
Build 16911: arc lint + arc unit

Event Timeline

yelirekim updated this revision to Diff 39324.Jul 31 2016, 4:22 PM

yelirekim retitled this revision from to Search builds based on who kicked them off.

yelirekim updated this object.

yelirekim edited the test plan for this revision. (Show Details)

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 31 2016, 4:22 PM

Herald added a subscriber: epriestley. · View Herald Transcript

yelirekim updated this object.Jul 31 2016, 4:24 PM

yelirekim edited edge metadata.

epriestley accepted this revision.Jul 31 2016, 7:00 PM

epriestley added a reviewer: epriestley.

epriestley added inline comments.

src/applications/harbormaster/typeahead/HarbormasterBuildInitiatorDatasource.php
11	Prefer "Herald" to "herald" (one more below).
17	This could probably use `PhabricatorPeopleUserFunctionDatasource` to pick up `viewer()` and similar.
src/applications/herald/query/HeraldRuleQuery.php
53–56	`NgramIndex` is the newer and slightly better way to do this, but they're a big pain to set up. This is fine and we can swap over when someone eventually does the legwork.
src/applications/herald/storage/HeraldRule.php
37	I think this is bad news if users have 129+ character rules -- they'll be forced to run `bin/storage adjust --unsafe`, which is nonstandard. Better solutions: Use `sort255`, put the key on `name(128)` to index only the first 128 characters and doesn't run over keysize limits. Use `sort255`, but no key. Argument is: if this table gets big enough that the key matters, we should probably switch to `NgramIndex` anyway. Keep `sort128`, explicitly migrate the column so users don't need to `--unsafe`. Any of these seem fine to me.
src/applications/herald/typeahead/HeraldRuleDatasource.php
7	Prefer "Herald" to "herald" when spelling an application name.
41	I think this status is called "Archived" in Herald. Possibly we should rename it, but we should probably be consistent one way or the other.

This revision is now accepted and ready to land.Jul 31 2016, 7:00 PM

capitalization
keep herald's name field the same length, shorten index
use PhabricatorPeopleUserFunctionDatasource in the initiator datasource

Herald added a subscriber: Korvin. · View Herald TranscriptJul 31 2016, 8:36 PM

yelirekim marked 4 inline comments as done.Jul 31 2016, 8:38 PM

yelirekim added inline comments.

src/applications/herald/typeahead/HeraldRuleDatasource.php
41	I'm not sure I catch your meaning? The property on the class is `isDisabled`. Do you mean we should update the UI to match or vice versa?

The UI says "Archive":

We should keep the UI consistent, either:

do setClosed(pht('Archived')) instead of setClosed(pht('Disabled')); or
keep that "Disabled" but change "Archived" to "Disabled" in rule details / transactions / header status.

For extreme consistency we could also change the internal field name to isArchived instead of isDisabled if we stick with "Archived" in the UI, but I wouldn't bother since that's a lot of work for nearly zero value.