Summary: Depends on D19071. Ref T13073. While the daemons are supposedly doing things, show the user any logs they generate. There's often something relevant but unearthing it can be involved.
Test Plan: {F5427773}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19072
Summary: Depends on D19070. Ref T13073. Some messages contain an interesting story or a clever anecdote. Respect newlines during rendering to preserve authorial intent.
Test Plan:
Viewed a message with linebreaks and could still read it.
{F5427754}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19071
Summary:
Ref T13073. When a Blueprint says it will be able to allocate a resource but then throws an exception while attempting that allocation, we currently fail the lease permanently.
This is excessively harsh. This blueprint may have the best of intentions and have encountered a legitimately unforseeable failure (like a `vm.new` call to build a VM failed) and be able to succeed in the future.
Even if this blueprint is a dirty liar, other blueprints (or existing resources) may be able to satisfy the lease in the future.
Even if every blueprint is implemented incorrectly, leaving the lease alive lets it converge to success after the blueprints are fixed.
Instead of failing, log the issue and yield.
(In the future, it might make sense to distinguish more narrowly between "actually, all the resources are used up" and all other failure types, since the former is likely more routine and less concerning.)
Test Plan:
- Wrote a broken `Hoax` blueprint which always claims it can allocate but never actually allocates (just `throw` in `allocateResource()`).
- Used `bin/phd drydock lease` to acquire a Hoax lease.
- Before patch: lease abruptly failed permanently.
- After patch: lease yields after allocation fails.
{F5427747}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19070
Summary: Like "Commit Hook" rules, these also fire oddly and don't have an object PHID or a list of transactions.
Test Plan: Verified that "Call Webhooks" was no longer available from Diff rules, but still available from other rule types.
Differential Revision: https://secure.phabricator.com/D19069
Summary:
See PHI346. Ref T13054. If you have prototypes enabled on the server but use `master` / `stable` on the client and run `arc diff --plan-changes`, the transition is rejected because "Draft -> Changes Planned" isn't currently a legal transition.
Allow this transition if not coming from the web UI (to keep it out of the dropdown).
Test Plan:
- Ran `arc diff --plan-changes` on `master`, got a "Changes Planned" revision instead of a validation error.
- Ran `arc diff` without `--plan-changes`, got a draft, verified that "Plan Changes" still doesn't appear in the action dropdown.
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19067
Summary:
Depends on D19065. Ref T13054. Instead of just updating `containerPHID` and hoping for the best, queue a proper BuildWorker to process a "your container has changed, update it" message.
We also need to remove a (superfluous) `withContainerPHIDs()` when loading active diffs for a revision.
Test Plan:
- Without daemons, created a revision and saw builds stick in "preparing" with no container PHID, but also stay in draft mode.
- With daemons, saw builds actually build and get the right container PHID.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19066
Summary:
Depends on D19064. Ref T13054. See that task for additional discussion.
When buildables are created by `arc` and have lint/unit messages, they can currently pass or fail before Herald triggers actual builds. This puts them in a pre-build state where they can't complete until Herald says it's okay.
On its own, this change intentionally strands `arc diff --only` diffs in the "PREPARING" stage forever.
Test Plan:
- Ran a build with `bin/harbormaster`, saw it build normally.
- Ran a build with web UI, saw it build normally.
- Ran a build with `arc diff`, saw it build normally.
- Ran a build with `arc diff --only`, saw it hang in "PREPARING" forever.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19065
Summary: Depends on D19063. Ref T13054. Prepare for the addition of a new `PREPARING` status by getting rid of the "scattered mess of switch statements" pattern of status management.
Test Plan: Searched/browsed buildables. Viewed buildables. Viewed revisions. Grepped for all affected symbols.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19064
Summary: Ref T13054. Companion storage change for D19062.
Test Plan: Applied migration and adjustments. Viewed messages in Harbormaster; created them with `harbormaster.sendmessage`; processed them with `bin/phd debug task`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19063
Summary:
See T13054. This prepares for Buildables to be sent messages ("attach", "done scheduling builds") to fix races between Harbormaster and Differential.
The `buildTargetPHID` is replaced with a `recipientPHID` in the API. An additional change will fix the storage.
In the future, this table could probably also replace `HarbormasterBuildCommand` now, which is approximately the same bus, but for Builds.
Test Plan: Viewed builds with messages. Sent messages with `harbormaster.sendmessage`. Processed messages with `bin/phd debug task`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19062
Summary: Fixes T13071. See that task for discusison. I think this `<= version` constraint is needless in normal cases (it should match everything in the table anyway), and slightly harmful in bizarre cases where a draft somehow gets a much larger ID than it should have.
Test Plan:
- Gave a draft an unreasonably large ID.
- Pre-patch, observed: submitting comments on the draft's object does not clear the draft.
- Post-patch: submitting comments on the draft's object now clears the draft correctly.
- Also added comments/actions, reloaded pages, saw drafts stick properly.
Maniphest Tasks: T13071
Differential Revision: https://secure.phabricator.com/D19060
Summary: Ref T13054. Fixes T12714. Applies read locks to all transactions instead of only a very select subset (chat messages in Conpherence).
Test Plan: See <T13054#235650> for discussion and testing.
Maniphest Tasks: T13054, T12714
Differential Revision: https://secure.phabricator.com/D19059
Summary: Depends on D19056. Fixes T8475. Ref T13054. Merges "ModernHunk" back into "Hunk".
Test Plan: Grepped for `modernhunk`. Reviewed revisions. Created a new revision. Used `bin/differential migrate-hunk` to migrate hunks between storage formats and back.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19057
Summary: Ref T13054. Ref T8475. This table has had no readers or writers for more than a year after it was migrated to the modern table.
Test Plan: Ran migration, verified that all the data was still around.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19056
Summary:
Ref T13054. Fixes T10746. Fixes T11154. This is really a one-line fix (include `ABORTED` in `BuildEngine->updateBuildable()`) but try to structure the code a little more clearly too and reduce (at least slightly) the number of random lists of status attributes spread throughout the codebase.
Also add a header tag for buildable status.
Test Plan: Aborted a build, saw buildable fail properly.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054, T11154, T10746
Differential Revision: https://secure.phabricator.com/D19055
Summary: See PHI358. The `bin/almanac [un]trust-key` workflows don't properly purge the SSH key cache, but should.
Test Plan:
- Added key `ssh-rsa xyz` to a device.
- Used `bin/ssh-auth | grep xyz` to test for the presence of the key.
- Before patch: Saw it not present, trusted it, saw it still not present.
- After patch: Saw it not present, trusted it, saw it now present. Untrusted it, saw it no longer present.
Differential Revision: https://secure.phabricator.com/D19053
Summary:
Fixes T6121. See PHI357.
- Allow emoji and other unicode (like Chinese characters) as long as you have at least three of them.
- Disallow macros with only latin symbols. These were previously allowed.
Test Plan: Created a macro for "🐶🐶🐶", then used it in a comment.
Maniphest Tasks: T6121
Differential Revision: https://secure.phabricator.com/D19051
Summary: Depends on D19049. Ref T11330. Adds some documentation for webhooks.
Test Plan: Read the documentation and found it to be exceptionally accurate and helpful.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19050
Summary: Depends on D19048. Fixes T11330.
Test Plan: Wrote rules to call webhooks selectively, saw them fire appropriately with correct trigger attribution.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19049
Summary: Depends on D19047. Ref T11330. Triggers every firehose hook on every edit; prepares for Herald triggers.
Test Plan: Configured a firehose hook, edited some objects, saw callbacks.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19048
Summary: Depends on D19046. Ref T11330. Supports querying for specific transactions while responding to webhooks.
Test Plan: Called `transaction.search` with and without PHID constraints.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19047
Summary:
Depends on D19045. Ref T11330.
- View/regenerate HMAC keys.
- Pretty JSON.
- Readable status transactions.
- test, silent, secure flags.
- Dates on request view.
- More icons.
- Can test any object.
- GC for requests.
Test Plan: Went through each feature poking at it in the web UI and with `bin/webhook call ...` / `bin/garbage collect ...`.
Subscribers: ftdysa
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19046
Summary: Ref T11330. Adds general support for webhooks. This is still rough and missing a lot of pieces -- and not yet useful for anything -- but can make HTTP requests.
Test Plan: Used `bin/webhook call ...` to complete requests to a test endpoint.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19045
Summary: I made the red stronger (always visible, not just a hover state) for the "Mute" feature, but this made Logout look a little intense. Just make it normal-colored, logging out isn't a big deal.
Test Plan: No longer saw bright red logout action in profile dropdown menu.
Differential Revision: https://secure.phabricator.com/D19044
Summary: Ref T12677. Skip these checks if we're doing the new stuff. Also, allow priority to be unspecified.
Test Plan: Will deploy.
Maniphest Tasks: T12677
Differential Revision: https://secure.phabricator.com/D19043
Summary: See PHI356. Adds inline comment and done counts to the filetree. Also makes the filetree wider by default.
Test Plan: Fiddled with filetrees in different browsers on different revisions. Added inlines, marked them done/undone.
Differential Revision: https://secure.phabricator.com/D19041
Summary: See PHI356. Makes it easier to pick out change types in the filetree view in Differential.
Test Plan: Created a diff with adds, copies, moves, deletions, and binary files. Viewed in Differential, had an easier time picking stuff out.
Differential Revision: https://secure.phabricator.com/D19040
Summary: Depends on D19038. Fixes T4434. Updates the SearchEngine and Query to handle these fields.
Test Plan: Filtered and ordered by date and closer.
Maniphest Tasks: T4434
Differential Revision: https://secure.phabricator.com/D19039
Summary:
Depends on D19037. Ref T4434. Adds closed date to `maniphest.search` and "Export Data".
When a task has been closed, show the closed date with a checkmark in the UI instead of the modified date.
Test Plan:
- Exported data to CSV, saw close information.
- Saw close information in `/maniphest/`.
- Queried for close information via `maniphest.search`.
Maniphest Tasks: T4434
Differential Revision: https://secure.phabricator.com/D19038
Summary:
Ref T4434. Although some of the use cases for this data are better fits for Facts, this data is reasonable to track separately.
I have an approximate view of it already ("closed, ordered by date modified") that's useful to review things that were fixed recently. This lets us make that view more effective.
This just adds (and populates) the storage. Followups will add Conduit, Export, Search, and UI support.
This is slightly tricky because merges work oddly (see T13020).
Test Plan:
- Ran migration, checked database for sensible results.
- Created a task in open/closed status, got the right database values.
- Modified a task to close/open it, got the right values.
- Merged an open task, got updates.
Maniphest Tasks: T4434
Differential Revision: https://secure.phabricator.com/D19037
Summary:
See D18176. This query has no effect (other than wasting resources) and the result is unused.
`$repository` already has the URI loaded because we load them unconditionally during request initialization.
Test Plan: Viewed repository URIs.
Subscribers: jmeador
Differential Revision: https://secure.phabricator.com/D19036
Summary: Ref T13053. See PHI126. Add an explicit "Mute" action to kill mail and notifications for a particular object.
Test Plan: Muted and umuted an object while interacting with it. Saw mail route appropriately.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19033
Summary:
Ref T13053. Fixes T7804. Adds "Acting user" so you can have "always email me" stuff skip things you did or keep an eye on suspicious interns.
For the test console, the current user is the acting user.
For pushes, the pusher is the acting user.
Test Plan: Wrote acting user rules, triggered them via test console and via multiple actors on real objects.
Maniphest Tasks: T13053, T7804
Differential Revision: https://secure.phabricator.com/D19031
Summary: Fixes T10189. Ref T13053. We haven't sent these headers in a very long time. Briefly mention the new stamps header instead, although I expect to integrate stamp documentation into the UI in a more cohesive way in the future.
Test Plan: Read documentation.
Maniphest Tasks: T13053, T10189
Differential Revision: https://secure.phabricator.com/D19030
Summary: Depends on D19028. Ref T13053. Fixes T6576. An HTML body was built here, but not passed to the actual mail message.
Test Plan: Will verify production push mail.
Maniphest Tasks: T13053, T6576
Differential Revision: https://secure.phabricator.com/D19029
Summary: Ref T13053. Some mail (like push notification mail) doesn't currently generate any stamps. Drop this section if there aren't any stamps on the mail.
Test Plan: Will check push mail in production.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19028
Summary: Ref T13053. Uses the changes in D19026 to escape mail addresses. Those rules may not be right yet, but they're at least all in one place, have test coverage, and aren't obviously incorrect.
Test Plan: Will vet this more extensively when re-testing all mailers.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19027
Summary:
Ref T13053. Postmark support recommends testing requests against a whitelist of known remote addresses to determine request authenticity. Today, the list can be found here:
<https://postmarkapp.com/support/article/800-ips-for-firewalls>
This is potentially less robust than, e.g., HMAC verification, since they may need to add new datacenters or support IPv6 or something. Users might also have weird network topologies where everything is proxied, and this makes testing/simulating more difficult.
Allow users to configure the list so that they don't need to hack things apart if Postmark adds a new datacenter or remote addresses are unreliable for some other reason, but ship with safe defaults for today.
Test Plan:
Tried to make local requests, got kicked out. Added `0.0.0.0/0` to the list, stopped getting kicked out.
I don't have a convenient way to route real Postmark traffic to my development laptop with an authentic remote address so I haven't verified that the published remote address is legitimate, but I'll vet that in production when I go through all the other mailers.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19025
Summary:
See support email. There's nothing tricky here, we were just missing a check. The different parts of this got built at different times so I think this was simply overlooked.
Also add a redundant check just to future-proof this and be on the safe side.
Test Plan: Used `bin/phortune invoice` to charge a pact subscription. After deleting the card, the charge failed with an appropriate error.
Reviewers: amckinley
Differential Revision: https://secure.phabricator.com/D19020
Summary:
Depends on D19021. Ref T13053. When you "Subscribe", or make some other types of edits, we don't necessarily have reviewer data, but may now need it to do the new recipient list logic.
I don't have a totally clean way to deal with this in the general case in mind, but just load it for now so that things don't fatal.
Test Plan: Subscribed to a revision with the "Subscribe" action.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19022
Summary:
Depends on D19019. Ref T13053. Fixes T12689. See PHI178.
Currently, if `@alice` resigns from a revision but `#alice-fan-club` is still a subscriber or reviewer, she'll continue to get mail. This is undesirable.
When users are associated with an object but have explicitly disengaged in an individal role (currently, only resign in audit/differential) mark them "unexpandable", so that they can no longer be included through implicit membership in a group (a project or package).
`@alice` can still get mail if she's a explicit recipient: as an author, owner, or if she adds herself back as a subscriber.
Test Plan:
- Added `@ducker` and `#users-named-ducker` as reviewers. Ducker got mail.
- Resigned as ducker, stopped getting future mail.
- Subscribed explicitly, got mail again.
- (Plus some `var_dump()` sanity checking in the internals.)
Reviewers: amckinley
Maniphest Tasks: T13053, T12689
Differential Revision: https://secure.phabricator.com/D19021
Summary:
Depends on D19018. Fixes T4776. Ref T13053. When you remove someone from an object's recipient list (for example, by removing them a reviewer, auditor, subscriber, owner or author) we currently do not send them mail about it because they're no longer connected to the object.
In many of these cases (Commandeer, Reassign) the actual action in the UI adds them back to the object somehow (as a reviewer or subscriber, respectively) so this doesn't actually matter. However, there's no recovery mechanism for reviewer or subscriber removal.
This is slightly bad from a policy/threat viewpoint since it means an attacker can remove all the recipients of an object "somewhat" silently. This isn't really silent, but it's less un-silent than it should be.
It's also just not very good from a human interaction perspective: if Alice removes Bob as a reviewer, possibly "against his will", he should be notified about that. In the good case, Alice wrote a nice goodbye note that he should get to read. In the bad case, he should get a chance to correct the mistake.
Also add a `removed(@user)` mail stamp so you can route these locally if you want.
Test Plan:
- Created and edited some different objects without catching anything broken.
- Removed subscribers from tasks, saw the final email include the removed recipients with a `removed()` stamp.
I'm not totally sure this doesn't have any surprising behavior or break any weird objects, but I think anything that crops up should be easy to fix.
Reviewers: amckinley
Subscribers: sophiebits
Maniphest Tasks: T13053, T4776
Differential Revision: https://secure.phabricator.com/D19019
Summary:
Depends on D19017. Fixes T12491. Ref T13053. After SES threw us in the dungeon for sending mail to a spamtrap we changed outbound mail rules to stop sending to unverified addresses, except a small amount of registration mail which we can't avoid.
However, we'll still reply to random inbound messages with a helpful error, even if the sender is unverified.
Instead, only send exception mail back if we know who the sender is.
Test Plan: Processed inbound mail with `scripts/mail/mail_handler.php`. No more outbound mail for "bad address", etc. Still got outbound mail for "unknown command !quack".
Reviewers: amckinley
Maniphest Tasks: T13053, T12491
Differential Revision: https://secure.phabricator.com/D19018
Summary: Depends on D19016. Ref T13053. Adds a listener for the Postmark webhook.
Test Plan:
Processed some test mail locally, at least:
{F5416053}
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19017
Summary:
Depends on D19015. Ref T13053. Currently, we don't link up hyperlinks in the body of mail viewed in the web UI. We should, but this is a little tricky (see T13053#235074).
As a general improvement to make working with "Must Encrypt" mail less painful, add a big button to jump to the related object.
Test Plan: {F5415990}
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19016
