Summary:
Ref T13072. See PHI361. The bug in T10746 where aborting builds didn't propagate properly to the buildable was fixed, but existing builds are still stuck "Building".
Since it doesn't look like anything will moot this before these changes promote to `stable`, just migrate these builds into "failed".
Test Plan: Ran migration, saw it affect only relevant builds and correctly fail them.
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D19091
Summary: Depends on D19089. Fixes T13079. This is likely not the final form of this, but creates a defensible extension point.
Test Plan: See T13079 for discussion.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19090
Summary:
Depends on D19088. Ref T13079.
> Any sufficiently complicated C or Fortran program contains an ad hoc, informally-specified, bug-ridden, slow implementation of half of Common Lisp.
> - Greenspun's Tenth Rule
Move us a step closer to this noble goal.
This doesn't implement any `viewer(project())` stuff but it looks like the API doesn't need to change to do that in the future.
Test Plan: Grimmaced in pain.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19089
Summary: Depends on D19087. Ref T13079. This still doesn't feel like the most clean, general system in the world, but is a step forward from hard-coded `switch()` stuff.
Test Plan:
- Jumped to `r`.
- Jumped to `a`.
- Jumped to `r poe` (multiple results).
- Jumped to `r poetry` (one result).
- Jumped to `r syzygy` (no results).
- Jumped to `p`.
- Jumped to `p robot` (multiple results); `p assessment` (one result).
- The behavior for `p <string>` has changed slightly but should be more powerful now (it's consistent with `r <string>`).
- Jumped to `s <symbol>` and `s <context>-><symbol>`.
- Jumped to `d`.
- Jumped to `f`.
- Jumped to `t`.
- Jumped to `T123`, `D123`, `@dog`, `PHID-DREV-abcd`, etc.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19088
Summary: Ref T13079. This recently-introduced Engine/EngineExtension are a good fit for adding more datasource functions in general, but we didn't think quite big enough in naming them.
Test Plan: Used quick search typeahead, hit applications/users/monograms/symbols/etc.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19087
Summary: Fixes T13080. The banner wasn't properly included in the sleep/wake logic.
Test Plan:
Mentioned `Dxxx` on a task. Enabled persistent chat to activate Quicksand. Reloaded page. Clicked `Dxxx`. Scrolled down until a changeset header appeared. Pressed back button.
- Before patch: ended up on task, with header still around.
- After patch: ended up on task, with header properly vanquished.
Pressed "forward", ended up back on the revision with the header again.
Maniphest Tasks: T13080
Differential Revision: https://secure.phabricator.com/D19086
Summary:
Depends on D19084. Fixes T13078. When `phabricator.silent` is enabled, immediately fail the "HTTP Request", "CircleCI" and "Buildkite" build steps.
This doesn't feel quite as clean as most of the other behavior of `phabricator.silent`, since these calls are not exactly notifications in the same way that email is, and failing to make these calls means that builds run differently (whereas failing to deliver email doesn't really do anything).
However, I suspect that this behavior is almost always reasonable/correct, and that we can probably get away with it until this grey area between "notifications" and "external service calls" is more clearly defined.
Test Plan:
- Created a build with HTTP, CircleCI, and Buildkite steps.
- Put install in `phabricator.silent` mode: all three steps failed with "declining, because silent" messages.
- Put install back in normal mode: all three steps made HTTP requests.
- Read updated documentation.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13078
Differential Revision: https://secure.phabricator.com/D19085
Summary: Ref T13078. The `phabricator.silent` configuration flag should disable webhook calls, since this is consistent with the documented and desired behavior.
Test Plan: Enabled `phabricator.silent`, made test hook calls, saw them fail with a "silent" failure reason.
Maniphest Tasks: T13078
Differential Revision: https://secure.phabricator.com/D19084
Summary: Revisions with blocking reviewers had this stamp built incorrectly, which cascaded into trying to use `array()` as a PHID. Recover so these tasks succeed.
Test Plan: Will deploy production.
Differential Revision: https://secure.phabricator.com/D19082
Summary:
Depends on D19078. Ref T13073. Currently, there is a narrow window where we can acquire a resource after a reclaim has started against it.
To prevent this, briefly lock resources before acquiring them and make sure they're still good. If a resource isn't good, throw the lease back in the pool.
Test Plan:
This is tricky. You need:
- Hoax blueprint with limits and a rule where leases of a given "flavor" can only be satisfied by resources of the same flavor.
- Reduce the 3-minute "wait before resources can be released" to 3 seconds.
- Limit Hoaxes to 1.
- Allocate one "cherry" flavored Hoax and release the lease.
- Add a `sleep(15)` to `releaseResource()` in `DrydockResourceUpdateWorker`, after the `canReclaimResource()` check, with a `print`.
Now:
- Run `bin/phd debug task` in two windows.
- Run `bin/drydock lease --type host --attributes flavor=banana` in a third window.
- This will start to reclaim the existing "cherry" resource. Once one of the `phd` windows prints the "RECLAIMING" message run `bin/drydock lease --type host --attributes flavor=cherry` in a fourth window.
- Before patch: the "cherry" lease acquired immediately, then was released and destroyed moments later.
- After patch: the "cherry" lease yields.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19080
Summary:
Depends on D19077. Ref T13073. When we're using slot locks to enforce a limit (e.g., maximum of 5 simultaneous things) we currently load locks owned by the blueprint to identify which slots are likely to be free.
However, this isn't right: the blueprint doesn't own these locks. The resources do.
We still get the right behavior eventually, but we incorrectly identify that every slot lock is always free, so as the slots fill up we'll tend to guess wrong more and more often.
Instead, load the slot locks by name explicitly.
Test Plan: Implemented lock-based limiting on `HoaxBlueprint`, `var_dump()`'d the candidate locks, saw correct test state for locks. Acquired leases without releasing, got all of the slots filled without any slot lock collisions (previously, the last slot or two tended to collide a lot).
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19078
Summary:
Depends on D19076. Ref T13073. Blueprints are stored as an attribute and `setAttributes()` overwrites all attributes.
This is sorta junk but make it less obviously broken, at least.
Test Plan: Ran `bin/drydock lease --type working-copy --attributes x=y` without instantly getting a fatal about "no blueprint PHIDs".
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19077
Summary:
Depends on D19075. Ref T13073. If a lease acquires a resource but finds that the resource builds directly into a dead state (which can happen for any number of reasonable reasons), reset the lease and throw it back in the pool.
This isn't the lease's fault and it hasn't caused any side effects or done anything we can't undo, so we can safely reset it and throw it back in the pool.
Test Plan:
- Created a blueprint which throws from `allocateResource()` so that resources never activate.
- Tried to lease it with `bin/drydock lease ...`.
- Before patch: lease was broken and destroyed after a failed activation.
- After patch: lease was returned to the pool after a failed activation.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19076
Summary: Ref T13073. Depends on D19074. Update icons and UI for resource status.
Test Plan: Viewed resources in detail view and list view, saw better status icons.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19075
Summary:
Depends on D19073. Ref T13073. Give leases a normal header tag and try to wrangle their status constants a bit.
Also, try to capture the "status class" pattern a bit. Since we target PHP 5.2.3 we can't use `static::` so the actual subclass is kind of a mess. Not exactly sure if I want to stick with this or not. We could consider targeting PHP 5.3.0 instead to get `static::` / late static binding.
Test Plan: Viewed leases and lease lists, saw better and more conventional status information.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19074
Summary:
Depends on D19072. Ref T13073. Currently, you can leave leases stranded by using `^C` to interrupt the script. Handle signals and release leases on destruction if they haven't activated yet.
Also, print out more useful information before and after activation.
Test Plan: Mashed ^C while runnning `bin/drydock lease ... --trace`, saw the lease release.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19073
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: These transaction constants are flipped, which can produce the wrong result in some cases.
Test Plan: `./bin/storage upgrade -f --apply phabricator:20180208.maniphest.02.populate.php`
Differential Revision: https://secure.phabricator.com/D19054
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
