Summary:
Ref T4292. This is a required step in configuring a cluster: document and explain it.
Previously `bin/almanac register` could //also// add and trust keys. I've removed this capability since I think it's needless and complicated. If there's some real use for it eventually, we could add a `bin/almanac add-key` or whatever. The workflow is simpler and has better guard rails that point you in the correct direction now.
Test Plan:
- Read documentation.
- Ran `bin/almanac` with various good/bad flags.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15795
Summary: This gets over-escaped instead of bolded right now, but I only ever hit it when exporting/importing and never both cleaning it up.
Test Plan: Ran `bin/repository move-paths`, saw bolded "Move" instead of ANSI escape sequences.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15797
Summary:
Ref T10860. This allows us to recover if the connection to the database is lost during a push.
If we lose the connection to the master database during a push, we would previously freeze the repository. This is very safe, but not very operator-friendly since you have to go manually unfreeze it.
We don't need to be quite this aggressive about freezing things. The repository state is still consistent after we've "upgraded" the lock by setting `isWriting = 1`, so we're actually fine even if we lost the global lock.
Instead of just freezing the repository immediately, sit there in a loop waiting for the master to come back up for a few minutes. If it recovers, we can release the lock and everything will be OK again.
Basically, the changes are:
- If we can't release the lock at first, sit in a loop trying really hard to release it for a while.
- Add a unique lock identifier so we can be certain we're only releasing //our// lock no matter what else is going on.
- Do the version reads on the same connection holding the lock, so we can be sure we haven't lost the lock before we do that read.
Test Plan:
- Added a `sleep(10)` after accepting the write but before releasing the lock so I could run `mysqld stop` and force this issue to occur.
- Pushed like this:
```
$ echo D >> record && git commit -am D && git push
[master 707ecc3] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 254 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
BEGIN SLEEP
```
- Here, I stopped `mysqld` from the CLI in another terminal window.
```
END SLEEP
# CRITICAL. Failed to release cluster write lock!
# The connection to the master database was lost while receiving the write.
# This process will spend 300 more second(s) attempting to recover, then give up.
```
- Here, I started `mysqld` again.
```
# RECOVERED. Link to master database was restored.
# Released cluster write lock.
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
2cbf87c..707ecc3 master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15792
Summary:
Ref T10860. At least in Git over SSH, we can freely echo a bunch of stuff to stderr and Git will print it to the console, so we can tell users what's going on.
This should make debugging, etc., easier. We could tone this down a little bit once things are more stable if it's a little too chatty.
Test Plan:
```
$ echo D >> record && git commit -am D && git push
[master ca5efff] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 256 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
8616189..ca5efff master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15791
Summary: Ref T10860. This doesn't change anything, it just separates all this stuff out of `PhabricatorRepository` since I'm planning to add a bit more state to it and it's already pretty big and fairly separable.
Test Plan: Pulled, pushed, browsed Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15790
Summary:
Fixes T10865.
- Mock descriptions did not markup.
- Image descriptions did not get a proper container `<div />`.
Test Plan:
- Created a mock with remarkup in the mock description and in an image description.
- Viewed mock detail.
- Saw list styles render properly in both mock description and image description.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10865
Differential Revision: https://secure.phabricator.com/D15793
Summary: Fixes T10863. See that task for discussion.
Test Plan:
- Configured `aphlict` with no "logs".
- Started `aphlict`.
- Before change: exception.
- After change: worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10863
Differential Revision: https://secure.phabricator.com/D15788
Summary:
Ref T4292. Sometimes, we may not have a working copy for a repository. The easiest way to get into this condition is to deactivate a repository.
We could try to clone + fetch in this case, but that's kind of complex, and there's an easy command that administrators can run manually. For now, just tell them to do that.
This affects the inactive repositories on `secure`, like rGITCOINS.
Test Plan: Removed working copy, got message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15786
Summary:
Ref T7789. Currently, we use different viewers if you have `security.alternate-file-domain` configured vs if you do not.
This is largely residual from the days of one-time-tokens, and can cause messy configuration-dependent bugs like the one in T7789#172057.
Instead, always use the omnipotent viewer. Knowledge of the secret key alone is sufficient to access a file.
Test Plan:
- Disabled `security.alternate-file-domain`.
- Reproduced an issue similar to the one described on T7789.
- Applied change.
- Clean LFS interaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15784
Summary:
Ref T4292. When the daemons make a query for repository information, we need to make sure the working copy on disk is up to date before we serve the response, since we might not have the inforamtion we need to respond otherwise.
We do this automatically for almost all Diffusion methods, but this particular method is a little unusual and does not get this check for free. Add this check.
Test Plan:
- Made this code throw.
- Ran `bin/repository reparse --message ...`, saw the code get hit.
- Ran `bin/repository lookup-user ...`, saw this code get hit.
- Made this code not throw.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15783
Summary: I've possibly seen a couple of `aphlict` processes exit under suspicious circumstances (maybe?). Make sure any PHP errors get captured into the log.
Test Plan:
- Added an exception after forking.
- Before change: vanished into thin air.
- After change: visible in the log.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15782
Summary:
Fixes T10843. In a multi-server setup, we can do this:
- Two servers, A and B.
- You push an update.
- A gets pushed first.
- After A has been pushed, but before B has been pushed, a user loads a page from A.
- It generates resource URIs like `/stuff/new/package.css`.
- Those requests hit B.
- B doesn't have the new resources yet.
- It responds with old resources.
- Your CDN caches things. You now have a poisoned CDN: old data is saved in a new URL.
To try to avoid this with as little work as possible and generally make it hard to get wrong, check the URL hash against the hash we would generate.
If they don't match, serve our best guess at the resource, but don't cache it. This should make things mostly keep working during the push, but prevent caches from becoming poisoned, and everyone should get a working version of everything after the push finishes.
Test Plan:
- `curl`'d a resource, got a cacheable one.
- Changed the hash a little, `curl`'d again. This time: valid resource, but not cacheable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10843
Differential Revision: https://secure.phabricator.com/D15775
Summary: Ref T4292. This adds a new high-level overview panel.
Test Plan: {F1238854}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15772
Summary: Fixes T8897
Test Plan: Open any list view of Calendar events, every event should only show "Attending: ..." with users who are attending event.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8897
Differential Revision: https://secure.phabricator.com/D15779
Summary: This still wasn't quite right -- a link like `[[ Porcupine Facts ]]` with a space would not lookup correctly, and would render as `porcupine_facts`.
Test Plan: Verified that `[[ Porcupine Facts ]]` now works correctly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15780
Summary: Fixes T10845.
Test Plan: Verified that `[[ quack ]]` and `[[ QUACK ]]` both work. Previously, the link had to exactly match the capitalization of the target.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10845
Differential Revision: https://secure.phabricator.com/D15777
Summary: Fixes T10844. After recent changes to Aphlict (T6915 and T10697), `./bin/status` needs to be aware of the configuration file. As such, it is now necessary to run `./bin/aphlict status --config /path/to/config.json` rather than `./bin/aphlict status`.
Test Plan: Ran `./bin/aphlict start ...` and `./bin/aphlict status` and saw "Aphlict (`$PID`) is running".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10844
Differential Revision: https://secure.phabricator.com/D15776
Summary:
Fixes T8952. When Herald changes subscribers, it is zzzzz very boring.
When users change subscribers, it is still super boring (more boring than a merge, for example).
Test Plan: Viewed feed, saw fewer Herald stories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D15774
Summary:
Ref T8952. Currently, when an application (most commonly Herald, but sometimes Drydock, Diffusion, etc) publishes a feed story, we get an empty grey box for it in feed.
Instead, give the story a little application icon kind of "profile picture"-like thing.
Test Plan:
Here's how it looks:
{F1239003}
Feel free to tweak/counter-diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D15773
Summary:
Ref T10751. Add support tooling for manually prying your way out of trouble if disaster strikes.
Refine documentation, try to refer to devices as "devices" more consistently instead of sometimes calling them "nodes".
Test Plan: Promoted and demoted repository devices with `bin/repository thaw`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15768
Summary:
Ref T10751. Make the UI more useful and explain what failure states mean and how to get out of them.
The `bin/repository thaw` command does not exist yet, I'll write that soon.
Test Plan: {F1238241}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15766
Summary: Fixes T10744
Test Plan: Create recurring event, cancel one instance, cancel the parent event, reinstate event. Wording in the reinstating dialog should be clear about reinstating only instances that haven't been individually cancelled.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10744
Differential Revision: https://secure.phabricator.com/D15770
Summary: This is completely obsoleted by `owners.search`. See D15472.
Test Plan: Viewed API method in UI console.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15769
Summary:
Fixes T10840. When rendering mail, this rule wasn't falling through in quite the right way.
Also adjust where the rules are for this so the special styles show up in Maniphest, etc.
Test Plan:
Made this comment:
{F1238266}
Which produced this HTML:
{F1238267}
...and sent this mail:
{F1238283}
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T10840
Differential Revision: https://secure.phabricator.com/D15767
Summary: Ref T10751. This cleans this up so it's a little more modern, and fixes a possible bad access on the log detail page.
Test Plan: Viewed push log list, viewed push log detail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15765
Summary: This will stop breaking if you have subscribers and tags when updating a revision (`Error parsing field "Subscribers": The objects you have listed include objects which do not exist (Tags:)`), which I broke in D15749.
Test Plan: run through arc-diff --update that failed earlier.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15762
Summary:
Ref T4292. Right now, repository versions only get marked when a write happens.
This potentially creates a problem: if I pushed all the sync code to `secure` and enabled `secure002` as a repository host, the daemons would create empty copies of all the repositories on that host.
Usually, this would be fine. Most repositories have already received a write on `secure001`, so that working copy has a verison and is a leader.
However, when a write happened to a rarely-used repository (say, rKEYSTORE) that hadn't received any write recently, it might be sent to `secure002` randomly. Now, we'd try to figure out if `secure002` has the most up-to-date copy of the repository or not.
We wouldn't be able to, since we don't have any information about which node has the data on it, since we never got a write before. The old code could guess wrong and decide that `secure002` is a leader, then accept the write. Since this would bump the version on `secure002`, that would //make// it an authoritative leader, and `secure001` would synchronize from it passively (or on the next read or write), which would potentially destroy data.
Instead:
- Refuse to continue in situations like this.
- When a repository is on exactly one device, mark it as a leader with version "0".
- When a repository is created into a cluster service, mark its version as "0" on all devices (they're all leaders, since the repository is empty).
This should mean that we won't lose data no matter how much weird stuff we run into.
Test Plan:
- In single-node mode, used `repository update` to verify that `0` was written properly.
- With multiple nodes, used `repository update` to verify that we refuse to continue.
- Created a new repository, verified versions were initialized correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15761
Summary: Ref T4292. When we write a push log, also log which node received the request.
Test Plan: {F1230467}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15759
Summary:
Ref T4292. We currently synchronize hosted, clustered, Git repositories when we receive an SSH pull or push.
Additionally:
- Synchronize before HTTP reads and writes.
- Synchronize reads before Conduit requests.
We could relax Conduit eventually and allow Diffusion to say "it's OK to give me stale data".
We could also redirect some set of these actions to just go to the up-to-date host instead of connecting to a random host and synchronizing it. However, this potentially won't work as well at scale: if you have a larger number of servers, it sends all of the traffic to the leader immediately following a write. That can cause "thundering herd" issues, and isn't efficient if replicas are in different geographical regions and the write just went to the east coast but most clients are on the west coast. In large-scale cases, it's better to go to the local replica, wait for an update, then serve traffic from it -- particularly given that writes are relatively rare. But we can finesse this later once things are solid.
Test Plan:
- Pushed and pulled a Git repository over HTTP.
- Browsed a Git repository from the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15758
Summary:
Ref T4292. Before we write or read a hosted, clustered Git repository over SSH, check if another version of the repository exists on another node that is more up-to-date.
If such a version does exist, fetch that version first. This allows reads and writes of any node to always act on the most up-to-date code.
Test Plan: Faked my way through this and got a fetch via `bin/repository update`; this is difficult to test locally and needs more work before we can put it in production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15757
Summary:
Ref T4292. When you run `git fetch` and connect to, say, `repo001.west.company.com`, we'll look at the current version of the repository in other nodes in the cluster.
If `repo002.east.company.com` has a newer version of the repository, we'll fetch that version first, then respond to your request.
To do this, we need to run `git fetch repo002.east.company.com ...` and have that connect to the other host and be able to fetch data.
This change allows us to run `PHABRICATOR_AS_DEVICE=1 git fetch ...` to use device credentials to do this fetch. (Device credentials are already supported and used, they just always connect as a user right now, but these fetches should be doable without having a user. We will have a valid user when you run `git fetch` yourself, but we won't have one if the daemons notice that a repository is out of date and want to update it, so the update code should not depend on having a user.)
Test Plan:
```
$ PHABRICATOR_AS_DEVICE=1 ./bin/ssh-connect local.phacility.com
Warning: Permanently added 'local.phacility.com' (RSA) to the list of known hosts.
PTY allocation request failed on channel 0
phabricator-ssh-exec: Welcome to Phabricator.
You are logged in as device/daemon.phacility.net.
You haven't specified a command to run. This means you're requesting an interactive shell, but Phabricator does not provide an interactive shell over SSH.
Usually, you should run a command like `git clone` or `hg push` rather than connecting directly with SSH.
Supported commands are: conduit, git-lfs-authenticate, git-receive-pack, git-upload-pack, hg, svnserve.
Connection to local.phacility.com closed.
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15755
Summary:
Ref T4292. This consolidates code for figuring out which user we should connect to hosts with.
Also narrows a lock window.
Test Plan: Browsed Diffusion, pulled and pushed through an SSH proxy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15754
Summary:
In calendar, dashboard, diffusion, diviner, feed, fund,
maniphest, pholio, ponder, and slowvote use the term 'tags' if possible.
This intenctionally skips diffusion, differential, and the projects application itself.
Ref T10326 Ref T10349
Test Plan: inspection on a running, locally modified, system
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10835, T10326, T10349
Differential Revision: https://secure.phabricator.com/D15753
Summary:
Ref T10832. In practice, `git --version` is not a useful test for this issue:
- Vendors like Debian have backported the patch into custom versions like `0.0.0.1-debian-lots-of-patches.3232`.
- Vendors like Ubuntu distribute multiple different versions which report the same string from `git --version`, some of which are patched and some of which are not.
In other cases, we can perform an empirical test for the vulnerability. Here, we can not, because we can't write a 2GB path in a reasonable amount of time.
Since vendors (other than Apple) //generally// seem to be on top of this and any warning we try to raise based on `git --version` will frequently be incorrect, don't raise this warning.
I'll note this in the changelog instead.
Test Plan: Looked at setup issues, no more warning for vulnerable git version.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10832
Differential Revision: https://secure.phabricator.com/D15756
Summary:
Ref T4292. Ref T10366. Depends on D15751. Today, generating repository commands is purely a function of the repository, so they use protocols and credentials based on the repository configuration.
For example, a repository with an SSH "remote URI" always generate SSH "remote commands".
This needs to change in the future:
- After T10366, repositories won't necessarily just have one type of remote URI. They can only have one at a time still, but the repository itself won't change based on which one is currently active.
- For T4292, I need to generate intracluster commands, regardless of repository configuration. These will have different protocols and credentials.
Prepare for these cases by separating out command construction, so they'll be able to generate commands in a more flexible way.
Test Plan:
- Added unit tests.
- Browsed diffusion.
- Ran `bin/phd debug pull` to pull a bunch of repos.
- Ran daemons.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292, T10366
Differential Revision: https://secure.phabricator.com/D15752
Summary: Users can't find the "Tags" field in the Edit Menu; Added keyword "Tag".
Test Plan: Looked in Edit page; I think this shouldn't change anything else?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15749
Summary: Ref T4292. This will let the UI and future `bin/repository` tools give administrators more tools to understand problems when reporting or resolving them.
Test Plan:
- Pushed fully clean repository.
- Pushed previously-pushed repository.
- Forced write to abort, inspected useful information in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15748
Summary:
Ref T4292. Small fixes:
- There was a bug with the //first// write, where we'd write 1 but expect 0. Fix this.
- Narrow the window where we hold the `isWriting` lock: we don't need to wait for the client to finish.
- Release the lock even if something throws.
- Use a more useful variable name.
Test Plan:
- Made new writes to a fresh cluster repository.
- Made sequential writes.
- Made concurrent writes.
- Made good writes and bad writes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15747
Summary:
Fixes T10830.
- The return code from `storage adjust` did not propagate correct.
- There was one column issue which I missed the first time around because I had a bunch of unrelated stuff locally.
Test Plan:
- Ran `bin/storage upgrade -f` with failures, used `echo $?` to make sure it exited nonzero.
- Got fully clean `bin/storage adjust` by dropping all my extra local tables.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10830
Differential Revision: https://secure.phabricator.com/D15746
Summary: Ref T10832. Raise a setup warning for out-of-date versions of `git`.
Test Plan: {F1224632}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10832
Differential Revision: https://secure.phabricator.com/D15745
Summary:
Fixes T10830. Ref T10366. I wasn't writing to this table yet so I didn't build it, but the fact that `bin/storage adjust` would complain slipped my mind.
- Add the table.
- Make the tests run `adjust`. This is a little slow (a few extra seconds) but we could eventually move some steps like this to run server-side only.
Test Plan: Ran `bin/storage upgrade -f`, got a clean `adjust`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10830
Differential Revision: https://secure.phabricator.com/D15744
Summary:
Ref T10748. This supports more transaction types in the modern editor and improves validation so Conduit benefits.
You can technically create repositories via `diffusion.repository.edit` now, although they aren't very useful.
Test Plan:
- Used `diffusion.repository.edit` to create and edit repositories.
- Used `/editpro/` to edit repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15740
Summary: Ref T10748. Ref T10366. No support for editing and no impact on the UI, but get some of the basics in place.
Test Plan: {F1223279}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15742
Summary: Ref T10748. Pretty straightforward. I'd like to put a little "!" icon in the menu if there's a warning/error eventually, but can deal with that latre.
Test Plan: {F1223096}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15741
Summary:
Ref T10748. This is roughly where I'm headed, if it makes some kind of sense? The "Edit" links in sub-sections don't work yet since I haven't built the thing.
Probably depends on D15736.
Test Plan: Manually navigated to `/manage/`, clicked around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15737
Summary: Ref T10748. Ref T10337. This technically implements this stuff, but it does not do anything useful yet. This skips all the hard stuff.
Test Plan:
- Technically used `diffusion.repository.search` to get repository information.
- Technically used `diffusion.repository.edit` to change a repository name.
- Used `editpro/` to edit a repository name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10337, T10748
Differential Revision: https://secure.phabricator.com/D15736
Summary: This error message is pointless and dead-ends logged-in users needlessly if they're sent to the register page by documentation or Advanced Enterprise Sales Funnels.
Test Plan: Visited `/auth/register/` while logged in, was sent home.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15739
Summary: Also make `../` work to start relative a link so I don't have to document it as `./../path`.
Test Plan:
- Used `./`, `../`. `./../`, and normal links (proper title pickup).
- Used bad links (red).
- Regenerated documentation:
{F1221692}
Reviewers: hach-que
Reviewed By: hach-que
Differential Revision: https://secure.phabricator.com/D15734
Summary: Ref T7691 (errata). This shows links to Phriction documents in red if they're missing, and links to Phriction documents in grey with a lock icon if the user doesn't have the correct permissions to see the document.
Test Plan:
Tested a bunch of different configurations:
```
[[ ./../ ]] Back to Main Document
[[ ./../subdocument_2]] Mmmm more documents
[[ ./../invisible_document]] Mmmm more documents
[[ ./../ | Explicit Title ]] Back to Main Document
[[ ./../subdocument_2 | Explicit Title ]] Mmmm more documents
[[ ./../invisible_document | Explicit Title ]] Mmmm more documents
[[ ]] Absolute link
[[ subdocument_2 ]] Absolute link
[[ invisible_document ]] Absolute link
[[ | Explicit Title ]] Absolute link
[[ subdocument_2 | Explicit Title ]] Absolute link
[[ invisible_document | Explicit Title ]] Absolute link
```
Got the expected result:
{F1221106}
Reviewers: epriestley, chad, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T7691
Differential Revision: https://secure.phabricator.com/D15733
Summary:
Resolves T7691. This turned out more complex than I really wanted, mainly because I needed to feed the slug information through to both the document renderer and the preview window that appears in the edit controller.
After this change, you can now create relative links in Phriction by doing `[[ ./../some/relative/path ]]`. Relative paths aren't handled anywhere else (they'll still render, but the dots are turned into a literal 'dot' as per existing behaviour).
Test Plan: Created some Phriction documents with relative links, saw them all link correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T7691
Differential Revision: https://secure.phabricator.com/D15732
Summary:
This nearly works but I didn't have time to get back to it and it isn't stable enough to turn on in the cluster yet.
We have enough other stuff going out this week, so just disable it before `stable` gets cut. Should be ready by next week if things go well.
Test Plan: Fetched a Git SSH repo locally.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15731
Summary: Ref T10697. I missed this so it isn't reading the new config properly.
Test Plan: Ran `bin/aphlict stop`, saw it read config.
Reviewers: chad, Mnkras
Reviewed By: Mnkras
Subscribers: Mnkras
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15729
Summary: Fixes T9348. If you have edit permission, you can kick people out of a room.
Test Plan:
- Kicked people out of a room.
- As an unprivileged user, wasn't able to kick people out of a room.
- Hit most (all?) of the various weird dialog sub-cases.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9348
Differential Revision: https://secure.phabricator.com/D15728
Summary: Fixes T6806. We haven't seen users having issues with `phd.user` in a very long time.
Test Plan:
- Configured daemons to run as `notepriestley`, got a well-explained exception.
- Configured daemons to run as `epriestley`, got a clean start.
- Configured daemons with `phd.user=null`, got a clean start.
Reviewers: chad, areitz
Reviewed By: areitz
Subscribers: areitz
Maniphest Tasks: T6806
Differential Revision: https://secure.phabricator.com/D15726
Summary:
Ref T10756. This:
- Fixes T7307. This UI is now admin-only.
- Makes the main "running daemons" table more useful for multi-host setups (show where daemons are running).
- Removes logs from the web UI: these are sometimes vaguely sensitive and shouldn't be visible. The UI tells you how to get them with `bin/phd log`.
- Minor modernization.
Test Plan:
- As a non-admin, viewed daemons (access error) and bulk jobs (worked great).
- Browsed bulk job pages.
- Ran a bulk job.
- Viewed daemon console.
- Viewed task detail / daemon detail / daemon list pages.
{F1220516}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7307, T10756
Differential Revision: https://secure.phabricator.com/D15724
Summary: Fixes T10706
Test Plan: Open UI Example of badges. Shouldn't be broken
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: Korvin
Maniphest Tasks: T10706
Differential Revision: https://secure.phabricator.com/D15723
Summary:
Fixes T9385. This was accidentally mangled a bit a long time ago by D12797, which was a 1,000-file change which got almost everything right.
Simplify the message and fix all the `%s` conversions and how they map to parameters.
Test Plan: {F1220400}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9385
Differential Revision: https://secure.phabricator.com/D15722
Summary:
Ref T10696. By default, `node` uses 1.5GB, which is enormous overkill for this service and can crowd out other services if it's running next to things like a database on the same host.
Provide a configuration option to adjust it via `--max-old-space-size` and default to 256MB. It only seems to need about 30M locally, so this should be plenty of headroom.
Test Plan:
Ran `bin/aphlict debug`, things seemed OK.
It takes a long time (days?) to grow to 1.5GB so I can't easily test this locally without a lot of work, but I'll keep an eye on it in production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10696
Differential Revision: https://secure.phabricator.com/D15720
Summary: Fixes T10816. The way these work is a little unusual since these chunks of file-rendering code are unusuall performance-sensitive, so the Differential version doesn't adapt directly to Diffusion. Both can possibly be unified at some point in the future, although they do slightly different things.
Test Plan: {F1220170}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10816
Differential Revision: https://secure.phabricator.com/D15719
Summary:
Ref T6915. This allows multiple notification servers to talk to each other:
- Every server has a list of every other server, including itself.
- Every server generates a unique fingerprint at startup, like "XjeHuPKPBKHUmXkB".
- Every time a server gets a message, it marks it with its personal fingerprint, then sends it to every other server.
- Servers do not retransmit messages that they've already seen (already marked with their fingerprint).
- Servers learn other servers' fingerprints after they send them a message, and stop sending them messages they've already seen.
This is pretty crude, and the first message to a cluster will transmit N^2 times, but N is going to be like 3 or 4 in even the most extreme cases for a very long time.
The fingerprinting stops cycles, and stops servers from sending themselves copies of messages.
We don't need to do anything more sophisticated than this because it's fine if some notifications get lost when a server dies. Clients will reconnect after a short period of time and life will continue.
Test Plan:
- Wrote two server configs.
- Started two servers.
- Told Phabricator about all four services.
- Loaded Chrome and Safari.
- Saw them connect to different servers.
- Sent messages in one, got notifications in the other (magic!).
- Saw the fingerprinting stuff work on the console, no infinite retransmission of messages, etc.
(This pretty much just worked when I ran it the first time so I probably missed something?)
{F1218835}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6915
Differential Revision: https://secure.phabricator.com/D15711
Summary: Fixes T10812. Make it easier to disambiguate great passwords like `iI|l1oO()thenumber1nospellitout`.
Test Plan: {F1219074}
Reviewers: chad, yelirekim
Reviewed By: yelirekim
Maniphest Tasks: T10812
Differential Revision: https://secure.phabricator.com/D15715
Summary: Updating the subproject and member pages in Projects to new UI
Test Plan: Visit a subproject parent page, visit members pages
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15687
Summary: Default to "All" (maybe "Active" in the future). Adds more info to results.
Test Plan: visit /phurl/, see additional information about URL
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15713
Summary: Fixes T10806. Although browsers don't seem to care about this, it's more correct to support it, and the new test console uses normal `cURL` and does care.
Test Plan:
- Hit the error case for providing a chain but no key/cert.
- Used `openssl s_client -connect localhost:22280` to connect to local Aphlict servers.
- With SSL but no chain, saw `openssl` fail to verify the remote.
- With SSL and a chain, saw `openssl` verify the identify of the remote.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10806
Differential Revision: https://secure.phabricator.com/D15709
Summary:
Ref T10809. Currently, both the proxy and target may mutate URIs (rewriting "svn+ssh://x/diffusion/Y/" to a path on disk).
I believe this previously worked by fate/chance/luck since both URI variants contain the repository information, but the algorithms were tightened up recently with callsign removal.
Stop rewriting them if we're the intracluster proxy -- they only need to be rewritten on the target host.
Test Plan:
- Checked out a proxied SVN repository, with and without a callsign.
- Checked out an unproxied SVN repository, with and without a callsign.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10809
Differential Revision: https://secure.phabricator.com/D15712
Summary:
Fixes T10783 (what little of it remains). Ref T10697.
Aphlict currently uses request paths for two different things:
- multi-tenant instancing in the Phacility cluster (each instance gets its own namespace within an Aphlict server);
- some users configure nginx and apache to do proxying or SSL termination based on the path.
Currently, these can collide.
Put a "~" before the instance name to make it unambiguous. At some point we can possibly just use a GET parameter, but I think there was some reason I didn't do that originally and this sequence of changes is disruptive enough already.
Test Plan: Saw local Aphlict unambiguously recognize "local.phacility.com" as instance "local", with a "~"-style URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697, T10783
Differential Revision: https://secure.phabricator.com/D15705
Summary:
Fixes T10697. This finishes bringing the rest of the config up to cluster power levels.
Phabricator is now given an arbitrarily long list of notification servers.
Each Aphlict server is given an arbitrarily long list of ports to run services on.
Users are free to make them meet in the middle by proxying whatever they want to whatever else they want.
This should also accommodate clustering fairly easily in the future.
Also rewrote the status UI and changed a million other things. 🐗
Test Plan:
{F1217864}
{F1217865}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15703
Summary: Ref T10697. Mostly straightforward. Also allow the server to have multiple logs and log options in the future (e.g., different verbosities or separate admin/client logs or whatever). No specific plans for this, but the default log is pretty noisy today.
Test Plan: Set up a couple of logs, started server, saw it log to them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15702
Summary: Ref T10697. This isn't everything but starts generalizing options and moving us toward a cluster-ready state of affairs.
Test Plan: Started server in various configurations, hit most (all?) of the error cases with bad configs, sent test notifications.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15701
Summary: Ref T10697. This just improves a couple of minor `bin/aphlict` things: make argument parsing more explicit/consistent, consolidate a little bit of duplicated code.
Test Plan: Ran all `bin/aphlict` commands.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15698
Summary:
Ref T2783. This allows this worker to run on a machine different to the one that stores the repository, by routing the execution of Git over Conduit calls.
This API method is super gross, but fixing it isn't straightforward and it runs into other complicated considerations. We can fix it later; for now, just define it as "internal" to limit how much mess this creates.
"Internal" methods do not appear on the console.
Test Plan: Ran `bin/repository reparse --change <commit> --trace` on several commits, saw daemons make a Conduit call instead of running a `git` command.
Reviewers: hach-que, chad
Reviewed By: chad
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11874
Summary: Fixes T10797. This seems to fix things on my local system.
Test Plan:
- Cloned with a username, got prompted for a password.
- Cloned with a username + password.
- Cloned with a username + bad password (error).
Reviewers: chad
Reviewed By: chad
Subscribers: Grimeh
Maniphest Tasks: T10797
Differential Revision: https://secure.phabricator.com/D15706
Summary:
Ref T10784. Currently, if you terminate SSL at a load balancer (very common) and use HTTP beyond that, you have to fiddle with this setting in your premable or a `SiteConfig`.
On the balance I think this makes stuff much harder to configure without any real security benefit, so don't apply this option to intracluster requests.
Also document a lot of stuff.
Test Plan: Poked around locally but this is hard to test outside of a production cluster, I'll vet it more thoroughly on `secure`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10784
Differential Revision: https://secure.phabricator.com/D15696
Summary:
Ref T10784. On `secure`, logged-out users currently can't browse repositories when cluster/service mode is enabled because they aren't permitted to make intracluster requests.
We don't allow totally public external requests (they're hard to rate limit and users might write bots that polled `feed.query` or whatever which we'd have no way to easily disable) but it's fine to allow intracluster public requests.
Test Plan: Browsed a clustered repository while logged out locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10784
Differential Revision: https://secure.phabricator.com/D15695
Summary: Fixes T10772, not sure why this fails, but reverting the code back to old dialog call works.
Test Plan:
- Try to add a new credential when importing a repository.
- Also created a new credential normally, via Passphrase.
- Also edited a credential.
Reviewers: chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T10772
Differential Revision: https://secure.phabricator.com/D15691
Summary:
Ref T10751. We currently have a placeholder Almanac document, and a fairly-bad-advice section in Daemons.
Pull these into the modern cluster documentation.
Test Plan: 17 phabricator PHDs
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15689
Summary: Fixes T10789. If we aren't configured with a device, we never grabbed a lock in the first place, and should not expect one to be held.
Test Plan: Pushed non-cluster-configured Git SSH repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10789
Differential Revision: https://secure.phabricator.com/D15692
Summary:
Ref T4292. This mostly implements the locking/versioning logic for multi-master repositories. It is only active on Git SSH pathways, and doesn't actually do anything useful yet: it just does bookkeeping so far.
When we read (e.g., `git fetch`) the logic goes like this:
- Get the read lock (unique to device + repository).
- Read all the versions of the repository on every other device.
- If any node has a newer version:
- Fetch the newer version.
- Increment our version to be the same as the version we fetched.
- Release the read lock.
- Actually do the fetch.
This makes sure that any time you do a read, you always read the most recently acknowledged write. You may have to wait for an internal fetch to happen (this isn't actually implemented yet) but the operation will always work like you expect it to.
When we write (e.g., `git push`) the logic goes like this:
- Get the write lock (unique to the repository).
- Do all the read steps so we're up to date.
- Mark a write pending.
- Do the actual write.
- Bump our version and mark our write finished.
- Release the write lock.
This allows you to write to any replica. Again, you might have to wait for a fetch first, but everything will work like you expect.
There's one notable failure mode here: if the network connection between the repository node and the database fails during the write, the write lock might be released even though a write is ongoing.
The "isWriting" column protects against that, by staying locked if we lose our connection to the database. This will currently "freeze" the repository (prevent any new writes) until an administrator can sort things out, since it'd dangerous to continue doing writes (we may lose data).
(Since we won't actually acknowledge the write, I think, we could probably smooth this out a bit and make it self-healing //most// of the time: basically, have the broken node rewind itself by updating from another good node. But that's a little more complex.)
Test Plan:
- Pushed changes to a cluster-mode repository.
- Viewed web interface, saw "writing" flag and version changes.
- Pulled changes.
- Faked various failures, got sensible states.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15688
Summary:
Ref T4292. This adds some very basic cluster/device data to the new management view. Nothing interesting yet.
Also deal with disabled bindings a little more cleanly.
Test Plan: {F1214619}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15685
Summary:
Ref T4292. This puts a very rough skeleton in place for the new "Manage Repository" UI, somewhat similar to the "Settings" UI.
Right now, it has one panel with no content, and is not reachable from the UI.
Test Plan: {F1214525}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15683
Summary:
Ref T10756. When repositories are properly configured for the cluster (which is hard to set up today), be smart about which repositories are expected to exist on the current host, and only pull them.
This generally allows daemons to pretty much do the right thing no matter how many copies are running, although there may still be some lock contention issues that need to be sorted out.
Test Plan: {F1214483}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10756
Differential Revision: https://secure.phabricator.com/D15682
Summary: Ref T10702
Test Plan: Open a user profile, attempt to award an archived or previously awarded badge, badges dialog should provide a typeahead, and the suggestions should offer details about whether a badge is archived or already awarded.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10702
Differential Revision: https://secure.phabricator.com/D15665
Summary:
Ref T4571. Write more of the missing documentation sections and clarify a few things.
Since the "replicating master" check needs a special permission, imposes a performance penalty, is probably very difficult to misconfigure, and likely not a big deal anyway, just drop the idea of trying to automatically detect + prevent it. We still show if it's an issue on the status page, provided we have permission to check.
When you don't have any cluster databases configured, never stop trying to connect to the default master database. We might want to do this eventually as load reduction, but just don't muddy the waters too much for now while things stabilize.
Test Plan:
- Tested functionality in cluster, non-cluster, and degraded-cluster modes.
- Used status console to monitor a health check cycle.
- Read docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15679
Summary:
Ref T4571. When a database goes down briefly, we fall back to replicas.
However, this fallback is slow (not good for users) and keeps sending a lot of traffic to the master (might be bad if the root cause is load-related).
Keep track of recent connections and fully degrade into "severed" mode if we see a sequence of failures over a reasonable period of time. In this mode, we send much less traffic to the master (faster for users; less load for the database).
We do send a little bit of traffic still, and if the master recovers we'll recover back into normal mode seeing several connections in a row succeed.
This is similar to what most load balancers do when pulling web servers in and out of pools.
For now, the specific numbers are:
- We do at most one health check every 3 seconds.
- If 5 checks in a row fail or succeed, we sever or un-sever the database (so it takes about 15 seconds to switch modes).
- If the database is currently marked unhealthy, we reduce timeouts and retries when connecting to it.
Test Plan:
- Configured a bad `master`.
- Browsed around for a bit, initially saw "unrechable master" errors.
- After about 15 seconds, saw "major interruption" errors instead.
- Fixed the config for `master`.
- Browsed around for a while longer.
- After about 15 seconds, things recovered.
- Used "Cluster Databases" console to keep an eye on health checks: it now shows how many recent health checks were good:
{F1213397}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15677
Summary:
Ref T4571. If we fail to connect to the master, automatically try to degrade into a temporary read-only mode ("UNREACHABLE") for the remainder of the request, if possible.
If the request was something like "load the homepage", that'll work fine. If it was something like "submit a comment", there's nothing we can do and we just have to fail.
Detecting this condition imposes a performance penalty: every request checks the connection and gives the database a long time to respond, since we don't want to drop writes unless we have to. So the degraded mode works, but it's really slow, and may perpetuate the problem if the root issue is load-related.
This lays the groundwork for improving this case by degrading futher into a "SEVERED" mode which will persist across requests. In the future, if several requests in a short period of time fail, we'll sever the database host and refuse to try to connect to it for a little while, connecting directly to replicas instead (basically, we're "health checking" the master, like a load balancer would health check a web application server). This will give us a better (much faster) degraded mode in a major service disruption, and reduce load on the master if the root cause is load-related, giving it a better chance of recovering on its own.
Test Plan:
- Disabled master in config by changing the host/username, got degraded automatically to UNREACAHBLE mode immediately.
- Faked full SEVERED mode, requests hit replicas and put me in the mode properly.
- Made stuff work, hit some good pages.
- Hit some non-cluster pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15674
Summary: Ref T4571. If `cluster.databases` is configured but only has replicas, implicitly drop to read-only mode and send writes to a replica.
Test Plan:
- Disabled the `master`, saw Phabricator automatically degrade into read-only mode against replicas.
- (Also tested: explicit read-only mode, non-cluster mode, properly configured cluster mode).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15672
Summary:
Ref T4571. Allows users to click the "read-only mode" notification to get more information about why an install is in read-only mode.
Installs can be in this mode for several reasons (explicit administrative action, no masters defined, no masters reachable), and it's useful to be able to tell the difference.
Test Plan: {F1212930}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15671
Summary:
Ref T4571. Ref T10759. Ref T10758. This isn't complete, but gets most of the job done:
- When `cluster.databases` is set up, most things ignore `mysql.host` now.
- You can `bin/storage upgrade` and stuff works.
- You can browse around in the web UI and stuff works.
There's still a lot of weird tricky stuff to navigate, and this has real no advantages over configuring a single server yet (no automatic failover, etc).
Test Plan:
- Configured `cluster.databases` to point at my `t1.micro` hosts in EC2 (master + replica).
- Ran `bin/storage upgrade`, got a new install setup on them properly.
- Survived setup warnings, browsed around.
- Switched back to local config, ran `bin/storage upgrade`, browsed around, went through setup checks.
- Intentionally broke config (bad hosts, no masters) and things seemed to react reasonably well.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571, T10758, T10759
Differential Revision: https://secure.phabricator.com/D15668
Summary: Ref T4571. The configuration option still doesn't do anything, but add a status panel for basic setup monitoring.
Test Plan:
Here's what a good version looks like:
{F1212291}
Also faked most of the errors it can detect and got helpful diagnostic messages like this:
{F1212292}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15667
Summary:
Ref T4571. This adds a new option which allows you to upgrade your one-host configuration to a multi-host configuration by configuring it.
Doing this currently does nothing. I wrote a lot of words about what it is //supposed// to do in the future, though.
Test Plan:
- Tried to configure the option in all the possible bad ways, got errors.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15663
Summary:
Ref T4571. There will be a very long path beyond this, but add a basic read-only mode. You can explicitly enable this to put Phabricator in a sort of "maintenance" mode today if you're swapping databases or something.
In the long term, we'll automatically degrade into this mode if the master database is down.
Test Plan:
- Enabled read-only mode.
- Browsed around.
- Didn't immediately see anything that was totally 100% broken.
Most stuff is 80-90% broken right now. For example:
- Stuff like submitting comments doesn't work, and gives you a confusing, unhelpful error.
- None of the UI really knows that it's read-only. EditEngine stuff should all hide itself and say "you can't add new comments while an install is in read-only mode", for example, but currently does not.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15662
Summary: Testing out a new 'nav' layout in Settings / Config. Spent a few days here and couldn't find much better overall.
Test Plan: View each page in Settings and in Config. Save some config options. Test mobile, desktop, tablet.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15659
Summary: Fixes T5813, while I'm in here...
Test Plan: Sorted stuff by end date.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5813
Differential Revision: https://secure.phabricator.com/D15657
Summary: Fixes T10684. Fixes T10520. This primarily implements a date/epoch field, and then does a bunch of standard plumbing.
Test Plan:
- Created countdowns.
- Edited countdowns.
- Used HTTP prefilling.
- Created a countdown ending on "Christmas Morning", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10520, T10684
Differential Revision: https://secure.phabricator.com/D15655
Summary: Closes T10690
Test Plan: Open Badges application, go to Advanced Search, search for a badge by its name and see result.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T10690
Differential Revision: https://secure.phabricator.com/D15656
Summary:
Fixes T8613. This was pretty straightforward, I just never dug into it originally.
`rawResultLimit = 0` just means "no limit", so the fix is to only apply a limit if it is set to some nonzero value.
Also modernize a few pieces of code.
Test Plan: I'm actually not sure this can actually be hit normally? I faked `setGenerateGhosts(true)` into an unrelated query, hit the fatal, then fixed it.
Reviewers: lpriestley, chad
Reviewed By: chad
Maniphest Tasks: T8613
Differential Revision: https://secure.phabricator.com/D15653
Summary: Found another bouncing around.
Test Plan: Review in diff
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15651
Summary: Ref T10262. Instead of dumping an unhelpful 403 "ACCESS DENIED" page on users, explain the most likely cause of the issue and give them a link to return to the file detail page to learn more or get an up-to-date link.
Test Plan: Hit both errors, had a lovely experience with the helpful dialog text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15650
Summary: Ref T6027. We got a not-very-user-friendly default string before.
Test Plan: Selected "Move", didn't change the dropdown, hit submit. Now, got a nice human-readable description of the issue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15649
Summary: Bumps to 14px, fixes some on Differential
Test Plan: view various headers in Differential
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15647
Summary:
Ref T10262. Currently, we always render a tag like this when you `{F123}` an image in remarkup:
```
<img src="/xform/preview/abcdef/" />
```
This either generates the preview or redirects to an existing preview. This is a good behavior in general, because the preview may take a while to generate and we don't want to wait for it to generate on the server side.
However, this flickers a lot in Safari. We might be able to cache this, but we really shouldn't, since the preview URI isn't a legitimately stable/permanent one.
Instead, do a (cheap) server-side check to see if the preview already exists. If it does, return a direct URI. This gives us a stable thumbnail in Safari.
Test Plan:
- Dragged a dog picture into comment box.
- Typed text.
- Thing didn't flicker like crazy all the time in Safari.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15646
Summary: Going to render these all normal case instead of all caps, and bump up the font size. Should be more consistent. Yellow if you green anything orange.
Test Plan: grep, lint
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15645
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
Summary:
Ref T10262. Files have an internal secret key which is partially used to control access to them, and determines part of the URL you need to access them. Scramble (regenerate) the secret when:
- the view policy for the file itself changes (and the new policy is not "public" or "all users"); or
- the view policy or space for an object the file is attached to changes (and the file policy is not "public" or "all users").
This basically means that when you change the visibility of a task, any old URLs for attached files stop working and new ones are implicitly generated.
Test Plan:
- Attached a file to a task, used `SELECT * FROM file WHERE id = ...` to inspect the secret.
- Set view policy to public, same secret.
- Set view policy to me, new secret.
- Changed task view policy, new secret.
- Changed task space, new secret.
- Changed task title, same old secret.
- Added and ran unit tests which cover this behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15641
Summary:
This reverts commit 3f50ba90f1.
Fixes T10412. Everyone seems to hate this and I don't feel strongly about it. It's definitely a little weird.
Test Plan: Straight revert.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10412
Differential Revision: https://secure.phabricator.com/D15644
Summary: Fixes T10737. I agree that this is a better behavior than always making them blue (boring).
Test Plan:
what
{F1208333}
wut
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T10737
Differential Revision: https://secure.phabricator.com/D15643
Summary:
Ref T6027. Fixes T10734.
- If one of the projects a task is tagged with isn't visible to the user or doesn't have a board, it won't have columns.
- Don't show options for projects with disabled boards.
Test Plan:
- Viewed task with project with no columns; no fatal.
- Viewed task with project with disabled board; no options to move on that board.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10734, T6027
Differential Revision: https://secure.phabricator.com/D15640
Summary:
Ref T6027. Normally, actions use the same order as the form, but in some cases (like moving stuff on workboards) it makes sense to reorder them explicitly.
Pin "Move on board" near the bottom, and "projects/subscribers" at the bottom. I think these are generally reasonable rules in all cases.
Test Plan: Opened menu, saw slightly better action order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15639
Summary:
Ref T6027. Try this out and see how it feels? Clear issues:
- This definitely shouldn't be at the top.
- You should probably be able to select it multiple times?
- Some of the "which columns show up" rules might need adjustment?
- Diamond marker maybe not great?
Not sure I love this but it doesn't feel //terrible//...
Test Plan: {F1207891}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15638
Summary: Ref T5214. Fixes T10486. Ref T6027. This exposes the `TYPE_COLUMNS` transaction in a usable way via API, and fixes the interactions via prefilling.
Test Plan:
- Created tasks directly into columns via API.
- Moved tasks between columns via API.
- Used `?column=...` to try to create a template task with valid and bogus column PHIDs.
Reviewers: chad
Reviewed By: chad
Subscribers: AmyLewis
Maniphest Tasks: T5214, T6027, T10486
Differential Revision: https://secure.phabricator.com/D15636
Summary: Ref T6027. This adds human-readable rendering for the new `TYPE_COLUMNS` core transactions.
Test Plan: {F1207784}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15635
Summary:
Ref T6027. We currently have two different transaction types:
- `TYPE_PROJECT_COLUMNS` does most of the work, but has a sort of weird structure and isn't really suitable for API use.
- `TYPE_COLUMN` is this weird, junk transaction which mostly just creates the other transaction.
Merge them into a single higher-level `TYPE_COLUMNS` transaction which works properly and has a sensible structure and comprehensive error checking.
Remaining work here:
- I've removed the old rendering logic, but not yet added new logic. I need to migrate the old transaction types and add new rendering logic.
- Although the internal representation is now //suitable// for use in the API, it isn't properly exposed yet.
Test Plan:
- Created tasks into a column.
- Ran unit tests.
- Moved tasks between columns.
- Will perform additional testing in followups.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15634
Summary: Ref T10671
Test Plan: Open Conduit application, open `badges.edit` or `badges.search`, create, edit, or query for a badge.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10671
Differential Revision: https://secure.phabricator.com/D15622
Summary: Modernize Files a bit, use newPage
Test Plan: New file, drag and drop file, view file, edit file
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15631
Summary: Ref T7673. This is really just so I can force admin.phacility.com logout when you log out of an instance, but there are a few other things we could move here eventually, like the WILLREGISTERUSER event.
Test Plan: Logged out of an instance, got logged out of parent (see next change).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7673
Differential Revision: https://secure.phabricator.com/D15629
Summary:
Ref T7303. Currently, our handling of "scope" is fairly rigid and adheres to the spec, but some of these behaviors don't make much sense in practice.
Soften some behaviors and make them more flexible:
**Soft Failure on Unknown Permissions**: If a client asks for a permission we don't know about, just warn that we don't recognize it instead of fataling. In particular, I plan to make `offline_access` and `whoami` implicit. Older clients that request these permissions will still work fine as long as we don't hard-fatal.
**Move `user.whoami` to ALWAYS scope**: Make `whoami` a default permission. We've already done this, in effect; this just formalizes it.
**Tokens no longer expire**: Make `offline_access` (infinite-duration tokens) a default permission. I think the OAuth model doesn't map well to reality. It is common for other providers to issue "temporary" tokens with a duration of multiple years, and the refesh workflow is sort of silly. We can add a "temporary" scope later if we need temporary tokens.
This flow was potentially extra silly with the "log out of Phacility" use case, where we might need to have you log in again before we could log you out, which is bizarre and senseless. Avoid this nonsense.
**Move away from granular permissions**: Users currently get to pick-and-choose which permissions they grant, but this likely rarely/never works in practice and is fairly hostile since applications can't communicate which permissions they need. Applications which can actually operate with only some subset of permissions can make separate requests (e.g., when you activate "cool feature X", it asks for X permission). I think applications that do this are rare; pretty much everything just asks for tons of permissions and everyone grants them.
Making this all-or-nothing is better for well-behaved applications and better for users. It's also slightly better for overzealous applications that ask for more than they need, but whatever. Users can make an informed decision, hopefully, and I plan to let administrators force applications to a subset of permissions once we introduce meaningful scopes.
Test Plan:
- Generated tokens.
- Used tokens.
- Authorized an instance.
- Faked some bogus scopes, got clean authorization.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15621
Summary: Converts over to `newPage`
Test Plan: Pull up Settings panel, test a few.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15625
Summary: Ref T7303. This interaction is very oldschool; modernize it to enable/disable instead of "nuke from orbit".
Test Plan:
- Enabled applications.
- Disabled applications.
- Viewed applications in list view.
- Generated new tokens.
- Tried to use a token from a disabled application (got rebuffed).
- Tried to use a token from an enabled application (worked fine).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15620
Summary: Runs through Releeph to move to new UI and `newPage`
Test Plan: Ran through product, release, branch, everything seems to work.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15623
Summary: Swaps over to new hotness
Test Plan: Pull up mail view, see new UI
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15616
Summary: Swaps over to `newPage` and `newDialog`
Test Plan: Unsure how to actually pull these up?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15617
Summary: Updates to use new UI
Test Plan: Save a custom query, edit a custom query
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15618
Summary: Updates Console and Operations page.
Test Plan: Pull up Console, pull up status page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15615
Summary: Moves these Maniphest pages over to modern UI, components
Test Plan: Batch Edit Tasks, View some reports.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15614
Summary: Ref T7303. This application is currently stone-age tech (no transactions, hard "delete" action). Bring it up to modern specs.
Test Plan:
- Created and edited an OAuth application.
- Viewed transaction record.
- Tried to create something with no name, invalid redirect URI, etc. Was gently rebuffed with detailed explanatory errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15609
Summary: View various conduit pages and update to new UI and add calls to newPage
Test Plan: View list, view method, make a call.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15613
Summary: Uses modern UI, `newPage`, etc. Changes table behavior to always scroll if too large for container, can't find anything this breaks, but be on the lookout.
Test Plan: Pull up help and view pages, search for some people and projects.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15611
Summary: Single callsite, swap to `newPage`
Test Plan: Visit page, see same status message. Also remove device ready flag.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15610
Summary:
Fixes T10721. When trying to load commits by identifier, we would take some bad pathways in Subversion if the repository had no callsign and end up missing the commits.
Fix this logic so it works for either callsigns (e.g., if passed `rXyyy`) or with PHIDs if passed repositories.
Test Plan:
- Viewed SVN commit in a Subversion repository with no callsign.
- Added a callsign, looked at it again.
- Viewed non-SVN commits in callsign and non-callsign repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10721
Differential Revision: https://secure.phabricator.com/D15607
Summary: Ref T7303. Small modernization.
Test Plan:
- Searched by various users.
- Viewed all, reordered, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15606
Summary:
Ref T7303. Ref T7673. This implements an "auth.logout" which:
- terminates all web sessions;
- terminates the current OAuth token if called via OAuth; and
- may always be called via OAuth.
(Since it consumes an OAuth token, even a "malicious" OAuth application can't really be that much of a jerk with this: it can't continuously log you out, since calling the method once kills the token. The application would need to ask your permission again to get a fresh token.)
The primary goal here is to let Phacility instances call this against the Phacility upstream, so that when you log out of an instance it also logs you out of your Phacility account (possibly with a checkbox or something).
This also smooths over the session token code. Before this change, your sessions would get logged out but when you reloaded we'd tell you your session was invalid.
Instead, try to clear the invalid session before telling the user there's an issue. I think that ssentially 100% of invalid sessions are a result of something in this vein (e.g., forced logout via Settings) nowadays, since the session code is generally stable and sane and has been for a long time.
Test Plan:
- Called `auth.logout` via console, got a reasonable logout experience.
- Called `auth.logout` via OAuth.
- Tried to make another call, verified OAuth token had been invalidated.
- Verified web session had been invalidated.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303, T7673
Differential Revision: https://secure.phabricator.com/D15594
Summary:
Ref T7303. OAuth scope handling never got fully modernized and is a bit of a mess.
Also introduce implicit "ALWAYS" and "NEVER" scopes.
Always give tokens access to meta-methods like `conduit.getcapabilities` and `conduit.query`. These do not expose user information.
Test Plan:
- Used a token to call `user.whoami`.
- Used a token to call `conduit.query`.
- Used a token to try to call `user.query`, got rebuffed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15593
Summary:
Ref T7303. This inches toward properly-behaved cluster logout.
- Use IDs instead of PHIDs in URIs.
- Slightly more modern code.
- Fix some crumb stuff.
Test Plan: Created, edited, viewed, deleted, showed secret for, authorized, test-auth'd an application.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15592
Summary: Updates various /people/ pages for new UI and newPage
Test Plan: Review creating people, new people, sending invites, editing a profile, setting a new picture, something with LDAP
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15604
Summary: Cleans up Feed Story individual page
Test Plan: View an individual story by clicking on date.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15599
Summary: No UI updates, just swapping over to `newPage`
Test Plan: Pull up each page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, yelirekim
Differential Revision: https://secure.phabricator.com/D15601
Summary: Updates fund for new edit UI
Test Plan: Create Fund, Edit Fund
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15595
Summary: Updates Macro Audit Edit page with new UI and newPage
Test Plan: Edit Audio on macro, see new layout, save file.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15598
Summary: Brings the edit paths page in owners up to new UI
Test Plan: Edit some paths, yo.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15596
Summary: Closes T8940, recipient list in badge view should show awarder and date info. Took a first stab at how we want to make the date look, but not sure. Looks odd as it is.
Test Plan: Open badge that has awards. Each recipient in list should have a subheader such as "Awarded by ... on ..."
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8940
Differential Revision: https://secure.phabricator.com/D15590
Summary: Missed converting this page, scenario. The box was poorly formatted.
Test Plan: Create a new document that needs signed, verify box is correctly spaced and colored.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15591
Summary: Forgot a more efficient way to get badge from award
Test Plan: Badges on user profiles should still show up with awarder handle on the back of the card
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15589
Summary: Ref T8940
Test Plan: Award badge, open recipient profile page, badge should appear in badges list, and flipping the badge card should show who awarded it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, Korvin
Maniphest Tasks: T8940
Differential Revision: https://secure.phabricator.com/D15570
Summary: Converts Config to new UI, updates to `newPage`
Test Plan: Review all pages in Config, setup issues, ignore an issue, edit a config option
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15587
Summary: Updates Legalpad Manage/Edit with new UI layouts.
Test Plan: Wrote a new document with and without a preamble, edit document, sign document
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15576
Summary: Pulls everything over to two column UI and new edit pages. Removed history view and consolidated some pages.
Test Plan: New Panel, Edit Panel. New Dashboard, Edit Dashboard, View Standalone pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15588
Summary: Minor, moves to `newPage`
Test Plan: Test both pages, still work
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15582
Summary: Little straggler here, updates to `newPage`
Test Plan: Review a document, no visibile changes
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15580
Summary: Updates Conpherence pages to use `newPage`
Test Plan: View a Room, view list of joined rooms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15578
Summary: Just clearing these all out.
Test Plan: Visit channel list and log page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15577
Summary:
Ref T10709. Two issues:
- If a user sends an invalid `!command`, we can throw, which means we don't return HTTP 200. This makes Mailgun re-send the mail later.
- We don't parse headers of the modern API correctly, so the "Message-ID" failsafe doesn't work. Parse them correctly. I //believe// Mailgun's API changed at some point.
Test Plan:
This is difficult to test exhaustively in isolation. I used Mailgun's web tools to verify the format of the hook request, and faked some requests locally.
I'll keep an eye on this as it goes to production and make sure the fix is correct there.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10709
Differential Revision: https://secure.phabricator.com/D15575
Summary: Modernize and use newer UI
Test Plan: Bounce around various views.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15574
Summary: Updates various Phriction pages to match new UI
Test Plan: New Document, Edit Document, View History, Revert Change
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15572
Summary: Swap over to modern components, `newPage` and `handleRequest`.
Test Plan: `arc lint` :(
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D15571
Summary: Runs through Meta, cleaned up policies and editing email addresses to new UI
Test Plan: Set a new Email address for Maniphest, edit policies.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15569
Summary: Fixes T10704. This is just bad copy-paste -- "O" for "old" should be "N" for "new".
Test Plan:
- Followed steps on T10704.
- Applied patch.
- Marked inline done, replied, etc. No more JS errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10704
Differential Revision: https://secure.phabricator.com/D15566
Summary: Cleans up Pholio, moves to two column layout, fix some transaction inconsistencies. This moves "Image" to the MainColumn, which feels fine, but I think we'll likely want some sort of "fullscreen" option for Pholio V2 like we have on workboards perhaps.
Test Plan:
New Mock, Edit Mock, View Mock.
{F1200450}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15564
Summary: Ref T9007
Test Plan: Navigate to "Advanced Search" in Badges, order by rarity, then by commonality. Rarest and most common badges should be ordered, respectively.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9007
Differential Revision: https://secure.phabricator.com/D15555
Summary: This doesn't hit the ambiguous case in Diffusion so it seems fine to make it more consistent.
Test Plan: Looked at a little task-o.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15561
Summary: Walks through various object, rule, create forms and transcripts in Herald. Slightly nicer looking.
Test Plan: Make rules, see rules, edit rules, see transcripts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15559
Summary: Fixes T10672. Cleaning this up myself since I was responsible for the implementation.
Test Plan: Leave a comment, Edit a badge, create a badge.
Reviewers: lpriestley, epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10672
Differential Revision: https://secure.phabricator.com/D15556
Summary: Adds basic commenting to Fund Initiatives.
Test Plan: Leave a comment, see comment.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15554
Summary: Uses BLUE_PROPERTY on Recipients box, removes redundent properties since we render the badge itself already.
Test Plan: View a badge with and without a description.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15553
Summary: [WIP] Tossing this up for safety and to read through it. Need to test, update some of the other flows. This updates everything in Auth for new UI and modern conventions.
Test Plan: Loooots of random testing, new providers, edit providers, logging out, forgot password... more coming.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15550
Summary:
[WIP] Allows awarding a badge from a user profile. Unsure of the interactions here if a user can't award any badges, or if we should just hide this.
Fixes T10688
Fixes T10318
Test Plan: Award some badges. Steal them back.
Reviewers: lpriestley, epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10318, T10688
Differential Revision: https://secure.phabricator.com/D15544
Summary:
Ref T10537. Currently, Nuance has a `NuanceRequestor` object, intended to represent the external user who created content (e.g., a GitHub account or a Twitter account or whatever).
This object is currently almost unused, and its design predates Doorkeeper. In D15541, I chose to use doorkeeper objects instead of NuanceRequestor objects to represent requestors.
I don't currently anticipate a need for such an object, given that we have Doorkeeper. If we do need it in the future for some reason, it would be fairly easy to restore it, create a requestor type which wraps a Doorkeeper object, and then migrate. Not super thrilling to do that, but not a huge mess.
`NuanceItem` still has a `requestorPHID`, but this is now a less formal object PHID instead of a more formal Requestor-object PHID, and holds a doorkeeper exeternal object PHID for GitHub events.
Test Plan:
- Grepped for `nuancerequestor`.
- Ran `bin/storage upgrade -f`.
- Grepped for `requestor`, remaining uses of this term seem reasonable/correct.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15546
Summary: This is kinda bad in terms of UI (It just makes a json of the thing and diffs that), but it's a start.
Test Plan: edit rule, create rule, add/remove/edit conditions, actions
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15542
Summary:
Ref T10538. Ref T10537. This creates PHIDs which represent GitHub users, and uses them as the actors for synchronized comments.
I've just made them Doorkeeper objects. There are three major kinds of objects they //could// possibly be:
- Nuance requestor objects.
- External account objects.
- Doorkeeper objects.
I don't think we actually need distinct nuance requestor objects. These don't really do anything right now, and were originally created before Doorkeeper. I think Doorkeeper is a superset of nuance requestor functionality, and better developed and more flexible.
Likewise, doorkeeper objects are much more flexible than external account objects, and it's nice to imagine that we can import from Twootfeed or whatever without needing to build full OAuth for it. I also like less stuff touching auth code, when possible.
Making these separate from external accounts does make it a bit harder to reconcile external users with internal users, but I think that's OK, and that it's generally desirable to show the real source of a piece of content. That is, if I wrote a comment on GitHub but also have a Phabricator account, I think it's good to show "epriestley (GitHub)" (the GitHub user) as the author, not "epriestley" (the Phabricator user). I think this is generally less confusing overall, and we can add more linkage later to make it clearer.
Test Plan:
{F1194104}
{F1194105}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537, T10538
Differential Revision: https://secure.phabricator.com/D15541
Summary: Ref T10538. This probably gets push events where GitHub does not recognize the author wrong, but I don't have any of those yet.
Test Plan: Added and ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15540
Summary: Ref T10537. Add a new content source for Nuance. Prepare for better author attribution.
Test Plan: {F1194038}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15539
Summary: Fixes T10670, for users with exclusively archived badges, user profile should show "no badges" message instead of blank box
Test Plan: Award badge to user with no badges, archive badge, user profile should show "no badges" message under badges.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10670
Differential Revision: https://secure.phabricator.com/D15538
Summary:
Ref T10537.
- Let nuance items render custom curtain panels.
- Add a custom panel linking to the imported task, if one exists.
- Actually extract comments properly.
Test Plan:
Unit tests, plus:
{F1193800}
{F1193801}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15537
Summary: Ref T10677, Awarding/revoking badge should create a feed story on homepage with badge handle recipient handles
Test Plan: Award/revoke badge, open Feed, should see story with badge link and recipient links.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10677
Differential Revision: https://secure.phabricator.com/D15534
Summary: Ref T10677, awarding/revoking a badge should create timeline entries with titles that are more clear (excludes homepage feed stories)
Test Plan: Award/revoke a badge to single or multiple users. See timeline entries that reflect those actions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10677
Differential Revision: https://secure.phabricator.com/D15533
Summary: Cleans up EditEngine, adds new layout to EditEngine and descendents
Test Plan: Test creating a new form, reordering, marking and unmarking defaults. View new forms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15531
Summary: Adds headers, new layout to edit panels on Almanac.
Test Plan: Pull up each edit panel in sandbox, save form.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15527
Summary: Ref T8996, Convert badge recipients from Edges to actual BadgeAward objects
Test Plan: Create badge, award it to recipient. Make sure adding/removing recipients works. (Still need to migrate exisiting recipients to new table and need to create activity feed blurbs)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin
Maniphest Tasks: T8996
Differential Revision: https://secure.phabricator.com/D15014
Summary:
Fixes T10385. Two issues:
- `$show_blame` and `$show_color` were improperly swapped.
- Code to hide these columns got dropped somewhere, probably in my recent-ish rewrite.
Test Plan:
- Showed/hid blame.
- Showed/hid colors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10385
Differential Revision: https://secure.phabricator.com/D15528
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.
Split ContentSource apart so applications can add new content sources.
Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.
- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.
{F1190182}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15521
Summary: Updates the Harbormaster UI to match the new two column everywhere else.
Test Plan: Did best I could, tested builds, plans, steps, buildables. Unable to test lint/unit locally, I need to set that up. Kick the tires for me pls. :3
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15523
Summary: Ref T10537. Show when an object is bridged to something external.
Test Plan: {F1190099}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15520
Summary:
Fixes T10665. See that task for discussion.
Because `$head_map` is not properly re-initialized for each ref we check, pushes which affect multiple branches (say, "A" and "B") can have information bleed from the first branch check to the second branch.
To trigger a problem behavior, you can push one commit which updates an existing branch, plus one commit which creates a new branch. If they process in the right order, the `$head_map` from the updated branch will bleed into the `$head_map` for the new branch and trigger an incorrect head split detection.
Test Plan:
- Pushed a set of changes which updated `branch-a` and created `branch-b`.
- Before change: improper detection of split heads.
- After change: clean push.
- Pushed a set of changes which split the head of `branch-d`.
- Correct detection of split heads.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10665
Differential Revision: https://secure.phabricator.com/D15522
Summary: Ref T10538. This makes us render better human-readable descriptions of more GitHub event types.
Test Plan: Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15516
Summary: Ref T10538. Very sloppy, but starting to sort of work. This sort of gets a piece of framework into a reasonable spot, next couple of diffs are going to be "extract comment text" and "show stuff in the UI" sorts of things.
Test Plan: {F1186726}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15511
Summary: This URI is currently a little whack.
Test Plan:
- With MFA, clicked "Edit Subscription" on a subscription.
- Clicked "Cancel".
- Before: Sent to `/phortune/phortune/edit/...`, a 404.
- After: Properly returned to subscription detail page.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15514
Summary: Moves everything I could find in Phortune to new UI layouts.
Test Plan: Tested every page I could get two, unclear how to test subscriptions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15513
Summary:
Ref T10537. Generally, when users interact with Nuance items we'll dump a command into a queue and apply it in the background. This avoids race conditions with multiple users interacting with an item, which Nuance is more subject to than other applications because it has an import/external component.
The "sync" command doesn't actually do anything yet.
Test Plan: {F1186365}
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15506
Summary: Ref T10538. This extracts and renders URIs for GitHub events so we can link to the original thing on GitHub.
Test Plan: {F1186332}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15505
Summary:
Ref T10537. This allows item types to expose item actions. Eventually these actions might be things like "promote to task", "tweet reply", "ban user forever", etc.
For now, provide a simple action which shows a raw item in a dialog.
Test Plan: {F1185573}
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15504
Summary:
Ref T10537. These are objects which are bound to some external object, like a Maniphest task which is a representation of a GitHub issue.
This doesn't do much yet and may change, but my thinking is:
- I'm putting these on-object instead of on edges because I think we want to actively change the UI for them (e.g., clearly call out that the object is bridged) but don't want every page to need to do extra queries in the common case where zero bridged objects exist anywhere in the system.
- I'm making these one-to-one, more or less: an issue can't be bridged to a bunch of tasks, nor can a bunch of tasks be bridged to a single issue. Pretty sure this makes sense? I can't come up with any reasonable, realistic cases where you want a single GitHub issue to publish to multiple different tasks in Maniphest.
- Technically, one type of each bridgable object could be bridged, but I expect this to never actually occur. Hopefully.
Test Plan: Ran storage upgrade, loaded some pages.
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15502
Summary: This is failing locally for me, set to getViewer and pull up cart.
Test Plan: View cart with a description.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15509
Summary: Ref T9456. This makes everything work, except that CircleCI doesn't fetch tags which are not ancestors of branch heads.
Test Plan: Ran passing builds through CircleCI.
Reviewers: chad
Reviewed By: chad
Subscribers: dpaola2, JustinTulloss
Maniphest Tasks: T9456
Differential Revision: https://secure.phabricator.com/D14288
Summary: Ref T9456. Some rough edges and we can't complete the build yet since I haven't written a webhook, but this mostly seems to be working.
Test Plan:
- Ran this build on some stuff.
- Ran a normal HTTP step build to make sure I didn't break that.
{F880301}
{F880302}
{F880303}
Reviewers: chad
Reviewed By: chad
Subscribers: JustinTulloss, joshma
Maniphest Tasks: T9456
Differential Revision: https://secure.phabricator.com/D14286
Summary: Ref T9456. This is just a convenience type for things like API tokens, to make it harder for users to make mistakes and keep SSH keys out of the dropdown for "choose your API token".
Test Plan: {F879820}
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Maniphest Tasks: T9456
Differential Revision: https://secure.phabricator.com/D14284
Summary: Fixes T10648. This was goofed and always did a meaningless no-op slice -- I mucked it up while doing the disabled project stuff elsewhere.
Test Plan:
- Tagged something with 5 projects.
- Saw the list sliced to 4 (the limit) with "...".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10648
Differential Revision: https://secure.phabricator.com/D15508
Summary:
Fixes T10646. When you load the page or click "New Condition" or "New Action", we try to add a condition and action with some default values.
Currently, the logic just sets everything to `null` or `'default'`. This technically works in Safari, but is less successful in Chrome. (I think Safari prevents you from picking an invalid value.)
Instead of relying on the browser to pick the right value, set the correct value explicitly.
Test Plan:
- Created a new rule in Chrome, Safari.
- Added fields and conditions in Chrome, Safari.
- Edited existing rules in Chrome, Safari.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10646
Differential Revision: https://secure.phabricator.com/D15507
Summary: Updates Spaces to new two column layout
Test Plan: Create a space, edit a space
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15503
Summary: Fixes T10234. This usage is unusual, out of date, and has some bad interactions with engines and custom rules.
Test Plan:
- Added `CustomInlineCodeRule` from P1129 as an extension rule.
- Put a custom `<code> ... </code>` block in a Maniphest task description.
- Saw fatal as described in task; applied change; saw rule work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10234
Differential Revision: https://secure.phabricator.com/D15501
Summary:
D9087 adds a nice typeahead but breaks the existing regex
search by quoting the pattern. Ideally, this change won't break the
typeahead, which as far as I can tell doesn't use the `pattern`
argument.
Test Plan:
Not yet.
RFC as to whether this change makes sense, will fix my local setup and resend if so.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15500
Summary: Ref T7789. Implement proper detection for read-only requests. Previously, we assumed every request was read/write and required lots of permissions, but we don't need "Can Push" permission if you're only cloning/fetching/pulling.
Test Plan:
- Set push policy to "no one".
- Fetched, got clean data out of LFS.
- Tried to push, got useful error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15499
Summary: Fixes T10625.
Test Plan: Faked this locallly and it looked OK, I'll check the mail in production. :3333
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10625
Differential Revision: https://secure.phabricator.com/D15497
Summary:
- Fix spacing on InfoView inside collasped boxes
- Fix spacing on stacked PropertyLists in TwoColumn
- Fix spacing on Readmes on Tablets
- Fix unset variable on importing commits
Test Plan: Review each of the above cases.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15496
Summary: Ref T7789. This isn't the most perfect UI imaginable, but it's similar to what GitHub does and seems reasonable.
Test Plan:
{F1180271}
{F1180272}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15494
Summary: Ref T7789. Make sure these get cleaned up when a repository is destroyed.
Test Plan:
- Created a new repository.
- Pushed some LFS data to it.
- Used `bin/remove destroy` to nuke it.
- Verified the LFS stuff was cleaned up and the underlying files were destroyed (`SELECT * FROM repository_gitlfsref`, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15493
Summary:
Ref T7789. Ref T10604. This implements the `upload` action, which streams file data into Files.
This makes Git LFS actually work, at least roughly.
Test Plan:
- Tracked files in an LFS repository.
- Pushed LFS data (`git lfs track '*.png'; git add something.png; git commit -m ...; git push`).
- Pulled LFS data (`git checkout master^; rm -rf .git/lfs; git checkout master; open something.png`).
- Verified LFS refs show up in the gitlfsref table.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789, T10604
Differential Revision: https://secure.phabricator.com/D15492
Summary: I think I like this better -- but maybe right-aligned?
Test Plan:
{F1180295}
{F1180296}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15495
Summary:
Ref T7789. This implements:
- A new table to store the `<objectHash, filePHID>` relationship between Git LFS files and Phabricator file objects.
- A basic response to `batch` commands, which return actions for a list of files.
Test Plan:
Ran `git lfs push origin master`, got a little further than previously:
```
epriestley@orbital ~/dev/scratch/poemslocal $ git lfs push origin master
Git LFS: (2 of 1 files) 174.24 KB / 87.12 KB
Git LFS operation "upload/b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69" is not supported by this server.
Git LFS operation "upload/b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69" is not supported by this server.
```
With `GIT_TRACE=1`, this shows the batch part of the API going through.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15489
Summary:
adds the `{{PHID....}}` rule. Should mostly be useful in UI code that refers to Objects.
It doesn't add any mention links/transactions.
Test Plan: Comment with this, see email (plain + html) and comment box.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15488
Summary: This updates (all?) of Diffusion/Audit to new UI, included edit and other extra form pages. It's fairly complete but I don't know all the nooks and crannies so to speak to fully verify I didn't mess anything up.
Test Plan: Tested creating new repositories, browsing, searching, auditing. Need more eyes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15487
Summary:
Ref T7789. This builds on top of `git-lfs-authenticate` to detect LFS requests, read LFS tokens, and route them to a handler which can do useful things.
This handler promptly drops them on the floor with an error message.
Test Plan:
Here's a transcript showing the parts working together so far:
- `git-lfs` connects to the server with SSH, and gets told how to connect with HTTP to do uploads.
- `git-lfs` uses HTTP, and authenticates with the tokens properly.
- But the server tells it to go away, and that it doesn't support anything, so the operation ultimately fails.
```
$ GIT_TRACE=1 git lfs push origin master
12:45:56.153913 git.c:558 trace: exec: 'git-lfs' 'push' 'origin' 'master'
12:45:56.154376 run-command.c:335 trace: run_command: 'git-lfs' 'push' 'origin' 'master'
trace git-lfs: Upload refs origin to remote [master]
trace git-lfs: run_command: git rev-list --objects master --not --remotes=origin
trace git-lfs: run_command: git cat-file --batch-check
trace git-lfs: run_command: git cat-file --batch
trace git-lfs: run_command: 'git' config -l
trace git-lfs: tq: starting 3 transfer workers
trace git-lfs: tq: running as batched queue, batch size of 100
trace git-lfs: prepare upload: b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69 lfs/dog1.jpg 1/1
trace git-lfs: tq: sending batch of size 1
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload
trace git-lfs: api: batch 1 files
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects/batch
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\/batch\" is not supported by this server."}
trace git-lfs: HTTP:
trace git-lfs: api: batch not implemented: 404
trace git-lfs: run_command: 'git' config lfs.batch false
trace git-lfs: tq: batch api not implemented, falling back to individual
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69
trace git-lfs: api: uploading (b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69)
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\" is not supported by this server."}
trace git-lfs: HTTP:
trace git-lfs: tq: retrying 1 failed transfers
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69
trace git-lfs: api: uploading (b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69)
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\" is not supported by this server."}
trace git-lfs: HTTP:
Git LFS: (0 of 1 files) 0 B / 87.12 KB
Git LFS operation "objects" is not supported by this server.
Git LFS operation "objects" is not supported by this server.
```
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15485
Summary:
Ref T7789. This implements a (probably) usable "git-lfs-authenticate" on top of the new temporary token infrastructure.
This won't actually do anything yet, since nothing reads the tokens.
Test Plan:
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate'
phabricator-ssh-exec: Expected `git-lfs-authenticate <path> <operation>`, but received too few arguments.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate x'
phabricator-ssh-exec: Unrecognized repository path "x". Expected a path like "/diffusion/X/" or "/diffusion/123/".
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22'
Exception: Expected `git-lfs-authenticate <path> <operation>`, but received too few arguments.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22 y'
Exception: Git LFS operation "y" is not supported by this server.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22 upload'
{"header":{"Authorization":"Basic QGdpdC1sZnM6NmR2bDVreWVsaXNuMmtnNXBtbnZwM3VlaWhubmI1bmI="},"href":"http:\/\/local.phacility.com\/diffusion\/22\/new-callsign-free-repository.git\/info\/lfs"}
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15482
Summary:
Ref T10560. Reverts D15460. See that task for discussion: we dug up some more information to explain the behavior, and this key was just sort of sidestepping an analyze/cardinality estimate issue on the index.
With proper cardinality estimates it shouldn't be used, so just nuke it.
Test Plan: Ran `bin/storage adjust`, saw key drop.
Reviewers: eadler, chad
Reviewed By: chad
Maniphest Tasks: T10560
Differential Revision: https://secure.phabricator.com/D15486
Summary: Fixes T10603. This is the last of the ad-hoc temporary tokens.
Test Plan:
- Used a file token.
- Viewed type in {nav Config > Temporary Tokens}.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15481
Summary: Ref T10603. Swap these over and give them nice UI strings.
Test Plan:
- Refreshed a Twitter OAuth link.
- Unlinked and re-linked a Twitter account.
- Viewed the new type in {nav Config > Temporary Tokens}.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15480
Summary:
Ref T10603. We have a couple of sort of ad-hoc tokens, so start formalizing them. First up is MFA tokens.
Also adds a new config module panel for these.
Test Plan:
- Added MFA.
- Added MFA, intentionally fumbled the input, completed the workflow.
- Removed MFA.
- Viewed tokens, saw MFA sync tokens.
- Viewed new module config panel.
{F1177014}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15479
Summary:
Ref T10603. This makes minor updates to temporary tokens:
- Rename `objectPHID` (which is sometimes used to store some other kind of identifier instead of a PHID) to `tokenResource` (i.e., which resource does this token permit access to?).
- Add a `userPHID` column. For LFS tokens and some other types of tokens, I want to bind the token to both a resource (like a repository) and a user.
- Add a `properties` column. This makes tokens more flexible and supports custom behavior (like scoping LFS tokens even more tightly).
Test Plan:
- Ran `bin/storage upgrade -f`, got a clean upgrade.
- Viewed one-time tokens.
- Revoked one token.
- Revoked all tokens.
- Performed a one-time login.
- Performed a password reset.
- Added an MFA token.
- Removed an MFA token.
- Used a file token to view a file.
- Verified file token was removed after viewing file.
- Linked my account to an OAuth1 account (Twitter).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15478
Summary:
Ref T10603. This converts existing hard-codes to modular constants.
Also removes one small piece of code duplication.
Test Plan:
- Performed one-time logins.
- Performed a password reset.
- Verified temporary tokens were revoked properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15476
Summary:
Ref T10603. For LFS, we need to issue a new type of temporary token.
This makes the temporary token code modular so applications can add new token types without modifying the Auth application.
(I'm moving slowly here because it impacts authentication.)
Test Plan:
- Used `bin/auth recover` to get a one-time token from the CLI.
- Used "Forgot your password?" to get a one-time token from the web UI.
- Followed the web UI token to initiate a password reset, prompting generation of a password token.
- Viewed these tokens in the web UI:
{F1176908}
- Revoked a token.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15475
Summary: Fixes T10591. This was accidentally reverted in 148a50e48b, probably when resolvign a merge/rebase.
Test Plan: Will push to production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10591
Differential Revision: https://secure.phabricator.com/D15474
Summary: Updates Drydock to use two column + curtain layouts.
Test Plan: Tested what I could get to, need @epriestley to run this locally for edge cases.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D15467
Summary:
First pass at converting Differential, I likely have some buggy-poos but thought I'd toss this up now in case very bad bugs present.
To do:
- Need to put status back on Hovercards
- "Diff Detail" probably needs a better design
Test Plan: Looking at lots of diffs, admittedly I dont have harbormaster, etc, running locally. Checked Diffusion for Table of Content changes on small and large commits.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15463
Summary: Fixes T10539. When building mail targets, we fail to preserve whether a recipient was originally "To" or "Cc", and just move everyone to "To".
Test Plan:
Added a comment to a task with a "To" user and a "Cc" user, with `metamta.placeholder-to-recipient` set and `metamta.one-mail-per-recipient` set.
Got mail with me Cc'd as the Cc'd user:
{F1172020}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10539
Differential Revision: https://secure.phabricator.com/D15465
Summary: Fixes T10546. Some day, decades from now, we can revisit this when we iterate on Phrequent. Just don't regress for no real reason in the meantime, since it's easy enough to keep it working in reasonable shape.
Test Plan: {F1169096}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10546
Differential Revision: https://secure.phabricator.com/D15461
Summary:
Ref T10563. This isn't a complete fix, but should make viewing complex inline threads a little more manageable.
This just tries to put stuff in thread order instead of in pure chronological order. We can likely improve the display treatment -- this is a pretty minimal approach, but should improve clarity.
Test Plan:
T10563 has a "before" shot. Here's the "after":
{F1169018}
This makes it a bit easier to follow the conversations.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10563
Differential Revision: https://secure.phabricator.com/D15459
Summary:
Ref T10493.
- Call this action "Unassigned" instead of "Up For Grabs", since the latter implies that it's OK for anyone to grab it, which is a social/context thing that we probably shouldn't assume.
- Show who a task was previously assigned to in the transaction.
The text is a little clunky, yell if you've got a better wording? Or maybe I'll come up with something.
Test Plan: {F1166299}
Reviewers: chad
Reviewed By: chad
Subscribers: cburroughs
Maniphest Tasks: T10493
Differential Revision: https://secure.phabricator.com/D15454
Summary:
Ref T10560. I don't fully understand what MySQL is doing here, but it looks like this key improves the problematic dataset in practice.
(It makes sense that this key helps, I'm just not sure why the two separate keys and the UNION ALL are so bad.)
This key isn't hugely expensive to add, so we can try it and see if there are still issues.
Test Plan: Ran `bin/storage adjust`, saw key added to table. Used `SHOW CREATE TABLE ...` to verify the key exists. Used `EXPLAIN SELECT ...` to make sure MySQL actually uses it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10560
Differential Revision: https://secure.phabricator.com/D15460
Summary: I think this works?
Test Plan:
i am wizard
{F1168808}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15457
Summary: Adds a date with the author name on the Authored By panel in Maniphest. A basic treatment, will see how it feels.
Test Plan: Look at a few tasks
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15456
Summary:
To improve the performance of Herald, we attempt to generate the value for each field (e.g., a task title) only once.
For most field values this is cheap, but for some (like a commit's branches) it can be quite expensive. We only want to pay this cost once, so we cache field values.
However, D12957 accidentally added a check where we bypass the cache and generate the value for every field, before reading the cache. This causes us to generate each field for every rule that uses it, plus one extra time.
Instead, use the cache for this check, too. Also allow the cache to cache `null`, since it can be expensive to generate `null` even though the value isn't too interesting.
The value of this early hit isn't even used (we only care if it throws or not).
Test Plan:
- Wrote a rule like "if any condition matches: branches contain a, branches contain b, branches contain c".
- Put `phlog(new Exception())` in `DiffusionCommitBranchesHeraldField`.
- Before patch, saw `bin/repository reparse --herald <any commit>` compute branches three times.
- After patch, saw only one computation.
- Verified field values in the transcript view
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15451
Summary:
Ref T10538.
This is a tiny fraction of the API. GitHub has 25 primary event types; we currently partially parse 3 of them. GitHub has 17 issue event types; we currently partially parse 12.
Test Plan: Ran `arc unit`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15448
Summary:
Ref T10538. This sets up a Doorkeeper bridge for GitHub issues, and pulls issues from GitHub to create ExternalObject references.
Broadly, does nothing useful.
Test Plan: Put a `var_dump()` in there somewhere and saw it probably do something when running `bin/nuance update --item 44`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15447
Summary:
Ref T10538. The primary GitHub event activity stream does not report minor events (labels, milestones, etc).
GitHub has a second, similar activity stream which does report these events (the "Issues Events API").
Use two separate cursors: one consumes the primary stream; the second consumes the events stream.
One possible issue with this is that we may write events in a different order than they occurred, so GitHub shows "comment, label, close" but we show "comment, close, label" or similar. This is probably OK because the secondary API doesn't seem to have any very important events (e.g., it's probably fine if label changes are out-of-order), but we can conceivably put some buffer stage in between the two if it's an issue.
Test Plan: {F1164894}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10538
Differential Revision: https://secure.phabricator.com/D15446
Summary: Fixes T10545. Converts layout to two column.
Test Plan: Review a few project manage pages, see new layout and flag ability.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10545
Differential Revision: https://secure.phabricator.com/D15450
Summary: Ref T10545, this brings flags back? and converts the layout to two column w/curtain
Test Plan: View a few manage pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10545
Differential Revision: https://secure.phabricator.com/D15449
Summary:
Ref T10537. This adds an update daemon for pulling item data (e.g., figuring out who the author of a GitHub comment is) and routing items (e.g., sending them to a queue or applying them directly to a task).
Also adds `bin/nuance update --item X` for doing this manually for debugging.
And adds item types, for specializing item behavior. Previously, sources completely dictated item behavior, but I think we want something a little more flexible.
Test Plan:
- This still does nothing.
- Ran `bin/nuance update --item 15`.
- Saw an item route to a default queue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15441
Summary: This inverts colors and icons a bit, so they're not as harsh. So instead of a dark green item with white icon, its now light green with a dark green icon. I've also changed all text and comment boxes to be "grey" visually to separate out the UI from converation/actions. Give it a spin and let me know how this feels. I still need to update the comment UI.
Test Plan:
UIExamples, lots of various tasks and diffs.
{F1163837}
{F1163839}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15442
Summary:
Ref T10537. Ref T10538. This polls the GitHub events API and creates Nuance items from the raw data.
It does nothing useful with them.
Test Plan:
- Polled GitHub.
- Saw some items get created.
- X-Poll-Interval seemed to work.
- ETag seemed to work.
- Recognizing when we hit items we've already seen seemed to work.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537, T10538
Differential Revision: https://secure.phabricator.com/D15440
Summary: Cleaner UI, moved visit to be button.
Test Plan: Make a phurl about cats, click on it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15385
Summary:
Ref T10537. More infrastructure:
- Put a `bin/nuance` in place with `bin/nuance import`. This has no useful behavior yet.
- Allow sources to be searched by substring. This supports `bin/nuance import --source whatever` so you don't have to dig up PHIDs.
Test Plan:
- Applied migrations.
- Ran `bin/nuance import --source ...` (no meaningful effect, but works fine).
- Searched for sources by substring in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15436
Summary:
Ref T10537. Some sources (like the future "GitHub Repository" source) need to poll remotes.
- Provide a mechanism for sources to emit import cursors.
- Hook them into the trigger daemon so they'll fire periodically.
- Provide some storage.
This diff does nothing useful or interesting, and is pure infrastructure.
Test Plan:
- Ran `bin/storage upgrade -f`, no adjustment issues.
- Poked around Nuance.
- Ran the trigger daemon, verified it didn't crash and checked for Nuance stuff to do.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15435
Summary: Ref T10537. Converts sources to EditEngine.
Test Plan:
- Created a new source.
- Edited an existing source.
- Submitted a complaint with the complaint form.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15434
Summary: See D15432. There, we can use this test to check if the user //could// reassign the task by using "Edit Form" or the stacked actions, so any dedicated "claim" element is consistent with the other permissions.
Test Plan:
- Added a `var_dump($can_reassign)` after the call.
- Saw `true`.
- Edited the edit form, locked and disabled "Assigned To".
- Saw `false`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15433
Summary: Ref T10537. Update queue editing to use EditEngine.
Test Plan:
- Created a new queue.
- Edited an existing queue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15431
Summary: Ref T10537. Minor updates to simplify and modernize these codepaths.
Test Plan: Searched for queues and sources.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15429
Summary: Ref T10537. Minor cleanup of controllers to be more modern / work better on mobile.
Test Plan: Browsed all queue / source pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15428
Summary: Ref T10093. Changes must be pushed to staging before they can be landed from the web.
Test Plan:
Changes must be pushed to staging before they can be landed from the web.
{F1161909}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10093
Differential Revision: https://secure.phabricator.com/D15427
Summary:
By default, `bin/phd debug` activates `--trace`, which is incredibly verbose.
Instead, be moderately verbose by default, and only include tracing if `--trace` was passed to `bin/phd debug`.
See also D15422.
Test Plan:
- Ran `bin/phd debug task`, got moderate amount of most useful debug output.
- Ran `bin/phd debug task --trace`, got very verbose, detailed low-level debug output.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15423
Summary: Ref T10093. Show better errors when a commit fails because it has already been merged and when a fetch fails because the ref isn't present in the remote.
Test Plan:
{F1160794}
{F1160795}
Reviewers: chad
Reviewed By: chad
Subscribers: michaeljs1990, yelirekim
Maniphest Tasks: T10093
Differential Revision: https://secure.phabricator.com/D15420
Summary: Moves over everything except Maniphest, which has some special behavior.
Test Plan:
- Viewed a badge.
- Viewed a calendar event.
- Viewed a countdown.
- Viewed a Fund initiative.
- Viewed a Herald rule.
- Viewed a macro.
- Viewed an application.
- Viewed an owners package.
- Viewed a credential.
- Viewed a Ponder question.
- Viewed a poll.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15416
Summary: Moves Maniphest over, and allows application to provide ad-hoc panels more easily.
Test Plan: {F1160591}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15417
Summary: Convert Almanac interfaces to Curtain views.
Test Plan: Viewed Services, Bindings, Devices, Namespaces and Networks.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15415
Summary:
This opens up the new action column to have specialized rendering and behavior. Briefly:
- Converted applications (right now, only Paste) render a `CurtainView` to build the column content.
- This view uses new extensions to build panels (projects, subscribers, tokens).
- The panel extension code and rendering can be changed without breaking old stuff.
Minor changes:
- Token awards now load their tokens, for consistency/simplicity.
- Removed the rest of the "fork of" / "forked from" UI in Paste -- I essentially removed these features a while ago, and no one has complained.
Test Plan:
UI is a bit rough, but works, and it's going to get changed now anyway:
{F1160550}
{F1160551}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15414
Summary:
Two minor changes here:
- Replace `get/setUser()` with `get/setViewer()` for consistency with everything else.
- `getViewer()` now throws if no viewer is set. We had a lot of code that either "should" check this but didn't, or did check it in an identical way, duplicating work. In contrast, very little code checks for a viewer but works if one is not present.
Test Plan:
- Grepped for `->user`.
- Attempted to fix all callsites inside `*View` classes.
- Browsed around a bunch of applications, particularly Calendar, Differential and Diffusion, which seemed most heavily affected.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15412
Summary:
Every caller returns `true`. This was added a long time ago for Projects, but projects are no longer subscribable.
I don't anticipate needing this in the future.
Test Plan: Grepped for this method.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15409
Summary: Clean up owners a bit, move to two columns.
Test Plan:
Review a package, edit paths, remove all paths. Archive.
{F1139351}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15388
