Summary:
Ref T4045. We have a lot of direct queries against the hunk table right now. These are messy, not really policy-aware, and limit our options on T4045.
This query is unusual (it requires changesets, and does not accept IDs). This keeps us from having to load changeset -> diff -> revision in order to do policy checks. We could also fix this with smarter policy checks and caching, but I'd rather not open that can of worms for now. This object is very low level and relatively unusual, and this small deviation from convention seems like the cleanest cut to make to keep this from snowballing.
Test Plan: Used Herald dry runs to verify that the affected rules still output the same data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045
Differential Revision: https://secure.phabricator.com/D8765
Summary: Ref T4045. These three methods are fairly copy-pastey. Provide a more formal DifferentialHunk API for querying various types of line ranges.
Test Plan: Used test console to verify that "added content", "removed content", and "changed content" rules still produce the same data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045
Differential Revision: https://secure.phabricator.com/D8764
Summary: wasn't working due to some type issues. Fixes T4756. I also made it display nicer while I was debugging this.
Test Plan: created a herald rule to block changes that added refs. git tag -a "test" -m "test test"; git push origin test got me blocked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4756
Differential Revision: https://secure.phabricator.com/D8724
Summary:
Fixes T4636. If a user manually deletes a "repository" setting from a revision, Herald attempts to resolve it. Instead, Herald should now just trust Differential. Generally, the new logic is:
- When diffs are created, figure out repository information.
- When revisions are updated, copy info from diffs.
- Everywhere else, just trust the revision field.
Test Plan:
- Created revisions.
- Used Herald to dry-run revisions before and after a manual edit to remove the repository setting.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4636
Differential Revision: https://secure.phabricator.com/D8576
Summary: Fixes T4403. Supports the "send an email" action in Maniphest.
Test Plan: Wrote a "email duck" rule, then commented on a task and saw "duck" get an email.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4403
Differential Revision: https://secure.phabricator.com/D8529
Summary:
Fixes T4594. Also, allow "exists" / "does not exist" to be run against author/committer. This allows construction of rules like:
- Committer identities must be authentic.
- Committer identities must be resolvable.
- Author identities must be resolvable.
Test Plan: Created some rules using these new rules and ran them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4594
Differential Revision: https://secure.phabricator.com/D8507
Summary: Useful in cases where there is an Arcanist Project but not a repository tracked by Phabricator for a particular revision.
Test Plan: Created a new rule to flag Differential revisions with a particular Arcanist project, verified that it applied as expected via the test console to revisions with the project specified and with a different project specified.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8463
Summary:
Ref T2222.
- Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
- Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
- Moves Releeph churn field off `DifferentialCommentQuery`.
- Removes dead code in `DifferentialRevisionViewController`.
- Removes `DifferentialException` (no references).
- Removes `DifferentialRevision->loadComments()` (no callsites).
- Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
- Removes `DifferentialCommentQuery` (all callsites updated).
Test Plan: Mostly a lot of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8476
Summary:
Ref T2222. Ref T4484. See D8404 for discussion.
When a revision is updated with the new Editor, apply Herald rules. Additionally, apply them in a modern way which generates transactions.
Test Plan: {F122299}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T2222, T4484
Differential Revision: https://secure.phabricator.com/D8405
Summary:
Ref T4403. Implements "only the first time" for Maniphest rules, and fixes the trigger itself.
The trigger would never fire and block rules because it was comparing a string (like "first") to an int (like 0).
The "only" vs "every" stuff is contributed and I should have pushed back harder on this toInt / toString stuff. Maybe I'll just get rid of it; it purely causes confusion and problems.
Test Plan: Wrote an "only the first time" rule, ran it twice, it applied once.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4403
Differential Revision: https://secure.phabricator.com/D8193
Summary: It used to say "Mark with flag 7" or whatever, and now it says "Mark with flag Checkered"
Test Plan: noted previous rule I made was more understandable
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8158
Summary: turn on herald rules ability to specify other herald rules. Fixes T4294.
Test Plan: made a rule to be cc'd on new tasks. made another rule to flag a task if it contained "test test" in the title AND the cc'd rule for new tasks matched. Made some new tasks and verified new "test test" tasks were flagged.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8157
Summary: adds a new FIELD and a new VALUE to support this. Slightly dodgy because priorities do not have phids so we have to special case how we handle this in a few spots. Ref T4294.
Test Plan: made a new rule to get cc'd on unbreak now and wishlist tasks. verified got cc'd correctly and not cc'd correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8156
Summary: ...and surface it in all adapters except commit adapters. Values are true or false. Ref T4294
Test Plan: made a herald rule to be cc'd on new tasks. was cc'd on new tasks and not cc'd on updates to existing tasks.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8142
Summary:
Fixes T1353. Also some minor unrelated cleanup:
- `openTransaction()` / `saveTransaction()` exist now, fix TODOs.
- Fix some instructions.
- Make `diffusion.branchquery` return empty for SVN rather than fataling.
Test Plan:
- Added a branches rule.
- Ran a dry run against commits in different VCSes.
{F105574}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, Nopik
Maniphest Tasks: T1353
Differential Revision: https://secure.phabricator.com/D8086
Summary: See IRC. This is a likely fix for @DctrWatson's error: we're returning `null` but should return `array()` to indicate no values.
Test Plan: Will make @DctrWatson do it.
Reviewers: btrahan
Reviewed By: btrahan
CC: dctrwatson, aran
Differential Revision: https://secure.phabricator.com/D7950
Summary: This also cleans up some code a little bit. Most of the gymnastics are to make sure we call `needProjectPHIDs()` appropriately.
Test Plan: Created new commit and revision rules with this field. Ran commits and revisions through the test console. Field behavior seemed correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7923
Summary: use the loadReviewedBy function, which seems to do what we want -- returns a reviewer IFF the last thing was an accept
Test Plan: i believe in the power of loadReviewedBy
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, aarwine
Differential Revision: https://secure.phabricator.com/D7903
Summary: "Run" is clearer than "Apply". This has already been changed in Harbormaster itself.
Test Plan: used eyeballs
Reviewers: btrahan, zeeg
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7889
Summary:
Fixes T4276. This adds "Change is enormous" to pre-commit content rules so we can, e.g., just reject these and not worry about them elsewhere.
Also, use the same numeric limits across the mechanisms so there's a consistent definition of an "enormous" changeset.
Test Plan:
- Set enormous limit to 15 bytes, pushed some changes, got blocked by a rule.
- Set it back, pushed OK.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7887
Summary:
Ref T4276. When a change is larger than 2GB, PHP can not read the entire change into a string, so Herald can not process it.
Additionally, we already have a time limit for practical reasons, but it's huge (probably incorrectly). To deal with these things:
- Add an optional byte limit to `diffusion.rawdiffquery`.
- Make the query with a 1GB limit.
- Reduce the diff timeout from 15 hours to 15 minutes.
- Add a "Changeset is enormous" field. This field is true for changes which are too large to process.
This generally makes behaviors more sane:
- We'll always make progress in Herald in a reasonable amount of time.
- Installs can write global rules to handle (or reject) these types of changes.
Test Plan: Set limit to 25 bytes instead of 1GB and ran test console on various changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7885
Summary:
Fixes T4264. Adds:
- New "Repository's projects" field to Herald pre-commit rules, so you can write global rules which act based on projects.
- Allows pre-ref/pre-content rules to bind to projects, and fire for all repositories in that project, so users with limited power can write rules which apply to many repositories.
- The pre-ref and pre-content classes were starting to share a fair amount of code, so I made them both extend an abstract base class.
Test Plan: Wrote new pre-ref and pre-content rules bound to projects, then pushed commits into repositories in those projects and not in those projects. The "repository projects" field populated, and the rules fired for repositories in the relevant projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7883
Summary:
Ref T4264. Allows you to create "Object" rules, in addition to Global and Personal rules. If you choose to create an Object rule, you'll be prompted to select an object on a new screen. You must be able to edit and object in order to create rules for it.
Ref T3506. This makes "All" the default filter for the transcript view, which should reduce confusion on smaller installs.
Test Plan:
- Created non-object rules.
- Created object rules.
- Triggered object rules against matching and unmatching objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3506, T4264
Differential Revision: https://secure.phabricator.com/D7853
Summary: Ref T4264. Lays the groundwork for new "Object" rule types. Prevents personal "Hook" rules, which don't make any sense.
Test Plan: Created new Maniphest (global/personal available) and Ref Hook (global only) rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7852
Summary: Ref T4264. Instead of a dropdown, make this step more informative.
Test Plan: {F93928}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7846
Summary:
Ref T4195. A legitimate rule which needs this field is "do not allow commits as root". Interestingly, we have exactly one commit as root in each Phabricator, Arcanist and libphutil.
Since the committer and author don't need to be Phabricator accounts (just the Pusher), the existing "Committer" and "Author" fields can't express this rule (they'll be empty).
Test Plan: {F93406}
Reviewers: btrahan
Reviewed By: btrahan
CC: SEJeff, aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7841
Summary:
Fixes T4195. Allows you to write a rule against a commit's branches.
This completes outstanding work on T4195.
Test Plan: Pushed to Git and Mercurial repositories and verified branches were selected correctly by examining transcripts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7820
Summary: add support for "assignee" conditions
Test Plan: Create a Herald rule where condition is assignee, and create a task assign to someone.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7813
Summary:
Ref T4195. This allows you to write rules which disallow merge commits.
Also make the reject message a little more useful.
Test Plan:
remote: This push was rejected by Herald push rule H27.
remote: Change: commit/daed0d448404
remote: Rule: No Merges
remote: Reason: No merge commits allowed. If you must push a merge, include "@force-merge" in the commit message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7809
Summary: Refs T4195. Fixes T3936. You can't currently write rules like "block commits unless they're attached to an **accepted** revision"; allow that.
Test Plan: Pushed commits into a rule with this field, saw it work / not crash.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T3936, T4195
Differential Revision: https://secure.phabricator.com/D7807
Summary: Ref T4195. Allows you to write revision-based commit hooks, e.g. block all commits with no corresponding revision.
Test Plan:
Here's are the fields populating:
{F90989}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7806
Summary: Ref T4249. Currently, a global rule can only trigger project audits. Although there probably aren't a huge number of use cases for triggering users from global rules, it works fine and it's somewhat confusing not to allow it.
Test Plan: {F90902}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4249
Differential Revision: https://secure.phabricator.com/D7803
Summary: Fixes T4225. Adds the NON_EXISTS condition to Herald for "Reviewers", and adds a few more conditions which have reasonable meanings.
Test Plan: Used test console to check a revision with reviewers, and another without reviewers. Both produced the expected results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4225
Differential Revision: https://secure.phabricator.com/D7757
Summary: adds FIELD_PROJECTS and deploys it to Maniphest Task Herald Adapter. Went with "projects" because it feels like that could go well in other Adapters that want to conditionalize based on project.
Test Plan: made a new herald rule to be cc'd if project foo was on a task. it worked!
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7564
Summary:
Depends on D7500.
This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).
Test Plan: Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, hwinkel
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7501
Summary: The "Reviewers" condition in Differential Revision rules has the wrong typeahead and can't select projects, but should be able to.
Test Plan: {F79273}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7526
Summary:
This code is a little funky right now, and can return `array("error message")` and then try to call `getHunks()` on it. Additionally, each field loads the commit's changes separately.
Instead, load the commit's changes once and cache them, and handle exceptions appropriately.
Test Plan:
- Created a rule like "changed, added, removed content all match /.*/" to force all fields to generate.
- Ran it successfully.
- Faked an error and ran it, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: bigo, aran
Differential Revision: https://secure.phabricator.com/D7384
Summary: Ref T1279. I only tested the global case. :O
Test Plan: Created a personal "add me as blocking" rule.
Reviewers: btrahan, zeeg
Reviewed By: zeeg
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7261
Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.
Test Plan: Added Herald rules and updated revisions; see screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7244
Summary:
Ref T603. Move toward stamping out all the Project / ProjectProfile query irregularities with respect to policies.
- Fixes a bug with Asana publishing when the remote task is deleted.
- Fixes an issue with Herald commit rules.
Test Plan:
- Viewed projects;
- edited projects;
- added and removed members from projects;
- republished Asana-bridged feed stories about commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7251
Summary:
Ref T1279. This allows installs to implement two different flavors of project review. They can either implement this rule:
When:
[ ... ] [ ... ]
Take Action:
[ Add blockign reviewers ] [ Security ]
...which means "every revision matching X needs to be signed off by someone else on the Security team, //even if the author is on that team//". The alternative is to implement this rule:
When:
[ Author's projects ] [ do not include ] [ Security ]
[ ... ] [ ... ]
Take Action:
[ Add blocking reviewers ] [ Security ]
...which means that people on the Security team don't need a separate signoff from someone else on the team.
I think this weaker version maps to some of what, e.g., Google does (you need to be reviewed by someone with "readability" in a language, but if you have it that's good enough), but I could imagine cases like "Security" wanting to prevent self-review from satisfying the requirement.
@zeeg, not sure which of these use cases is relevant here, but either one should work after this.
Test Plan: Created rules with this field, verified it populated properly in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7238
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.
I will probably write some lengthy treatise on how you shouldn't use this rule later.
Implementation is straightforward because Differential previously supported this rule.
This rule can also be used to add project reviewers.
Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7235
Summary:
Ref T603. This closes the other major policy loophole in Herald, which was that you could write a rule like:
When [Always], [Add me to CC]
...and end up getting email about everything. These rules are now enforced:
- For a //personal// rule to trigger, you must be able to see the object, and you must be able to use the application the object exists in.
- In contrast, //global// rules will //always// trigger.
Also fixes some small bugs:
- Policy control access to thumbnails was overly restrictive.
- The Pholio and Maniphest Herald rules applied only the //last// "Add CC" or "Add Project" rules, since each rule overwrote previous rules.
Test Plan:
- Created "always cc me" herald and maniphest rules with a normal user.
- Created task with "user" visibility, saw CC.
- Created task with "no one" visibility, saw no CC and error message in transcript ("user can't see the object").
- Restricted Maniphest to administrators and created a task with "user" visibility. Same deal.
- Created "user" and "no one" mocks and saw CC and no CC, respectively.
- Thumbnail in Pholio worked properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7224
Summary:
Used `DifferentialRevisionQuery` with the relevant `need*()` calls in the test controller.
And started assuming the revision has reviewers and CC phids in `HeraldDifferentialRevisionAdapter`.
Test Plan:
Added herald rules that use revisions (one for revisions another for commit) and reviewers.
Created, accepted and landed a revision that matched the rules and checked all rules were applied.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D6468
Conflicts:
src/applications/herald/adapter/HeraldCommitAdapter.php
src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
src/applications/herald/controller/HeraldTestConsoleController.php
