Summary: Make Workboard initialization more restrictive.
Test Plan: Log out, see "No Workboard", Log in with permissions, see "New Workboard", Log in with notchad, see "No Workboard".
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7410
Differential Revision: https://secure.phabricator.com/D14306
Summary: Right now logged out users can enable a workboard on a project.
Test Plan: Log out, view a public project, click on Workboard, get not set up dialog. Click Cancel, return to project details.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14304
Summary:
Fixes T9599. When APC/APCu are not available, we fall back to a disk-based cache.
We try to share this cache across webserver processes like APC/APCu would be shared in order to improve performance, but are just kind of guessing how to coordinate it. From T9599, it sounds like we don't always get this right in every configuration.
Since this is complicated and error prone, just stop trying to do this. This cache has bad performance anyway (no production install should be using it), and we have much better APC/APCu setup instructions now than we did when I wrote this. Just using the PID is simpler and more correct.
Test Plan:
- Artificially disabled APC.
- Reloaded the page, saw all the setup stuff run.
- Reloaded the page, saw no setup stuff run (i.e., cache was hit).
- Restarted the webserver.
- Reloaded the page, saw all the setup stuff run.
- Reloaded again, got a cache hit.
I don't really know how to reproduce the exact problem with the parent PID not working, but from T9599 it sounds like this fixed the issue and from my test plan we still appear to get correct behavior in the standard/common case.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9599
Differential Revision: https://secure.phabricator.com/D14302
Summary: Ref T8628. Updates Conduit for handleRequest
Test Plan: Use Conduit, test list, method calls, try a query, post this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8628
Differential Revision: https://secure.phabricator.com/D14265
Summary:
Fixes T9596.
Was unable to navigate to a task in Maniphest.
Test Plan: navigate to that task.
Reviewers: #blessed_reviewers, epriestley, avivey, tycho.tatitscheff
Reviewed By: avivey, tycho.tatitscheff
Subscribers: tycho.tatitscheff, avivey, Korvin
Maniphest Tasks: T9596
Differential Revision: https://secure.phabricator.com/D14300
Summary: Fixes T9592.
Test Plan: Log out ! Navigates to a task. See the add button grey-ed out !
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9592
Differential Revision: https://secure.phabricator.com/D14299
Summary: Ref T182. Nothing fancy, just make these slightly easier to work with.
Test Plan: {F884754}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14295
Summary: If you `!assign cahd` when you meant to `!assign chad`, we'll hit an "Undefined variable: assign_phid" a little further down.
Test Plan: Eyeballed it. See IRC.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14291
Summary:
Fixes T9273. Remarkup has reasonably good fundamentals but the API is a giant pain to work with.
Provide a `PHUIRemarkupView` to make it easier. This object is way simpler to use by default.
It's not currently as powerful, but we can expand the power level later by adding more setters.
Eventually I'd expect to replace `PhabricatorRemarkupInterface` and `PhabricatorMarkupOneOff` with this, but no rush on those.
I converted a few callsites as a sanity check that it works OK.
Test Plan:
- Viewed remarkup in Passphrase.
- Viewed remarkup in Badges.
- Viewed a Conduit method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9273
Differential Revision: https://secure.phabricator.com/D14289
Summary: Ref T182. Make a reasonable attempt to get the commit message, author, and committer data correct.
Test Plan: BEHOLD: rGITTEST810b7f17cd0c909256a45d29a5062fcf417d0489
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14280
Summary: Fixes T9568. We just weren't setting this properly so it would default away from the proper value.
Test Plan:
- Edited a credential in a non-default space, edit form populated properly.
- Changed "Space", introduced an error, saved form, got error with sticky value for "Space" properly.
- Saved form with new space value.
- Created a new credential.
Reviewers: chad
Reviewed By: chad
Subscribers: revi
Maniphest Tasks: T9568
Differential Revision: https://secure.phabricator.com/D14278
Summary:
Ref T9252. This fixes a bug from D14236. D14272 discusses the observable effects of the bug, primarily that the window for racing is widened from ~a few milliseconds to several minutes under our configuration.
This SQL query is missing a `GROUP BY` clause, so all of the resources get counted as having the same status (specifically, the alphabetically earliest status any resource had, I think). For test cases this often gets the right result since the number of resources may be small and they may all have the same status, but in production this isn't true. In particular, the allocator would sometimes see "35 destroyed resources" (or whatever), when the real counts were "32 destroyed resources + 3 pending resources".
Since this allocator behavior is soft/advisory this didn't cause any actual problems, per se (we do expect races here occasionally), it just made the race very very easy to hit. For example, Drydock in production currently has three pending working copy resources. Although we do expect this to be //possible//, getting 4 resources when the configured limit is 1 should be hard (not lightning strike / cosmic radiaion hard, but "happens once a year" hard).
Also exclude destroyed resources since we never care about them.
Test Plan:
Followed the plan from D14272 and restarted two Harbormaster workers at the same time.
After this patch was applied, they no longer created two different resources (we expect it to be possible for this to happen, just very hard).
We should still be able to force this race by putting something like `sleep(10)` right before the query, then `sleep(10)` right after it. That would prevent the allocators from seeing one another (so they would both think there were no other resources) and push us down the pathway where we exceed the soft limit.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14274
Summary:
Ref T9252. This is mostly a fix for an edge case from D14236. Here's the setup:
- There are no resources.
- A request for a new resource arrives.
- We build a new resource.
Now, if we were leasing an existing resource, we'd call `canAcquireLeaseOnResource()` before acquiring a lease on the new resource.
However, for new resources we don't do that: we just acquire a lease immediately. This is wrong, because we now allow and expect some resources to be unleasable when created.
In a more complex workflow, this can also produce the wrong result and leave the lease acquired sub-optimally (and, today, deadlocked).
Make the "can we acquire?" pathway consistent for new and existing resources, so we always do the same set of checks.
Test Plan:
- Started daemons.
- Deleted all working copy resources.
- Ran two working-copy-using build plans at the same time.
- Before this change, one would often [1] acquire a lease on a pending resource which never allocated, then deadlock.
- After this change, the same thing happens except that the lease remains pending and the work completes.
[1] Although the race this implies is allowed (resource pool limits are soft/advisory, and it is expected that we may occasionally run over them), it's MUCH easier to hit right now than I would expect it to be, so I think there's probably at least one more small bug here somewhere. I'll see if I can root it out after this change.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14272
Summary:
Ref T182. If 35 other things are configured completely correctly, make it remotely possible that this button may do something approximating the thing that the user wanted.
This primarily fleshes out the idea that "operations" (like landing, merging or cherry-picking) can have some beahavior, and when we run an operation we do whatever that behavior is instead of just running `git show`.
Broadly, this isn't too terrible because Drydock seems like it actually works properly for the most part (???!?!).
Test Plan: {F876431}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14270
Summary:
Ref T182. This doesn't do anything interesting yet and is mostly scaffolding, but here's roughly the workflow. From previous revision, you can configure "Repository Automation" for a repository:
{F875741}
If it's configured, a new "Land Revision" button shows up:
{F875743}
Once you click it you get a big warning dialog that it won't work, and then this shows up at the top of the revision (completely temporary/placeholder UI, some day a nice progress bar or whatever):
{F875747}
If you're lucky, the operation eventually sort of works:
{F875750}
It only runs `git show` right now, doesn't actually do any writes or anything.
Test Plan:
- Clicked "Land Revision".
- Watched `phd debug task`.
- Saw it log `git show` to output.
- Verified operation success in UI (by fiddling URL, no way to get there normally yet).
Reviewers: chad
Reviewed By: chad
Subscribers: revi
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14266
Summary:
Ref T182. This allows you to assign blueprints that a repository can use to perform working copy operations. Eventually, this will support "merge this" in Differential, etc.
This is just UI for now, with no material effects.
Most of this diff is just taking logic that was in the existing "Blueprints" CustomField and putting it in more general places so Diffusion (which does not use CustomFields) can also access it.
Test Plan:
- Configured repository automation for a repository.
- Removed repository automation for a repository.
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14259
Summary: Fixes T9552. We need to set a questionID and the question object (for policy) when initializing a new Answer.
Test Plan: Write an answer that mentions another user.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9552
Differential Revision: https://secure.phabricator.com/D14263
Summary:
Fixes T9558. The recent changes to validate PHID fields don't work cleanly with this gross hack.
This can probably be unwound now but it will definitely get fixed in T9132 so I may just wait for that.
Test Plan: Edited a custom "users" field in Maniphest. This should only affect Maniphest because it has a weird hack.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9558
Differential Revision: https://secure.phabricator.com/D14264
Summary:
In Mercurial 3.2 the `locate` command was deprecated in favor of `files` command. This change updates the DiffusionLowLevelMercurialPathsQuery command to conditionally use `locate` or `files` based on the version of Mercurial used.
Closes T7375
Test Plan:
My test/develop Phabricator instance is setup to run Mercurial 3.5.1.
The test procedure to verify valid file listings are being returned:
1. I navigated to `http://192.168.0.133/conduit/method/diffusion.querypaths/`
2. I populated the following fields:
- path: `"/"`
- commit: `"d721d5b57fc9ef72e47ff9d4e0c583d74a46590c"`
- callsign: `"HGTEST"`
3. I submitted request and verified that result contained all files in the repository:
```
{
"0": "README",
"1": "alpha/beta/trifle",
"2": "test/Chupacabra.cow",
"3": "test/socket.ks"
}
```
I repeated the above steps after setting up Mercurial 2.6.2, which I installed in the following manner:
1. I downloaded Mercurial 2.6.2 source and run `make local` which will only compile it to work from its own directory (`/opt/mercurial-2.6.2`)
2. I linked `/usr/local/bin/hg -> /opt/mercurial-2.6.2/hg` (there's also a `/usr/bin/hg` which is a link to `/usr/local/bin/hg`)
3. I navigated to my home directory and verify that `hg --version` returns 2.6.2.
4. I restarted phabricator services (probably unnecessary).
With the Multimeter application active
1. I verified that `/usr/local/bin/hg` referred to version 2.6
2. I ran the same conduit call from the conduit application
3. I verified that `http://192.168.0.133/multimeter/?type=2&group=label` incremented values for `bin.hg locate`.
4. I swapped out mercurial versions for 3.5.1
5. I ran the same conduit call from the conduit application
6. I verified that `http://192.168.0.133/multimeter/?type=2&group=label` incremented values for `bin.hg files`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T7375
Differential Revision: https://secure.phabricator.com/D14253
Summary:
Ref T9551. We currently use the same logic for generating project hashtags and Phriction slugs, but should be a little more conservative with project hashtags.
Stop them from generating with stuff that won't parse in a "Reviewers:" field or generally in commments (commas, colons, etc).
Test Plan:
Created a bunch of projects with nonsense in them and saw them generate pretty reasonable hashtags.
{F873456}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9551
Differential Revision: https://secure.phabricator.com/D14261
Summary:
Ref T9551. To set things up:
- Name a project `aa bb`. This will have the tag `aa_bb`.
- Try to visit `/tag/aa%20bb`.
Here's what happens now:
- You get an Aphront redirect error as it tries to add the trailing `/`. Add `phutil_escape_uri()` so that works again.
- Then, you 404, even though this tag is reasonably equivalent to the real project tag and could be redirected. Add a fallback to lookup, resolve, and redirect if we can find a hit for the tag.
This also fixes stuff like `/tag/AA_BB/`.
Test Plan: Visited URIs like `/tag/aa%20bb`, `/tag/aa%20bb/`, `/tag/Aa_bB/`, etc. None of them worked before and now they all do.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9551
Differential Revision: https://secure.phabricator.com/D14260
Summary:
Ref T9519. When acquiring leases on resources:
- Only consider resources created by authorized blueprints.
- Only consider authorized blueprints when creating new resources.
- Fail with a tailored error if no blueprints are allowed.
- Fail with a tailored error if missing authorizations are causing acquisition failure.
One somewhat-substantial issue with this is that it's pretty hard to figure out from the Harbormaster side. Specifically, the Build step UI does not show field value anywhere, so the presence of unapproved blueprints is not communicated. This is much more clear in Drydock. I'll plan to address this in future changes to Harbormaster, since there are other related/similar issues anyway.
Test Plan: {F872527}
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9519
Differential Revision: https://secure.phabricator.com/D14254
Summary: Updates metamta for handleRequest
Test Plan: Unable to test this, but looks safe?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14256
Summary: Updates Harbormaster for handleRequest over processRequest
Test Plan: Went through various Harbormaster areas, buildables, actions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14255
Summary:
Ref T9519. This is like 80% of the way there and doesn't fully work yet, but roughly shows the shape of things to come. Here's how it works:
First, there's a new custom field type for blueprints which works like a normal typeahead but has some extra logic. It's implemented this way to make it easy to add to Blueprints in Drydock and Build Plans in Harbormaster. Here, I've added a "Use Blueprints" field to the "WorkingCopy" blueprint, so you can control which hosts the working copies are permitted to allocate on:
{F869865}
This control has a bit of custom rendering logic. Instead of rendering a normal list of PHIDs, it renders an annotated list with icons:
{F869866}
These icons show whether the blueprint on the other size of the authorization has approved this object. Once you have a green checkmark, you're good to go.
On the blueprint side, things look like this:
{F869867}
This table shows all the objects which have asked for access to this blueprint. In this case it's showing that one object is approved to use the blueprint since I already approved it, but by default new requests come in here as "Authorization Requested" and someone has to go approve them.
You approve them from within the authorization detail screen:
{F869868}
You can use the "Approve" or "Decline" buttons to allow or prevent use of the blueprint.
This doesn't actually do anything yet -- objects don't need to be authorized in order to use blueprints quite yet. That will come in the next diff, I just wanted to get the UI in reasonable shape first.
The authorization also has a second piece of state, which is whether the request from the object is active or inactive. We use this to keep track of the authorization if the blueprint is (maybe temporarily) deleted.
For example, you might have a Build Plan that uses Blueprints A and B. For a couple days, you only want to use A, so you remove B from the "Use Blueprints: ..." field. Later, you can add B back and it will connect to its old authorization again, so you don't need to go re-approve things (and if you're declined, you stay declined instead of being able to request authorization over and over again). This should make working with authorizations a little easier and less labor intensive.
Stuff not in this diff:
- Actually preventing any allocations (next diff).
- Probably should have transactions for approve/decline, at least, at some point, so there's a log of who did approvals and when.
- Maybe should have a more clear/loud error state when no blueprints are approved?
- Should probably restrict the typeahead to specific blueprint types.
Test Plan:
- Added the field.
- Typed some stuff into it.
- Saw the UI update properly.
- Approved an authorization.
- Declined an authorization.
- Saw active authorizations on a blueprint page.
- Didn't see any inactive authroizations there.
- Clicked "View All Authorizations", saw all authorizations.
Reviewers: chad, hach-que
Reviewed By: chad
Maniphest Tasks: T9519
Differential Revision: https://secure.phabricator.com/D14251
Summary:
If Mercurial 3.4+ is used to host repositories in Phabricator, any clients using 3.5+ will receive an exception after the bundle is pushed up. Clients will also fail to update phases for changesets pushed up.
Before directly responding to mercurial clients with all capabilities, this change filters out the 'bundle2' capability so the client negotiates using a legacy bundle wire format instead.
Test Plan:
Server: Mercurial 3.5
Client: Mercurial 3.4
Test with both HTTP and SSH protocols:
1. Create a local commit on client
2. Push commit to server
3. Verify the client emits something like:
```
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
```
Closes T9450
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9450
Differential Revision: https://secure.phabricator.com/D14241
Summary:
Ref T9123. Two major Harbormaster-related UI changes in Diffusion:
- Tags table now shows tag build status.
- Branches table now shows branch build status.
Then some minor consistency / qualtiy of life changes:
- Picked a nicer looking "history" icon?
- Branches table now uses the same "history" icon as other tables.
- Tags table now has a "history" link.
- Browse table now has a "history" link.
- Dates now use more consistent formatting.
- Column order is now more consistent.
- Use of style is now more consistent.
Test Plan:
{F865056}
{F865057}
{F865058}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9123
Differential Revision: https://secure.phabricator.com/D14242
Summary: Ref T9252. Move these into a new "Drydock" group.
Test Plan: Clicked "Add Build Step", saw Drydock steps in a Drydock group.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14237
Summary:
Ref T9252. I think there's a more complex version of this problem discussed elsewhere, but here's what we hit today:
- 5 commits land at the same time and trigger 5 builds.
- All of them go to acquire a working copy.
- Working copies have a limit of 1 right now, so 1 of them gets the lease on it.
- The other 4 all trigger allocation of //new// working copies. So now we have: 1 active, leased working copy and 4 pending, leased working copies.
- The 4 pending working copies will never activate without manual intervention, so these 4 builds are stuck forever.
To fix this, prevent WorkingCopies from giving out leases until they activate. So now the leases won't acquire until we know the working copy is good, which solves the first problem.
However, this creates a secondary problem:
- As above, all 5 go to acquire a working copy.
- One gets it.
- The other 4 trigger allocations, but no longer acquire leases. This is an improvement.
- Every time the leases update, they trigger another allocation, but never acquire. They trigger, say, a few thousand allocations.
- Eventually the first build finishes up and the second lease acquires the working copy. After some time, all of the builds finish.
- However, they generated an unboundedly large number of pending working copy resources during this time.
This is technically "okay-ish", in that it did work correctly, it just generated a gigantic mess as a side effect.
To solve this, at least for now, provide a mechanism to impose allocation rate limits and put a cap on the number of allocating resources of a given type. As hard-coded, this the greater of "1" or "25% of the active resources in the pool".
So if there are 40 working copies active, we'll start allocating up to 10 more and then cut new allocations off until those allocations get sorted out. This prevents us from getting runaway queues of limitless size.
This also imposes a total active working copy resource limit of 1, which incidentally also fixes the problem, although I expect to raise this soon.
These mechanisms will need refinement, but the basic idea is:
- Resources which aren't sure if they can actually activate should wait until they do activate before allowing leases to acquire them. I'm fairly confident this rule is a reasonable one.
- Then we limit how many bookkeeping side effects Drydock can generate once it starts encountering limits.
Broadly, some amount of mess is inevitable because Drydock is allowed to try things that might not work. In an extreme case we could prevent this mess by setting all these limits at "1" forever, which would degrade Drydock to effectively be a synchronous, blocking queue.
The idea here is to put some amount of slack in the system (more than zero, but less than infinity) so we get the performance benefits of having a parallel, asyncronous system without a finite, manageable amount of mess.
Numbers larger than 0 but less than infinity are pretty tricky, but I think rules like "X% of active resources" seem fairly reasonable, at least for resources like working copies.
Test Plan:
Ran something like this:
```
for i in `seq 1 5`; do sh -c '(./bin/harbormaster build --plan 10 rX... &) &'; done;
```
Saw 5 plans launch, acquire leases, proceed in an orderly fashion, and eventually finish successfully.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14236
Summary:
Ref T9513. I checked this briefly but didn't do a very thorough job of it.
- Don't try to query merges for Subversion, since it doesn't support them.
- Fix up "existsquery" to work properly (and efficiently) for both hosted and imported repositories.
- Fix up "parentsquery" to have similar behavior on invalid commits to other VCSes (throw an exception).
Test Plan:
- No more merges warning on SVN.
- Hosted SVN gets the right exists result now.
- Visiting "r23980283789287" now 404's instead of "not parsed yet".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9513
Differential Revision: https://secure.phabricator.com/D14239
Summary:
Ref T9252. Currently, Harbormaster and Drydock work like this in some cases:
# Queue a lease for activation.
# Then, a little later, save the lease PHID somewhere.
# When the target/resource is destroyed, destroy the lease.
However, something can happen between (1) and (2). In Drydock this window is very short and the "something" would have to be a lighting strike or something similar, but in Harbormaster we wait until the resource activates to do (2) so the window can be many minutes long. In particular, a user can use "Abort Build" during those many minutes.
If they do, the target is destroyed but it doesn't yet have a record of the artifact, so the artifact isn't cleaned up.
Make these things work like this instead:
# Create a new lease and pre-generate a PHID for it.
# Save that PHID as something that needs to be cleaned up.
# Queue the lease for activation.
# When the target/resource is destroyed, destroy the lease if it exists.
This makes sure there's no step in the process where we might lose track of a lease/resource.
Also, clean up and standardize some other stuff I hit.
Test Plan:
- Stopped daemons.
- Restarted a build in Harbormaster.
- Stepped through the build one stage at a time using `bin/worker execute ...`.
- After the lease was queued, but before it activated, aborted the build.
- Processed the Harbormaster side of things only.
- Saw the lease get destroyed properly.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14234
Summary:
Ref T9028. When users push a commit, then later delete it (e.g., by deleting the branch which contained it) we currently explode when trying to view it.
Instead, degrade gradually if some information is not available.
Test Plan:
- Looked at valid commits with parents, refs, branches and merges.
- Looked at invalid commits.
- Looked at a previously valid, now-deleted + gc'd commit:
{F859273}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D14227
Summary: Ref T9478. This should probably be configurable eventually, but for now treat any 200-block status as success. Also show the result code.
Test Plan:
- Hit a bad URI, saw "HTTP 503" + failure.
- Hit a good URI, saw "HTTP 200" + success.
Reviewers: chad, hach-que
Reviewed By: chad, hach-que
Maniphest Tasks: T9478
Differential Revision: https://secure.phabricator.com/D14226
Summary:
Fixes T9494. This:
- Removes all the random GC.x.y.z config.
- Puts it all in one place that's locked and which you use `bin/garbage set-policy ...` to adjust.
- Makes every TTL-based GC configurable.
- Simplifies the code in the actual GCs.
Test Plan:
- Ran `bin/garbage collect` to collect some garbage, until it stopped collecting.
- Ran `bin/garbage set-policy ...` to shorten policy. Saw change in web UI. Ran `bin/garbage collect` again and saw it collect more garbage.
- Set policy to indefinite and saw it not collect garabge.
- Set policy to default and saw it reflected in web UI / `collect`.
- Ran `bin/phd debug trigger` and saw all GCs fire with reasonable looking queries.
- Read new docs.
{F857928}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9494
Differential Revision: https://secure.phabricator.com/D14219
Summary: Ref T9252. This variable was always wrong but we fell back to just resetting to `HEAD` before. Use the correct variable name.
Test Plan: Verified variable name.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14224
Summary: Ref T9352. See D13635. Build targets can have variables already, but let builds have them too. This mostly enables future use cases (sub-builds, more sophisticated build triggers).
Test Plan: With a custom Herald rule + action like the one in T9352, updated a revision and saw it generate multiple builds with varying parameters.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9352
Differential Revision: https://secure.phabricator.com/D14222
Summary: Ref T9494. Depends on D14216. Remove 10 copies of this code.
Test Plan: Ran `arc unit --everything`, browsed Config > Modules, clicked around Herald / etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9494
Differential Revision: https://secure.phabricator.com/D14217
Summary:
Ref T9252. This primarily allows Harbormaster to request (and Drydock to fulfill) working copies with a patch from a staging area. Doing this means we can do builds on in-review changes from `arc diff`.
This is a little cobbled-together but should basically work.
Also fix some other issues:
- Yielded, awakend workers are fine to update but could complain.
- We can't log slot lock failures to resources if we don't end up saving them.
- Killing the transaction would wipe out the log.
- Fix some TODOs, etc.
Test Plan: Ran Harbormaster builds on a local revision.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14214
Summary:
Ref T9252. Long ago you sometimes manually created resources, so they had human-enterable names. However, users never make resources manually any more, so this field isn't really useful any more.
In particular, it means we write a lot of untranslatable strings like "Working Copy" to the database in the default locale. Instead, do the call at runtime so resource names are translatable.
Also clean up a few minor things I hit while kicking the tires here.
It's possible we might eventually want to introduce a human-choosable label so you can rename your favorite resources and this would just be a default name. I don't really have much of a use case for that yet, though, and I'm not sure there will ever be one.
Test Plan:
- Restarted a Harbormaster build, got a clean build.
- Released all leases/resources, restarted build, got a clean build with proper resource names.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14213
Summary:
Ref T9252. See companion change in D14211. This does the same thing for leases.
Particularly, most of the TODOs about error handling can just be removed because they'll do the right things by default now.
This and D14211 also move slot lock release to after resource destruction. This feels cleaner than trying to release early at release/break.
Test Plan: Restarted a Harbormaster build, got a clean build result. This needs more vetting but I'll clean up any issues as I hit them.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14212
Summary:
Ref T9252. Currently, error handling behavior isn't great and a lot of errors aren't dealt with properly. Try to improve this by making default behaviors better:
- Yields, slot lock exceptions, and aggregate or proxy exceptions containing an excpetion of these types turn into yields.
- All other exceptions are considered permanent failures. They break the resource and
This feels a little bit "magical" but I want to try to get the default behaviors to align reasonably well with expectations so that blueprints mostly don't need to have a ton of error handling. This will probably need at least some refinement down the road, but it's a reasonable rule for all exception/error conditions we currently have.
Test Plan: I did a clean build, but haven't vetted this super thoroughly. Next diff will do the same thing to leases, then I'll work on stabilizing this code better.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14211
Summary: Ref T9252. Add a bit more logging and improve some behaviors.
Test Plan: Restarted a build, got a good result.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14210
Summary:
Ref T9252. This is the same as D14201, but for lease stuff instead of resource stuff.
This one is a little heavier but still feels pretty reasonable to me at the end of the day (worker is <1K lines and has a ton of comment stuff).
Also fixes a few random bugs I hit in the task queue.
Test Plan:
- Restarted some Harbormaster builds, saw them go through cleanly.
- Released pre-activation resources/leases.
- Probably still kinda buggy but I'll iron the details out over time.
Logs are starting to look somewhat plausible:
{F855747}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14202
Summary:
Ref T9252. Currently, Drydock Leases and Resources have several workers:
- Resources: ResourceWorker, ResourceUpdateWorker, ResourceDestroyWorker
- Leases: AllocatorWorker, LeaseWorker, LeaseUpdateWorker, LeaseDestroyWorker
This is kind of a lot of stuff, and it creates some problems.
In particular, leases and resources in early lifecycle phases (pending/allocating/acquiring) can't process commands yet, because that code is only in the "UpdateWorker" classes. If they aren't able to move forward because of a bug, they also can't be released because they can't react to the release command until later in their lifecycle. This creates a soft hang where I have to go wipe stuff out of the database since there's no other way to get rid of it.
Instead, I want leases and resources to be releasable from any (pre-release / pre-destroy) phase of their lifecycle. To support this, all the workers before the "UpdateWorker" need to be able to process commands.
A second, similar issue is that logging and exception handling behaviors are underpowered right now. Elsewhere I began improving this, but ran into issues where all of the workers needed to share very similar exception code. Merging them will make this future change simpler.
This diff fixes this for resources: it merges the Worker, UpdateWorker and DestroyWorker logic into UpdateWorker and throws away the other two workers.
Test Plan: Nothing substantive yet, see next diff. I'll do the same thing for Leases, then test both more thoroughly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14201
Summary: Ref T9252. Make log types modular so they can be translated and have complicated rendering logic if necessary (currently, none have this).
Test Plan: {F855330}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14198
Summary:
Ref T9252. Drydock logs are almost exclusively useful as a diagnostic tool for debugging immediate problems, so GC them fairly aggressively.
(I expect 99% of the usefulness of these logs to be within the first 24 hours, basically "why isn't my thing working". I can't really think of any cases where having old logs would be useful.)
Test Plan:
- Ran GC, saw it hit the log table (with no effect).
- Changed TTL from 30 days to 30 seconds, ran GC, saw it wipe recent logs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14197
Summary:
Ref T9252. Several general changes here:
- Moves logs to use PHIDs instead of IDs. This generally improves flexibility (for example, it's a lot easier to render handles).
- Adds `blueprintPHID` to logs. Although you can usually figure this out from the leasePHID or resourcePHID, it lets us query relevant logs on Blueprint views.
- Instead of making logs a top-level object, make them strictly a sub-object of Blueprints, Resources and Leases. So you go Drydock > Lease > Logs, etc., to get to logs.
- I might restore the "everything" view eventually, but it doesn't interact well with policies and I'm not sure it's very useful. A policy-violating `bin/drydock log` might be cleaner.
- Policy-wise, we always show you that logs exist, we just don't show you log content if it's about something you can't see. This is similar to seeing restricted handles in other applications.
- Instead of just having a message, give logs "type" + "data". This will let logs be more structured and translatable. This is similar to recent changes to Herald which seem to have worked well.
Test Plan:
Added some placeholder log writes, viewed those logs in the UI.
{F855199}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14196
Summary: Ref T9252. We're currently resetting to the local branch, but should be resetting to the origin branch.
Test Plan: Restarted a build, had it run `git show`, saw proper HEAD.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14194
Summary: Ref T9271, maybe fixes it. This restores feed publishing for answers (broken in D13951) and sends the author of the question an email for new answers. Also, unsure how to pull all question subsribers to the answer email, or is it automagical?
Test Plan: notchad asks a question, chad answers, log into notchad and see that mail was delivered, see feed story.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: revi, Korvin
Maniphest Tasks: T9271
Differential Revision: https://secure.phabricator.com/D14171
Summary: This is to make it more obvious how to ignore a folder that may be at the root, and it resolves https://secure.phabricator.com/T8894
Test Plan: Just a documentation change
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, nornagon
Differential Revision: https://secure.phabricator.com/D14193
Summary: Replaces D13687. Leases track an owner but don't currently show it.
Test Plan:
Looked at a lease.
{F851223}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14191
Summary: Fixes T9481. If the viewer does not have access to Differential (for example, because it is not installed), hide the "Revision" column in Diffusion.
Test Plan:
- Viewed history, saw "Revision" column.
- Uninstalled Differential, reloaded, no "Revision" column.
Reviewers: chad
Reviewed By: chad
Subscribers: revi
Maniphest Tasks: T9481
Differential Revision: https://secure.phabricator.com/D14188
Summary:
Ref T9482. This button goes to the wrong place and this table conditionally hides itself so I missed it.
Instead:
- Always show the table, with an empty string if there are no relevant commits.
- Link to a working UI.
Test Plan: Saw table. Clicked button.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9482
Differential Revision: https://secure.phabricator.com/D14189
Summary:
Fixes T9479. Currently, `@aaaaaaaa` may try to match as a commit hash, and `@C123456` may try to match as a Countdown reference. These should only match as user mentions.
Prevent object mention rules from matching after `@`. We already prevent them after `-` and `#`, and already prevented the username rule after `@` (i.e., preventing `@@user`).
Test Plan:
Created some "interesting" users locally and `@mentioned` them:
{F850779}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9479
Differential Revision: https://secure.phabricator.com/D14186
Summary:
Ref T9123. After recent changes, viewing a live build log from the web UI throws a CSRF exception.
Check `start` ("this object is an active log"), not `live` ("this log is open somewhere").
Test Plan: Will push / etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9123
Differential Revision: https://secure.phabricator.com/D14185
Summary:
Ref T9123. To run upstream builds in Harbormaster/Drydock, we need to be able to check out `libphutil`, `arcanist` and `phabricator` next to one another.
This adds an "Also Clone: ..." field to Harbormaster working copy build steps so I can type all three repos into it and get a proper clone with everything we need.
This is somewhat upstream-centric and a bit narrow, but I don't think it's totally unreasonable, and most of the underlying stuff is relatively general.
This adds some more typechecking and improves data/type handling for custom fields, too. In particular, it prevents users from entering an invalid/restricted value in a field (for example, you can't "Also Clone" a repository you don't have permission to see).
Test Plan: Restarted build, got a Drydock resource with multiple repositories in it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9123
Differential Revision: https://secure.phabricator.com/D14183
Summary:
Fixes T8728. As far as I can tell, I simply got this wrong in D11826. This is not the proper name for the preference.
That change primarily focused on the "spammy junk during import" issue, and the code did get the importing flag right. It looks like my testing in D11827 focused on "during import" and just missed this case.
Test Plan: Grepped for `disable-herald`. Grepped for `herald-disable`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8728
Differential Revision: https://secure.phabricator.com/D14181
Summary:
Ref T9252. For building Phabricator itself, we need to have `libphutil/`, `arcanist/` and `phabricator/` next to one another on disk.
Expand the Drydock WorkingCopy resource so that it can have multiple repositories if the caller needs them.
I'm not sure if I'm going to put the actual config for this in Harbormaster or Drydock yet, but the WorkingCopy resource itself should work the same way in either case.
Test Plan: Restarted a Harbormaster build which leases a working copy, saw it build as expected.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14180
Summary:
Ref T9252. Currently, Harbormaster does this when trying to acquire a working copy:
- Ask for a working copy.
- Yield for 15 seconds.
- Check if we have a working copy yet.
That's OK, but Drydock takes ~1s to acquire a working copy lease if a resource is already available, so we end up doing this:
- T+0: Ask for a working copy.
- T+0: Yield for 15 seconds.
- T+1: Working copy lease activates.
- T+15: Working copy lease is used.
- T+16: Build finishes.
So we end up spending about 2 seconds doing work and 14 seconds sleeping.
One way to fix this would be to fiddle with the yield duration, so we yield for 1, 2, 4, ... seconds or something. This probably isn't a bad idea for longer leases (i.e., wait for 15, 30, 45 ... seconds or similar) but it implies a lot of churn for short leases.
Instead, let tasks "awaken" other tasks when they complete. The "awaken" operation means: if a task is in a yielded state (no failures, no owner, explicitly yielded, future expires time), pretend it only yielded until right now instead of whenever it really yielded to.
Basically, this rewrites history so that even though Harbormaster did a `yield(15)`, we pretend it did a `yield(4)` after we activate the lease if lease activation took 4 seconds.
If this misses, it's fine: we fall back to the normal yield behavior and things move forward normally a few seconds later.
If it hits, we get a more nimble process pretty cleanly.
Test Plan:
- Restarted a build plan (lease working copy + run `ls`) with this patch no-op'd, took about 16 seconds.
- Restarted a build plan with this patch active, took about 1 second.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14178
Summary: Ref T9252. Show the user when a resource or lease has a pending release command in queue.
Test Plan: Released a resource and lease from the web UI. In both cases, saw a "releasing" tag and the action disable.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14177
Summary:
Fixes T6569. This implements an expiry mechanism for Drydock resources which parallels the mechanism for leases.
A few things are missing that we'll probably need in the future:
- An "EXPIRES" command to update the expiration time. This would let resources be permanent while leased, then expire after, say, 24 hours without any leases.
- A callback like `shouldActuallyExpireRightNow()` for resources and leases that lets them decide not to expire at the last second.
- A callback like `didAcquireLease()` for resource blueprints, to parallel `didReleaseLease()`, letting them clear or extend their timer.
However, this stuff would mostly just let us tune behaviors, not really open up new capabilities.
Test Plan: Changed host resources to expire after 60 seconds, leased one, saw it vanish 60 seconds later.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T6569
Differential Revision: https://secure.phabricator.com/D14176
Summary: Fixes T9474
Test Plan: Make a poll, log out, still see it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9474
Differential Revision: https://secure.phabricator.com/D14179
Summary: Fixes T5991. If //all requested documents// failed to index, consider this a catastrophic failure and exit with an error code.
Test Plan:
- Ran `bin/search index --type TASK`, observed successful exit despite a small number of un-indexable documents.
- Ran `bin/search index PHID-TASK-xxx` for an invalid task, observed exception on exit after complete failure.
- Ran normal indexing through daemons.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5991
Differential Revision: https://secure.phabricator.com/D14174
Summary: Ref T9272. This doesn't fix anything, just a little cleanup while I was looking at it.
Test Plan: Clicked "Show Details" on a couple description changes, got the same effect for less code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9272
Differential Revision: https://secure.phabricator.com/D14168
Summary: Fixes T9312. This is a bit fluff, but does simplify the view controller slightly and seems reasonable/useful in general.
Test Plan: Clicked "View Raw File" on a paste, got redirected to the raw file via a stable URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9312
Differential Revision: https://secure.phabricator.com/D14167
Summary: Ref T9252. This mostly cleans up future and log handling, and edges us closer to being able to do useful work with Harbormaster / Drydock.
Test Plan:
- Added a "Run `ls -alh`" step to my trivial build plan.
- Ran it a bunch of times.
- Worked great.
- Also did an HTTP plan.
{F835227}
{F835228}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14161
Summary: Fixes T9434. I'm not sure exactly what changed behavior here, but we need a `needAuditRequests()`.
Test Plan: Ran a query which hit the exception (empty query was good enough, locally), then applied this patch; saw exception go away.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9434
Differential Revision: https://secure.phabricator.com/D14162
Summary:
Fixes T9427. Currently, replies to audits/commits go to "Cxxx", but so do replies to countdowns.
There is non real non-disruptive approach available here and this seems least-bad.
Test Plan:
- Made a comment on a commit.
- Fished the reply-to address out of `bin/mail list-oubound` + `bin/mail show-outbound` (it was now "COMMIT...").
- Sent mail to that address.
- Grabbed the raw message and wrote it to `mail.txt`.
- Ran `cat mail.txt | ./scripts/mail/mail_handler.php --process-duplicates`.
- Used `bin/mail list-inbound` + `bin/mail show-inbound` to verify receipt.
- Saw comment appear on audit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9427
Differential Revision: https://secure.phabricator.com/D14163
Summary:
Fixes T9328. There's no way to hit these error states by clicking things in the UI that I could find, but if you mash stuff into your URL bar or "Inspect Element..." and then edit the form to be full of garbage you can hit them.
Make them a little more informative and don't send them to the log, since these are pretty much just fancy 404s.
Test Plan: Bashed my fist on the URL bar to hit all these messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9328
Differential Revision: https://secure.phabricator.com/D14164
Summary:
Fixes T9145. We currently restrict lint codes to 32 bytes, but PHPCS generates codes like "PHPCS.E.PEAR.Comments.Messages.Line.TooLong".
These codes seem reasonable as codes, and we don't currently have any key-length problems or other technical concerns with simply raising the size of this column.
Test Plan: Ran `bin/storage upgrade` to pick up adjustments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9145
Differential Revision: https://secure.phabricator.com/D14166
Summary: Ref T9252. This is still crude in a few ways but basically works, at least for commits.
Test Plan:
- Made a build plan with just this build step.
- Ran `bin/harbormaster build --plan 10 ...` on a commit.
- It actually built a working copy, leased it, took no action, and released the lease. MAGIC~~~
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14160
Summary: Ref T9252. This is the same as D14157, just for Resources and their leases.
Test Plan: Viewed a resource, saw only active leases, clicked "View All Leases", queried, clicked around, used crumbs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14158
Summary:
Ref T9252. Currently, Drydock blueprint pages:
- show all resources, even if there are a million;
- show resources in all states, although destroyed resources are usually uninteresting;
- have some junky `$pager` code.
Instead, show the few most recent active resources and link to a filtered resource view in ApplicationSearch.
Test Plan:
- Viewed some blueprints.
- Clicked "View All Resources".
- Saw all resources.
- Used query / crumbs / etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14157
Summary: Ref T9252. If you have a blueprint and you do not like that blueprint very much, you can disable it.
Test Plan: Disabled / enabled some blueprints.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14156
Summary:
Ref T9252. Move these to the more modern stuff to pick up ordering and interface support for free.
Also work around the blueprint / custom field integration a little more gracefully.
Test Plan: Searched for blueprints, resources and leases.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14155
Summary: Ref T9252. Resources always have a corresponding blueprint, and it makes sense to use the same policies for both.
Test Plan: Viewed resources in web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14154
Summary:
Ref T9252. Drydock currently uses integer statuses, but there's no reason for this (they don't need to be ordered) and it makes debugging them, working with them, future APIs, etc., more cumbersome.
Switch to string instead.
Also rename `STATUS_OPEN` to `STATUS_ACTIVE` and `STATUS_CLOSED` to `STATUS_RELEASED` for consistency. This makes resources and leases have more similar states, and gives resource states more accurate names.
Test Plan: Browsed web UI, grepped for changed constants, applied patch, inspected database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14153
Summary:
Ref T9252. Leases currently have a `resourceID`, but this is a bit nonstandard and generally less flexible than giving them a `resourcePHID`.
In particular, a `resourcePHID` is easier to use when rendering interfaces, since you can get handles out of a PHID.
Add a PHID column, copy over all the PHIDs that correspond to existing IDs, then drop the ID column.
Test Plan:
- Browsed web UIs.
- Inspected database during/after migration.
- Grepped for `resourceID`.
- Allocated a new lease with `bin/drydock lease`.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14151
Summary: Ref T9252. This is now more consistent (same as the equivalent Resource state) and accurate (leases can end up in this state a bunch of ways, including by expiring).
Test Plan: `grep`, browsed around web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14150
Summary: Ref T6569. If a lease is activated with an expiration date, schedule a task to try to clean it up after that time.
Test Plan:
- Used `bin/drydock lease ... --until ...` to activate a lease in the near future.
- Waited for a bit.
- Saw it expire and get destroyed at the scheduled time.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T6569
Differential Revision: https://secure.phabricator.com/D14148
Summary:
Ref T9252. This simplifies some Drydock code.
Most of this code relates to the old notion of Drydock being able to enumerate all the tasks it needs to complete in order to acquire a lease. The code has stepped back from this, since it's unnecessary, the queue is more powerful than it used to be, and it would be a lot of work to keep track of.
The ~only thing that should ever wait for leases in modern code is `bin/drydock lease`, and it's fine for it to just sit there sleeping, so this just does that.
This reduces the granularity of logging, but I'll address that separately in future logging-focused changes.
Test Plan: Used `bin/drydock lease` to acquire a lease, saw it acquire cleanly.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14147
Summary: Fixes T9388, lays in basic ApplicationSearch.
Test Plan: Build a dashboard with Posts and Blogs, click on search icon, get sent to correct page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9388
Differential Revision: https://secure.phabricator.com/D14146
Summary: Ref T9252. Some leases or resources may need to remove data, tear down VMs, etc., during cleanup. After they are released, queue a "destroy" phase for performing teardown.
Test Plan:
- Used `bin/drydock lease ...` to create a working copy lease.
- Used `bin/drydock release-lease` and `bin/drydock release-resource` to release the lease and then the working copy and host.
- Saw working copy and host get destroyed and cleaned up properly.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T6569, T9252
Differential Revision: https://secure.phabricator.com/D14144
Summary:
Ref T9252. Broadly, Drydock currently races on releasing objects from the "active" state. To reproduce this:
- Scatter some sleep()s pretty much anywhere in the release code.
- Release several times from web UI or CLI in quick succession.
Resources or leases will execute some release code twice or otherwise do inconsistent things.
(I didn't chase down a detailed reproduction scenario for this since inspection of the code makes it clear that there are no meaningful locks or mechanisms preventing this.)
Instead, add a Harbormaster-style command queue to resources and leases. When something wants to do a release, it adds a command to the queue and schedules a worker. The workers acquire a lock, then try to consume commands from the queue.
This guarantees that only one process is responsible for writes to active resource/leases.
This is the last major step to giving resources and leases a single writer during all states:
- Resource, Unsaved: AllocatorWorker
- Resource, Pending: ResourceWorker (Possible rename to "Allocated?")
- Resource, Open: This diff, ResourceUpdateWorker. (Likely rename to "Active").
- Resource, Closed/Broken: Future destruction worker. (Likely rename to "Released" / "Broken"; maybe remove "Broken").
- Resource, Destroyed: No writes.
- Lease, Unsaved: Whatever wants the lease.
- Lease, Pending: AllocatorWorker
- Lease, Acquired: LeaseWorker
- Lease, Active: This diff, LeaseUpdateWorker.
- Lease, Released/Broken: Future destruction worker (Maybe remove "Broken"?)
- Lease, Expired: No writes. (Likely rename to "Destroyed").
In most phases, we can already guarantee that there is a single writer without doing any extra work. This is more complicated in the "Active" case because the release buttons on the web UI, the release tools on the CLI, the lease requestor itself, the garbage collector, and any other release process cleaning up related objects may try to effect a release. All of these could race one another (and, in many cases, race other processes from other phases because all of these get to act immediately) as this code is currently written. Using a queue here lets us make sure there's only a single writer in this phase.
One thing which is notable is that whatever acquires a lease **can not write to it**! It is never the writer once it queues the lease for activation. It can not write to any resources, either. And, likewise, Blueprints can not write to resources while acquiring or releasing leases.
We may need to provide a mechinism so that blueprints and/or resource/lease holders get to attach some storage to resources/leases for bookkeeping. For example, a blueprint might need to keep some kind of cache on a resource to help it manage state. But I think we can cross that bridge when we come to it, and nothing else would need to write to this storage so it's technically straightforward to introduce such a mechanism if we need one.
Test Plan:
- Viewed buttons in web UI, checked enabled/disabled states.
- Clicked the buttons.
- Saw commands show up in the command queue.
- Saw some daemon stuff get scheduled.
- Ran CLI tools, saw commands get consumed and resources/leases release.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14143
Summary:
Ref T9458. This is basically the same as D13319, but the "Tags" field didn't get covered in that change.
Specifically, the issue is:
- We try to generate mail to a disabled user (later, we'll drop it without delivering it, but that filtering doesn't happen yet).
- The disabled user doesn't have permission to use Conduit (or any other Conduit-related problem occurs).
- We fail here, then retry generating the mail again later.
Instead, just degrade to not building the field and showing what went wrong.
Test Plan:
- Pushed some commits, saw mail generate.
- Added a fake exception to the field, saw the mail generate with an error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9458
Differential Revision: https://secure.phabricator.com/D14142
Summary: This UI should have a Collapsed PHUIObjectBoxView.
Test Plan: review a file in sandbox
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14139
Summary:
Ref T9253. For resources and leases that need to do something which takes a lot of time or requires waiting, allow them to allocate/acquire first and then activate later.
When we allocate a resource or acquire a lease, the blueprint can either activate it immediately (if all the work can happen quickly/inline) or activate it later. If the blueprint activates it later, we queue a worker to handle activating it.
Rebuild the "working copy" blueprint to work with this model: it allocates/acquires and activates in a separate step, once it is able to acquire a host.
Test Plan: With some power of imagination, brought up a bunch of working copies with `bin/drydock lease --type working-copy ...`
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14127
Summary: Ref T9253. Provide a meaningful command interface for Almanac hosts.
Test Plan:
Configued and leased a real host (`sbuild001.phacility.net`) and ran a command on it.
```
$ ./bin/drydock command --lease 90 -- ls /
bin
boot
core
dev
etc
home
initrd.img
lib
lib64
lost+found
media
mnt
opt
proc
root
run
sbin
srv
sys
tmp
usr
var
vmlinuz
```
Reviewers: chad, hach-que
Reviewed By: chad, hach-que
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14126
Summary: Ref T9253. We had some un-modern use of UI elements, clean that up. Add a tab for showing slot locks so you don't have to fish around in the database.
Test Plan: Looked at blueprints, resources and leases. Looked at slot locks.
Reviewers: chad, hach-que
Reviewed By: chad, hach-que
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14119
Summary:
See discussion in D10304. There's a lot of context there, but the general idea is:
- Blueprints should manage locks in a granular way during the actual allocation/acquisition phase.
- Optimistic "slot locks" might a pretty good primitive to make that easy to implement and reason about in most cases.
The way these locks work is that you just pick some name for the lock (like the PHID of a resource) and say that it needs to be acquired for the allocation/acquisition to work:
```
...
->needSlotLock("mylock(PHID-XYZQ-...)")
...
```
When you fire off the acquisition or allocation, it fails unless it could acquire the slot with that name. This is really simple (no explicit lock management) and a pretty good fit for most of the locking that blueprints and leases need to do.
If you need to do limit-based locks (e.g., maximum of 3 locks) you could acquire a lock like this:
```
mylock(whatever).slot(2)
```
Blueprints generally only contend with themselves, so it's normally OK for them to pick whatever strategy works best for them in naming locks.
This may not work as well if you have a huge number of slots (e.g., 100TB you want to give out in 1MB chunks), or other complex needs for locks (like you have to synchronize access to some external resource), but slot locks don't need to be the only mechanism that blueprints use. If they run into a problem that slot locks aren't a good fit for, they can use something else instead. For now, slot locks seem like a good fit for the problems we currently face and most of the problems I anticipate facing.
(The release workflows have other race issues which I'm not addressing here. They work fine if nothing races, but aren't race-safe.)
Test Plan:
To create a race where the same binding is allocated as a resource twice:
- Add `sleep(10)` near the beginning of `allocateResource()`, after the free bindings are loaded but before resources are allocated.
- (Comment out slot lock acquisition if you have this patch.)
- Run `bin/drydock lease ...` in two windows, within 10 seconds of one another.
This will reliably double-allocate the binding because both blueprints see a view of the world where the binding is free.
To verify the lock works, un-comment it (or apply this patch) and run the same test again. Now, the lock fails in one process and only one resource is allocated.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Differential Revision: https://secure.phabricator.com/D14118
Summary:
Ref T9253. Broadly, this realigns Allocator behavior to be more consistent and straightforward and amenable to intended future changes.
This attempts to make language more consistent: resources are "allocated" and leases are "acquired".
This prepares for (but does not implement) optimistic "slot locking", as discussed in D10304. Although I suspect some blueprints will need to perform other locking eventually, this does feel like a good fit for most of the locking blueprints need to do.
In particular, I've made the blueprint operations on `$resource` and `$lease` objects more purposeful: they need to invoke an activator on the appropriate object to be implemented correctly. Before they invoke this activator method, they configure the object. In a future diff, this configuration will include specifying slot locks that the lease or resource must acquire. So the API will be something like:
$lease
->setActivateWhenAcquired(true)
->needSlotLock('x')
->needSlotLock('y')
->acquireOnResource($resource);
In the common case where slot locks are a good fit, I think this should make correct blueprint implementation very straightforward.
This prepares for (but does not implement) resources and leases which need significant setup steps. I've basically carved out two modes:
- The "activate immediately" mode, as here, immediately opens the resource or activates the lease. This is appropriate if little or no setup is required. I expect many leases to operate in this mode, although I expect many resources will operate in the other mode.
- The "allocate now, activate later" mode, which is not fully implemented yet. This will queue setup workers when the allocator exits. Overall, this will work very similarly to Harbormaster.
- This new structure makes it acceptable for blueprints to sleep as long as they want during resource allocation and lease acquisition, so long as they are not waiting on anything which needs to be completed by the queue. Putting a `sleep(15 * 60)` in your EC2Blueprint to wait for EC2 to bring a machine up will perform worse than using delayed activation, but won't deadlock the queue or block any locks.
Overall, this flow is more similar to Harbormaster's flow. Having consistency between Harbormaster's model and Drydock's model is good, and I think Harbormaster's model is also simply much better than Drydock's (what exists today in Drydock was implemented a long time ago, and we had more support and infrastructure by the time Harbormaster was implemented, as well as a more clearly defined problem).
The particular strength of Harbormaster is that objects always (or almost always, at least) have a single, clearly defined writer. Ensuring objects have only one writer prevents races and makes reasoning about everything easier.
Drydock does not currently have a clearly defined single writer, but this moves us in that direction. We'll probably need more primitives eventually to flesh this out, like Harbormaster's command queue for messaging objects which you can't write to.
This blueprint was originally implemented in D13843. This makes a few changes to the blueprint itself:
- A bunch of code from that (e.g., interfaces) doesn't exist yet.
- I let the blueprint have multiple services. This simplifies the code a little and seems like it costs us nothing.
This also removes `bin/drydock create-resource`, which no longer makes sense to expose. It won't get locking, leasing, etc., correct, and can not be made correct.
NOTE: This technically works but doesn't do anything useful yet.
Test Plan: Used `bin/drydock lease --type host` to acquire leases against these blueprints.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Subscribers: Mnkras
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14117
Summary:
Ref T9253. The Drydock allocator is very pseudocodey right now. Particularly, it was written before Blueprints were concrete.
Reorganize it to make its responsibilities and error handling behaviors more clear.
In particular, the Allocator does not manage locks. It's primarily trying to reject allocations which can not possibly work. Blueprints are responsible for locks. See some discussion in D10304.
NOTE: This code probably doesn't work as written, see future diffs.
Test Plan: See future diffs.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Subscribers: cburroughs
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14114
Summary:
Ref T9253. Some of the Drydock code is pretty old. This applies standard modernizations to it:
- Modernize Query classes to use stuff like `buildWhereClauseParts()` and `loadStandardPage()`.
- Modernize all the getX() / attachX() stuff. In particular:
- Require and attach implementations to Blueprints.
- Require and attach Blueprints to Resources.
- BlueprintImplementations are now always unique per-Blueprint so they can store/cache state if they want without running over one another.
- BlueprintImplementations are now passed a `$blueprint`, like other similar APIs (this could go various ways but I generally like this as a balance of concerns).
NOTE: This probably doesn't run on its own, I'm just trying to split the next diff (core allocator stuff) up a bit and these pieces are all pretty standard.
Test Plan:
- Not much; see next revision or two.
- Clicked around Resource and Blueprint lists.
Reviewers: chad, hach-que
Reviewed By: chad, hach-que
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14113
Summary:
Ref T9253. See discussion in D13843.
I want to let Drydock blueprints for Almanac services choose those services from a typeahead, but only list appropriate services in the typeahead. To do this:
- Provide a StandardCustomField for an arbitrary datasource.
- Adjust the AlmanacServiceDatasource to allow filtering by service class.
This implementation is substantially the same as the one in D13843, with some adjustments:
- I lifted most of the code in the `Users` standard custom field into a new `Tokenizer` standard custom field.
- The `Users` and `Datasource` custom fields now extend the `Tokenizer` custom field and can share most of the code it uses.
- I exposed this field fully as a configurable field. I don't think anyone will ever use it, but this generality costs us nearly nothing and improves consistency.
- The code in D13843 didn't actually pass the parameters over the wire, since the object that responds to the request is not the same object that renders the field. Use the "parameters" mechanism in datasources to get things passed over the wire.
Test Plan:
- Created a custom "users" field in Maniphest and made sure it still wokred.
- Created a custom "almanc services" field in Maniphest and selected some services for a task.
- With additional changes from D13843, selected an appropriate Almanac service in a new Drydock blueprint.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14111
Summary:
Ref T9253. This comes from a time before Almanac. Now that we have Almanac, it makes much more sense to put this logic there than to try to put it in Drydock itself.
Remove the preallocated host blueprint, a relic of a bygone time.
Test Plan: Grepped for callsites.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14110
Summary: Ref T9253. See D13843 for some discussion. This is very bare-bones for now since I believe that almost all interesting configuration (e.g., credentials) should live in Drydock, although I imagine it getting some configuration eventually.
Test Plan: Used {nav Almanac > Services > Create Service} to create a new service of this type.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14109
Summary:
Fixes T9446. We allow administrators to send "Welcome" mail to bots and mailing lists.
This is harmless (these links do not function), but confusing.
Instead, disable this option in the UI and explain why it is disabled when it is clicked. Also prevent generation of this mail lower in the stack.
Test Plan:
- Viewed a bot page, saw action disabled, clicked it, got explanation.
- Viewed a normal user page, saw action enabled, clicked it, sent welcome email.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9446
Differential Revision: https://secure.phabricator.com/D14134
Summary: You can already pass other icons, but this makes it a bit simpler.
Test Plan: Test Maniphest, Badges
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14131
Summary: Builds a container of paste, makes it smaller on mobile.
Test Plan: View on desktop, tablet, mobile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14122
Summary: Adds full ROYGBIVP color spectrum, adds basic overflow, collapse protection.
Test Plan: Review small and large panels are various breakpoints.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14120
Summary: Making these a little more fun, a little more flexible and better looking. Will have an update for rSAAS in a bit.
Test Plan:
Make lots of them. Click.
{F815658}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14115
Summary:
I stumbled across this TODO and was worried that there was a glaring hole in MFA that I'd somehow forgotten about, but the TODO is just out of date.
These actions are rate limited properly by `PhabricatorAuthTryFactorAction`, which permits a maximum of 10 actions per hour.
- Remove the TODO.
- Add `bin/auth unlimit` to make it easier to reset rate limits if someone needs to do that for whatever reason.
Test Plan:
- Tried to brute force through MFA.
- Got rate limited properly after 10 failures.
- Reset rate limit with `bin/auth unlimit`.
- Saw the expected number of actions clear.
{F805288}
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Differential Revision: https://secure.phabricator.com/D14105
Summary: Fixes T9392, adds some sweet sweet margin to the pager.
Test Plan: See pager with new padding, test different pages, breakpoints.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9392
Differential Revision: https://secure.phabricator.com/D14098
Summary:
This reverts commit 1583738842.
See T8646 for discussion. This version of the feature feels terrible on real data.
Test Plan: Strict revert.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14097
Summary:
Fixes T9126. In particular:
- Add "Browse" links to all history views.
- Use icons to show "Browse" and "History" links, instead of text.
- Use FontAwesome.
- Generally standardize handling of these elements.
This might need a little design attention, but I think it's an improvement overall.
Test Plan:
- Viewed repository history.
- Viewed branch history.
- Viewed file history.
- Viewed table of contents on a commit.
- Viewed merged changes on a merge commit.
- Viewed a directory containing an external.
- Viewed a deleted file.
{F788419}
{F788420}
{F788421}
{F788422}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9126
Differential Revision: https://secure.phabricator.com/D14096
Summary:
Ref T8646. This is fairly rough:
This interface is very niche, and not really flexible enough to accommodate other result customization (but I don't think we have any plans here)?
I'm just //summarizing// the content of documents, basically showing the first paragraph of their content, summary, etc. This isn't what Google does: it shows snippets surrounding the actual search terms. However, this is more involved and might be less useful in structured data: for example, I'd imagine that the first line of most phriciton documents, maniphest tasks and Differential revisions really might be the best machine-generatable summary of them. The actual contextual snippeting in Google doesn't often seem hugely useful to me. But this might also not be very useful.
There's not much design, not sure if you had any ideas.
I only implemented this for tasks, revisions and the wiki since those seem most useful.
I'm generally on the fence about this, but it's not a ton of work to swap out for something else later. Maybe we can see how it feels? But happy to toss it or rethink the approach.
Test Plan: {F788026}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8646
Differential Revision: https://secure.phabricator.com/D14095
Summary: Reachable via the cache config page, restricted to admins only. This makes it convenient to hotfix phabricator without requiring a restart.
Test Plan:
- Local dev machine doesn't have apc, so I get the not installed message.
- Faked the name and isEnabled parameters, verified dialog shows up as expected.
- Didn't test clear code
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: tycho.tatitscheff, joshuaspence, Korvin
Differential Revision: https://secure.phabricator.com/D14064
Summary: Fixes T9351. This is straightforward since this application is now relatively modern and doesn't have any bizarre craziness.
Test Plan:
{F787981}
{F787982}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9351
Differential Revision: https://secure.phabricator.com/D14093
Summary: As described in T7959, it looks like Diffusion does not provide Mercurial the required HTTP credentials when pulling from an external repository.
Test Plan: Add an external Mercurial repository to Diffusion, that requires HTTP authentication. A private BitBucket repository for example.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Projects: #mercurial, #diffusion
Differential Revision: https://secure.phabricator.com/D14092
Summary: Fixes T9380. See that task for discussion. This doesn't feel awesome but is maybe the least-bad fix? I think this name is clearer.
Test Plan: Looked at autoplan in Harbormaster, saw new name.
Reviewers: meitros, chad
Reviewed By: chad
Maniphest Tasks: T9380
Differential Revision: https://secure.phabricator.com/D14088
Summary: Fixes T9369.
Test Plan:
- Sent a mail with Mail.app to `bugs@local.phacility.com`.
- Used "View Raw Mail", copy-pasted it into `mail.txt` on disk.
- Ran `cat mail.txt | ./scripts/mail/manage_mail.php --process-duplicates`.
- Saw task get created and me get added as CC.
- Changed "To" to include another user, ran command again, saw task get created and other user get added as CC.
Reviewers: chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T9369
Differential Revision: https://secure.phabricator.com/D14086
Summary:
- Fix missing space before "For example:".
- Fix instruction to run `bin/config set value` instead of `bin/config set key value`.
- Minor cleanup.
Test Plan: Tried to set `files.image-mime-types`, `load-libraries`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14080
Summary:
Ref T9346. This mostly allows us to give users additional advice based on which instance they are trying to log in to in the Phacility cluster.
It's also slightly more flexible than `auth.login-message` was, and maybe we'll add some more hooks here eventually.
This feels like it's a sidegrade in complexity rather than really an improvement, but not too terrible.
Test Plan:
- Wrote the custom handler in T9346 to replicate old config functionality.
- Wrote a smart handler for Phacility that can provide context-sensitive messages based on which OAuth client you're trying to use.
See new message box at top (implementation in next diff):
{F780375}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9346
Differential Revision: https://secure.phabricator.com/D14057
Summary:
Ref T7148. I can do most of the export stuff by only modifying the Instances codebase, but want to upload all the backups and exports as temporary files and can't currently do this via the API.
Make the necessary API changes so that the export workflow can use them when it gets built out.
Test Plan: See next diff. Uploaded files with `arc upload --temporary` and saw them upload as temporary files.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7148
Differential Revision: https://secure.phabricator.com/D14055
Summary: There are a handful of places I've been wanting to use a button here. Adds that ability and uses in app launcher.
Test Plan:
Test Applicatons->Launcher at desktop, mobile, tablet breakpoints
{F780453}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14059
Summary:
Fixes T9339.
- Don't show edit control for locked config at all.
- Don't show a "Cancel" button either.
- Change "Value" label to "Database Value" for non-custom config.
- Highlight effective value.
- Move examples under current state.
- Tweak some formatting.
Test Plan: {F777878}
Reviewers: chad, avivey
Reviewed By: chad, avivey
Subscribers: avivey
Maniphest Tasks: T9339
Differential Revision: https://secure.phabricator.com/D14054
Summary: Makes the New Comment, See Comments more obviously placed to find.
Test Plan: Review new CSS, answer question, comment, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14043
Summary:
Ref T7173. Depends on D14049. Now that Phacility can install custom exception handlers, this puts enough information on the exception so that we can figure out what to do with it.
- Generally modernize some of this code.
- Add some more information to PolicyExceptions so the new RequestExceptionHandler can handle them properly.
Test Plan: Failed authorizations, then succeeded authorizations. See next diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7173
Differential Revision: https://secure.phabricator.com/D14050
Summary:
Ref T1806. Ref T7173. Depends on D14047.
Currently, all exception handling is in this big messy clump in `AphrontDefaultApplicationConfiguration`.
Split it out into modular classes. This will let a future change add new classes in the Phacility cluster which intercept particular exceptions we care about and replaces the default, generic responses with more useful, tailored responses.
Test Plan:
{F777391}
- Hit a Conduit error (made a method throw).
- Hit an Ajax error (made comment preview throw).
- Hit a high security error (tried to edit TOTP).
- Hit a rate limiting error (added a bunch of email addresses).
- Hit a policy error (tried to look at something with no permission).
- Hit an arbitrary exception (made a randomc ontroller throw).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T1806, T7173
Differential Revision: https://secure.phabricator.com/D14049
Summary:
Ref T1806. Ref T7173. Context here is that I want to fix "you can not log in to this instance" being a confusing mess with an opaque error. To do this without hacks, I want to:
- clean up some exception handling behavior (this diff);
- modularize exception handling (next diff);
- replace confusing, over-general exceptions with tailored ones in the Phacility cluster, using the new modular stuff.
This cleans up an awkward "AphrontUsageException" which does some weird stuff right now. In particular, it is extensible and extended in one place in Diffusion, but that extension is meaningless.
Realign this as "AphrontMalformedRequestException", which is a better description of what it is and does: raises errors before we can get as far as normal routing and site handling.
Test Plan: Hit some of these exceptions, saw the expected "abandon all hope" error page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T1806, T7173
Differential Revision: https://secure.phabricator.com/D14047
Summary:
Ref T9309. This is a minor quality of life improvement, hopefully. We already have print CSS, just expose it more clearly.
Also, hide actions (these never seem useful?) and footers from printable versions. I opened the printable version in a new window since it now doesn't have any actions.
Test Plan:
{F777241}
{F777242}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9309
Differential Revision: https://secure.phabricator.com/D14045
Summary:
Fixes T9080. We try to list alternatives for the current ref (for example, if you're viewing a branch named "master" but there's also a tag named "master", or, in Mercurial, there are several branches named "master") but fail to abruptly if we can't get the list.
It's fine if we can't get the list; just continue. This is common when the repository hasn't cloned yet.
Test Plan: In a local repository with bad credentials, tried to do anything before and after. Before: completely blocked by error; after: things work normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9080
Differential Revision: https://secure.phabricator.com/D14044
Summary:
Ref T1806. Ref T5752. Currently, `handleRequest()` needs to return an `AphrontResponse`, but sometimes it's really convenient to return some other object, like a Dialog, and let that convert into a response elsewhere.
Formalize this and clean up some of the existing hacks for it so there's less custom/magical code in Phabricator-specific classes and more general code in Aphront classes.
More broadly, I want to clean up T5752 before pursuing T9132, since I'm generally happy with how `SearchEngine` works except for how it interacts with side navs / application menus. I want to fix that first so a new Editor (which will have a lot in common with SearchEngine in terms of how controllers interact with it) doesn't make the problem twice as bad.
Test Plan:
- Loaded a bunch of normal pages.
- Loaded dialogs.
- Loaded proxy responses (submitted empty comments in Maniphest).
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Maniphest Tasks: T1806, T5752
Differential Revision: https://secure.phabricator.com/D14032
Summary: See D14025. In all cases where we compare hashes, use strict, constant-time comparisons.
Test Plan: Logged in, logged out, added TOTP, ran Conduit, terminated sessions, submitted forms, changed password. Tweaked CSRF token, got rejected.
Reviewers: chad
Reviewed By: chad
Subscribers: chenxiruanhai
Differential Revision: https://secure.phabricator.com/D14026
Summary:
Fixes T9302. This datasource wasn't resolving package PHIDs correctly for the actual query.
Also fixes an issue with the "Affected packages that need audit" Herald rule.
Test Plan: Ran a "Needs Audit" query with only packages, and only `packages(user)`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9302
Differential Revision: https://secure.phabricator.com/D14029
Summary:
Ref T8320. I missed this a while ago and then it came to me in a dream.
Only consider paths in the same repo when looking at ownership.
(I think this is rarely reachable in practice.)
Test Plan: Verified that files and commits still listed ownership properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8320
Differential Revision: https://secure.phabricator.com/D14022
Summary:
Fixes T9218. Fixes T8320. Fixes T8661. This isn't exhaustive but documents the stuff that cropped up in this iteration as needing documentation. In particular:
- Be explicit about multiple ownership.
- Explain value of having one place to update your giant regexp of a trillion paths.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8320, T8661, T9218
Differential Revision: https://secure.phabricator.com/D14023
Summary:
Fixes T9279. Modernizes the SearchEngine and Query classes. User-facing changes:
- Added order by commit date, default to order by commit date with newest commits first.
- Added explicit "Needs Audit by".
- Added new `packages(...)` typeahead function.
- Picked up automatic subscribers, projects, and order fields.
This changes behavior a little bit: we previously attempted to exclude, e.g., commits which a package you own needs to audit, but which you have resigned from. This is difficult in general and I think it needs a more comprehensive solution. This shouldn't impact users much, anyway.
Test Plan: {F767628}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9279
Differential Revision: https://secure.phabricator.com/D14013
Summary: Ref T9089. This link leads to a detail page, not an edit page, and is always visible by users with permission to see the column.
Test Plan: Clicked "Column Details" with and without edit permission.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9089
Differential Revision: https://secure.phabricator.com/D14016
Summary: Fixes T9090. You don't need to be able to edit a project to create tasks on its workboard. Being able to view the project is sufficient, and the user certianly can if they got this far.
Test Plan: Viewed workboard, hit "Create Task".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9090
Differential Revision: https://secure.phabricator.com/D14015
Summary: Fixes T9278. Logged out viewers shouldn't see a form field to answer, just a login button.
Test Plan: Log out, go to question, click Login to Answer, login, get redirected back.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9278
Differential Revision: https://secure.phabricator.com/D14012
Summary:
This enables CORGI.
Currently, `AphrontSite` subclasses can't really have their own routes. They can do this sort of hacky rewriting of paths, but that's a mess and not desirable in the long run.
Instead, let subclasses build their own routing maps. This will let CORP and ORG have their own routing maps.
I was able to get rid of the `PhameBlogResourcesSite` since it can really just share the standard resources site.
Test Plan:
- With no base URI set, and a base URI set, loaded main page and resources (from main site).
- With file domain set, loaded resources from main site and file site.
- Loaded a skinned blog from a domain.
- Loaded a skinned blog from the main site.
- Viewed "Request" tab of DarkConsole to see site/controller info.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14008
Summary: Adds an additional field for questions, an answer wiki, should should usually be community editable.
Test Plan: New question, edit question, no wiki, lots of wiki.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14003
Summary: Adds a notice reminding viewers of their own question to resolve it and mark the correct answer.
Test Plan:
View my own open question, see notice. Resolve question, notice goes away.
{F743481}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13958
Summary: Updates Releeph callsites to handleRequest
Test Plan: Bounce around Releeph, cut a branch, edit a product, view history
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14001
Summary: Fixes T9217, adds detection for logged in users and adjusts the layout accordingly.
Test Plan: View logged in and logged out Conpherence
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9217
Differential Revision: https://secure.phabricator.com/D14002
Summary: Until we have a proper close as duplicate workflow for Ponder, remove the option with something more sensible.
Test Plan: Closed a question as invalid, saw it closed and in feed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14007
Summary: Ref T9134. It looks like this functionality was removed in D13848.
Test Plan: Submitted a diff successfully.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9134
Differential Revision: https://secure.phabricator.com/D13869
Summary:
Email is so exciting I can't wait 30 days for initial results.
ref T9161
Test Plan:
* `./bin/mail volume --days 60` took longer and gave plausibly larger
results.
* `./bin/mail volume --days 0` quickly told me no mail had been sent.
* `./bin/mail volume` Said it was still looking 30 days back.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T9161
Differential Revision: https://secure.phabricator.com/D13901
Summary:
We currently detect tab panels embedding themselves, but do not detect text panels embedding themselves with `{Wxx}`.
Detect these self-embedding panels.
I had to add a bit of a hack to pass the parent panel PHIDs to the rule. Generally, I got the Markup API kind of wrong and want to update it, I'll file a followup with details about how I'd like to move forward.
Test Plan:
Created a text panel embedding itself, a tab panel embedding a text panel embedding itself, a tab panel embedding a text panel embedding the tab panel, etc.
Rendered all panels standalone and as `{Wxx}` from a different context.
{F761158}
{F761159}
{F761160}
{F761161}
{F761162}
Reviewers: chad, jbeta
Reviewed By: chad, jbeta
Differential Revision: https://secure.phabricator.com/D13999
Summary:
Fixes T9268. Currently, we try to match any string like "a2f313f1" as a commit/revision, so short hashes will get picked up.
However, we don't require a word boundary or terminal after the match, so for input like "aaa...aaaaz" the engine can get stuck trying to split the string into sub-matches.
That is, in the original case, the input "aaaz" had valid matches against `[rA-Z0-9a-f]+` up to "z" of:
aaa
aa a
a aa
a a a
All of these will fail once it hits "z", but it has to try them all. This complexity is explosive with longer strings.
Instead, require a word boundary or EOL after the match, so this is the only valid match:
aaa
Then the engine sees the "z", says "nope, no match" and doesn't have to backtrack across all possible combinations.
Test Plan: Added a failing unit test, applied patch, clean test.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9268
Differential Revision: https://secure.phabricator.com/D13997
Summary:
Fixes T9251. Old mail could get saved with bad parameters for two reasons that I can come up with:
- Nothing ever set a parameter on it -- not sure this could ever actually happen; or
- some field contained non-UTF8 data prior to D13939 and we silently failed to encode it.
My guess is that the second case is probably the culprit here.
In any case, recover from this so `20150622.metamta.5.actor-phid-mig.php` can proceed.
Test Plan: Same effective patch as user patch in T9251; looked at some mail to make sure it was still pulling parameters properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9251
Differential Revision: https://secure.phabricator.com/D13990
Summary:
Ref T8783. Sort out some relationships and fields:
- Make Items 1:1 with Queues: each item is always in exactly one queue. Minor discussion on T8783. I think this is easier to understand and reason about (and implement!) and can't come up with any real cases where it isn't powerful enough.
- Remove "QueueItem", which allowed items to be in multiple queues at once.
- Remove "dateNuanced", which is equivalent to "dateCreated" in all cases.
Then add really basic routing:
- Add "Default Queue" for Sources. New items from the source route into that queue.
- (Some day there will be routing rules, but for now the rule is "always route into the default queue".)
- Show queue on items.
- Show more / more useful edit history and transactions in several UIs.
Test Plan:
{F749445}
{F749446}
{F749447}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8783
Differential Revision: https://secure.phabricator.com/D13988
Summary: Ref T2015. This fixes issues where the Drydock queries wouldn't filter (or throw an exception) when passed empty arrays for their `with` methods. In addition, this also adds `array_unique` to the resource and lease subqueries so that we don't pull in a bunch of stuff if logs or leases have the same related objects.
Test Plan: Tested it by using DarkConsole on the log controller.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D10879
Summary: Ref T2015. This allows searching based on blueprints, resources or leases when viewing the logs, which helps when searching for events that occured to a particular blueprint / resource / lease. Unlike the logs shown on the resource / lease pages, the search engine supports paging properly, which means it can be used to find entries in the past.
Test Plan: Used the Drydock log search page.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D10874
Summary: Show the time in addition to the date in the Drydock logs.
Test Plan: Brought forward from D10479.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10909
Summary: Ref T1049. This ensures the Harbormaster build target is associated with leases, so in the future we can query things and find out whether builds are still running with associated leases.
Test Plan: Leased a host, checked the DB and saw the field populated.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10870
Summary:
Ref T8783.
The "View" UI is where a user would check their request for feedback or a resolution, if it's something that makes sense for them to interact with from the web UI.
The "Edit" UI is the manage/admin UI where you'd respond to a request. It's similar to the view UI but will have actions and eventually some queue UI, etc.
(I don't think items need a normal "Edit" UI -- it doesn't make sense to "Edit" a tweet or inbound email -- but maybe this will shuffle around a little eventually.)
Test Plan:
View
{F747218}
Edit
{F747219}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8783
Differential Revision: https://secure.phabricator.com/D13980
Summary:
Fixes T9245. These picked up some possibly-confusing metadata, like in the screenshot on T9245 where "Subscribers" appears in the middle of the page for no obvious reason.
- Make these pages a little cleaner by removing elements which aren't important for signing agreements.
- Use the last time the actual document text was updated as the modification time, not the last time the "Document" object was modified. The latter will change for trivial things like altering the view/edit policy, but that could be confusing if you see that a TOS was "last updated yesterday" but can't figure out what actually changed (since nothing changed).
Test Plan: Viewed signature page for a document.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9245
Differential Revision: https://secure.phabricator.com/D13982
Summary:
Ref T8783. If you have a source (like a "report bug" form), let it put a link (like "View Form") on the source detail page.
This also straightens out getting definitions from sources, which had a bug with the modern way we do `PhutilClassMapQuery`.
Specifically, if you called the old mechanism on two different sources, they'd return the same definition object, but they need to return different definitions.
Test Plan:
{F747093}
{F747092}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8783
Differential Revision: https://secure.phabricator.com/D13966
Summary: Ref T8783. There's nothing at `/nuance/` right now, put something basic there.
Test Plan: {F747078}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8783
Differential Revision: https://secure.phabricator.com/D13965
Summary:
Ref T8672. Ref T9187. Root issue in at least one case is:
- User makes a commit including a file with some non-UTF8 text (say, a Japanese file full of Shift-JIS).
- We pass the file to the TransactionEditor so it can inline or attach the patch if the server is configured for these things.
- When inlining patches, we convert them to UTF8 before inlining. We must do this since the rest of the mail is UTF8.
- When attaching patches, we send them in the original encoding (as file attachments). This is correct, and means we need to give the worker the raw patch in whatever encoding it was originally in: we can't just convert it to utf8 earlier, or we'd attach the wrong patch in some cases.
- TransactionEditor does its thing (e.g., creates the commit), then gets ready to send mail about whatever it did.
- The publishing work now happens in the daemon queue, so we prepare to queue a PublishWorker and pass it the patch (with some other data).
- When we queue workers, we serialize the state data with JSON.
So far, so good. But this is where things go wrong:
- JSON can't encode binary data, and can't encode Shift-JIS. The encoding silently fails and we ignore it.
Then we get to the worker, and things go wrong-er:
- Since the data is bad, we fatal. This isn't a permanent failure, so we continue retrying the task indefinitely.
This applies several fixes:
# When queueing tasks, fail loudly when JSON encoding fails.
# In the worker, fail permanently when data can't be decoded.
# Allow Editors to specify that some of their data is binary and needs special handling.
This is fairly messy, but some simpler alternatives don't seem like good ways forward:
- We can't convert to UTF8 earlier, because we need the original raw patch when adding it as an attachment.
- We could encode //only// this field, but I suspect some other fields will also need attention, so that adding a mechanism will be worthwhile. In particular, I suspect filenames //may// be causing a similar problem in some cases.
- We could convert task data to always use a serialize()-based binary safe encoding, but this is a larger change and I think it's correct that things are UTF8 by default, even if it makes a bit of a mess. I'd rather have an explicit mess like this than a lot of binary data floating around.
The change to make `LiskDAO` will almost certainly catch some other problems too, so I'm going to hold this until after `stable` is cut. These problems were existing problems (i.e., the code was previously breaking or destroying data) so it's definitely correct to catch them, but this will make the problems much more obvious/urgent than they previously were.
Test Plan:
- Created a commit with a bunch of Shift-JIS stuff in a file.
- Tried to import it.
Prior to patch:
- Broken PublishWorker with distant, irrelevant error message.
With patch partially applied (only new error checking):
- Explicit, local error message about bad key in serialized data.
With patch fully applied:
- Import went fine and mail generated.
Reviewers: chad
Reviewed By: chad
Subscribers: devurandom, nevogd
Maniphest Tasks: T8672, T9187
Differential Revision: https://secure.phabricator.com/D13939
Summary: Fixes T9241. Users have a tendancy to assume Ponder is a "forum", make replying to your own question take an additional click.
Test Plan:
View my own question, see notice, click open answer box, reply. Visit not my question, see box as normal.
{F743412}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: cspeckmim, Korvin
Maniphest Tasks: T9241
Differential Revision: https://secure.phabricator.com/D13957
Summary: Ref T8588. It looks like something slow is happening //before// we start DarkConsole. Add some crude reporting to try to narrow it down.
Test Plan: {F743050}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8588
Differential Revision: https://secure.phabricator.com/D13956
Summary:
Fixes T9237. A while ago, the old (more-pink) indigo got split into "pink" (more pink) and "indigo" (more purple), but we didn't change this color config in Maniphest.
This generally made the color more purple, and it's now pretty simliar to the "needs triage" color (violet).
Make it "pink" instead.
Test Plan: {F742617}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9237
Differential Revision: https://secure.phabricator.com/D13954
Summary: Should fix all email reply issues, but no solid means of testing at home (how do you local reply test?)
Test Plan: Check for answer mail in /mail/ and see proper headers. Make sure question mail works too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3846
Differential Revision: https://secure.phabricator.com/D13951
Summary: Fixes T9234. The joins method was still the old method and the builtin was calling the wrong key.
Test Plan: Test authored builtin, custom search
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9234
Differential Revision: https://secure.phabricator.com/D13953
Summary: There is still some general buginess with answer comments, trying to work them out. This replaces timeline rendering into one offs (less performant) but resolves many bugs. Or if there is a more performant way, let me know? Also when leaving an answer comment, you currently get redirected back to the page, but both the comment form is still populated and you dont see your answer without a reload. I feel like I'm missing some magical parameter to pass, so just redirecting back to the question itself.
Test Plan: Leave lots of answer comments.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13946
Summary: Fixes T9226, shows the update time, not the creation time.
Test Plan: Update an answer, see new date.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9226
Differential Revision: https://secure.phabricator.com/D13945
Summary:
Ref T9218. See discussion there for rationale; I think this is the right behavior to pursue.
The screenshot below is pretty ugly. I think it's a lot worse than most real-world cases will be, since you have to sort of opt-in to having crazy levels of overlapping packages, and it's perfectly normal/reasonable for files owned by one package. Owners is powerful enough to let you specify sub-packages with exclusive ownership.
That said, this may be more typical than I hope. I don't think we can reduce the complexity here much for free, but it would might be reasonable to add some view options (e.g.: group by package?, show only packages I own?, show packages as icons with a tooltip?) if it's an issue.
Test Plan: {F734956}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9218
Differential Revision: https://secure.phabricator.com/D13940
Summary: Fixes T8428. Adds status to packages, allows setting and application search. I presume though these need checked elsewhere?
Test Plan: New package, edit package, archive package, run search queries.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8428
Differential Revision: https://secure.phabricator.com/D13925
Summary: This algorithm isn't quite right with respect to ordering more-specific packages correctly.
Test Plan: {F729864}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D13929
Summary: Pending other diffs, this actually removes Ponder as a prototype. Fixes T3578
Test Plan: No longer listed as a prototype
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3578
Differential Revision: https://secure.phabricator.com/D13920
Summary: Ref T9205. This is a likely fix.
Test Plan: This isn't straightforward to test in the upstream unless you have custom code on top of it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9205
Differential Revision: https://secure.phabricator.com/D13928
Summary: Fixes T9207. I think this is correct? Sorts based on vote count and reverses the order of the array so higher is first.
Test Plan: Test vote ordering on a number of sample tasks.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9207
Differential Revision: https://secure.phabricator.com/D13924
Summary: Ref T9068, Ref T3846. Maybe fixes both, but I'm having issues testing email replies in a sandbox. Moves answer feed/mail generation to the AnswerEditor, hides it in QuestionEditor.
Test Plan: Write an answer, see feed story, check /mail/ for mail generation.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3846, T9068
Differential Revision: https://secure.phabricator.com/D13905
Summary:
Fixes T8004.
- For paths which are part of a package, show the package.
- Highlight paths which are part of a package you (the viewer) have authority over.
Test Plan:
{F725418}
- Viewed owned and unowned chagnes in Diffusion and Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8004
Differential Revision: https://secure.phabricator.com/D13923
Summary: These were a little silly, simplify them
Test Plan: New countdown, edit preferences, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13888
Summary:
Ref T8320. This field doesn't have any special effects and has no reason to exist. Primary owners are always already owners.
This makes one minor implicit change to Owners: it is now possible to create a package with no owners. That seems fine; it is reasonable to create a package purely as an organizational tool and use it in, e.g., Herald.
Test Plan: Created and edited packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8320
Differential Revision: https://secure.phabricator.com/D13921
Summary:
Ref T9201. At the root of a repository the current path is `null`, but the OwnersQuery wants strings.
This could be resolved a couple different ways, but just cast the arguments to strings since that seems reasonable enough.
Test Plan: Browsed root of a repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9201
Differential Revision: https://secure.phabricator.com/D13919
Summary: When using the active query in Macro, all Macros are returned. Properly set the status query.
Test Plan: Review active macros, don't see archived macros.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13912
Summary: Ref T8700, I don't believe we need to be specific here about the object, since it displays on the object.
Test Plan: Change policy a few times on a task, see new translation
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8700
Differential Revision: https://secure.phabricator.com/D13913
Summary:
Ref T8320. Ref T8004. This just tries to generally modernize
It also replaces the nonfunctional "Find Owners" link with a new property that just shows owning packages.
Test Plan:
- Created and edited packages.
{F720478}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8004, T8320
Differential Revision: https://secure.phabricator.com/D13911
Summary:
Fixes T2183. We now use the same rendering element in both places.
Intentional changes:
- Package highlighting is out, coming back to both apps in next diff.
- removed redundant-feeling "Change" link. The information is now shown with a character ("M", "V", etc.) and the page is a click away under "History". Clicking the path also jumps you to substantially similar content. (We could restore it fairly easily, I just think it's probably the least useful thing in the table right now.)
Test Plan: Viewed a bunch of commits in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13910
Summary:
Ref T2183. Introduces a new View which can (in theory) unify the Revision, Diff and Commit table of contents views.
This has the same behavior as before, but accepts slightly more general primitives and parameters and has somewhat cleaner code.
I've made one intentinoal behavior change: removing the "Open All in Editor" button. I suspect this is essentially unused, and is a pain to keep around. We can look at restoring it if anyone notices.
Test Plan: Looked at a bunch of revisions, no changes from before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13908
Summary: Fixes T9099, I think this is as much as I can come up with for unbeta. Cleans up the answer header (profile image, smaller font, smaller header). Cleans up voting (new, with color), and makes it a bit more readable.
Test Plan:
Review a number of answers in ponder with and without votes, comments.
{F720189}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9099
Differential Revision: https://secure.phabricator.com/D13907
Summary: Ref T5791. This is still very basic (no global actions, no support for matching headers/bodies/recipients/etc) but gets the core in.
Test Plan:
{F715209}
{F715211}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13897
Summary: Ref T2183. These properties are not used and not useful.
Test Plan: Grepped for callsites and uses. Loaded revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13906
Summary: Fixes T8659. This isn't //explicitly// documented but I'm going to wait for a bit until the "Harbormaster" doc splits into internal/external builds to add docs for it. There's other similar stuff coming soon anyway.
Test Plan:
{F716439}
{F716440}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8659
Differential Revision: https://secure.phabricator.com/D13903
Summary:
Ref T8659. In the general case, this eventually allows build processes to do things like:
- Upload build results (like a ".app" or ".exe" or other binary).
- Pass complex results between build steps (e.g., build step A does something hard and build step B uses it to do something else).
Today, we're a long way away from having the infrastructure for that. However, it is useful to let third party build processes (like Jenkins) upload URIs that link back to the external build results.
This adds `harbormaster.createartifact` so they can do that. The only useful thing to do with this method today is have your Jenkins build do this:
params = array(
"uri": "https://jenkins.mycompany.com/build/23923/details/",
"name": "View Build Results in Jenkins",
"ui.external": true,
);
harbormaster.createartifact(target, 'uri', params);
Then (after the next diff) we'll show a link in Differential and a prominent link in Harbormaster. I didn't actually do the UI stuff in this diff since it's already pretty big.
This change moves a lot of code around, too:
- Adds PHIDs to artifacts.
- It modularizes build artifact types (currently "file", "host" and "URI").
- It formalizes build artifact parameters and construction:
- This lets me generate usable documentation about how to create artifacts.
- This prevents users from doing dangerous or policy-violating things.
- It does some other general modernization.
Test Plan:
{F715633}
{F715634}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8659
Differential Revision: https://secure.phabricator.com/D13900
Summary: I derped here and broke new questions, also remove old Answer constants
Test Plan: Ask a new question, see it save
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13899
Summary: Ref T9173, adds basic hide support for answers. Answer authors and Moderators can hide answers, unhide them.
Test Plan: Hide answer, log into other account, see hidden message. Mark as visible, see answer again.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9173
Differential Revision: https://secure.phabricator.com/D13894
Summary: Ref T5791. This collects outbound mail status in one place and makes the list view a little spiffier.
Test Plan: Looked at list and detail views. Grepped for changed classes/constants.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5791
Differential Revision: https://secure.phabricator.com/D13884
Summary: Ref T3578, Changes "Ask Away" to just "Submit", Changes Description to Details, check for is_new when offering closed state on question edit.
Test Plan: New Question, Edit Question
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3578
Differential Revision: https://secure.phabricator.com/D13891
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.
Test Plan: Poked around a bunch of pages.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13589
Summary: Ref T3578 Adds mailtags to Ponder, answer stuff not quite ready, but that's another diff.
Test Plan: set preferences to notify, second account updates a question, get notification on first.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3578
Differential Revision: https://secure.phabricator.com/D13886
Summary:
Ref T9141. Ref T5791. Ref T7013. Major changes here is:
- Currently, we don't store the headers we actually sent, or the reasons we actually did or did not deliver a mail.
- Start storing these (as `headers.sent` and `actors.sent`).
- Show them in the web UI.
- Show them in `bin/mail show-outbound` (previously, we sort of re-computed them in a hacky way).
- Take them into account in `bin/mail volume`.
Then some minor changes:
- Show mail bodies.
- Show more mail information.
- Start renaming "MetaMTA" to "Mail", at least in the web UI.
Test Plan:
{F707501}
{F707502}
{F707503}
{F707504}
{F707505}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5791, T7013, T9141
Differential Revision: https://secure.phabricator.com/D13878
Summary:
Ref T9099.
- Remove unused function
- Fix celerity resource include
- Add a note when an answer's been closed
- Clean up architecture a little
- Add a better notice when you've already answered.
Test Plan: Test with and without answers, with answering, with being closed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9099
Differential Revision: https://secure.phabricator.com/D13875
Summary: In Ponder, I think the right default query is "good" questions. That being recently asked, and either open or resolved. For browsing this seems correct. For moderators, I presume they use saved queries accordingly. This is similar to stack overflow and quora.
Test Plan: Review queries in my dev box, all seems to run correctly
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13877
Summary: Ref T9099, A step forward for the main Ponder UI. Mostly moving stuff into View classes and reducing clutter. Took a pass at keeping comments and helpfuls, but unclear what the 'final' UI will be (I'm just designing as I use the product).
Test Plan:
Review a number of questions and answers.
{F702495}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9099
Differential Revision: https://secure.phabricator.com/D13872
Summary: Fixes T9128. I missed this when converting other controllers to look in Harbormaster for lint/unit data.
Test Plan: Copy/pasted a diff successfully.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9128
Differential Revision: https://secure.phabricator.com/D13865
Summary:
Ref T8096. This modernizes the last thing which was reading the old datasource.
Also fix a bug where it didn't work.
Test Plan: {F698405}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13852
Summary: Ref T9099. Testing out a two column layout in Ponder, with the main idea being creating a more browsable, discoverable product. I'd like the side column though to be a little smarter and provide project based searching. Ideally, if I'm reading Resolved Maniphest questions, other Resolved Maniphest questions are likely interesting. Another scenario is if I'm answering questions, in which case browsing more Open questions would also be interesting. Ponder "Main Column" still needs to be redesigned.
Test Plan: Browse open questions, resolved questions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9099
Differential Revision: https://secure.phabricator.com/D13849
Summary: Ref T8096. This information has moved into Harbormaster.
Test Plan:
Pushed some test results in; see right margin:
{F698394}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13850
Summary:
Ref T8096.
Long ago, support for "postponed" lint and unit tests got hacked in. `arc` would publish a bunch of ghost results, and then something else would fill the results in later.
This was always a hack. It is not nearly as powerful or flexible as having a real build system, and is obsolete with Harbormaster, which supports these operations in a more reasonable and straightforward way.
This was used (only? almost only?) at Facebook.
- Remove `differential.finishpostponedlinters`. This only served to update postponed linters.
- Remove lint magic in `differential.setdiffproperty`. This magic only made sense in the context of postponed linters.
- Remove `differential.updateunitresults`. The only made sense for postponed unit tests.
And one minor change: when a diff contains >100 affected files, we hide the content by default, but show content for files with inline comments. Previously, we'd do this for lint inlines, too. I don't tink this is too useful, and it's much simpler to just remove it. We could add it back at some point, but I think large changes often trigger a lot of lint and no one actually cares. The behavior for actual human inlines is retained.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13848
Summary: Ref T8096. These are still reading out of the old diff property, but should read from Harbormaster instead.
Test Plan: {F698346}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13847
Summary:
Ref T8096. Hit this on IRC:
> epriestley: what's in the Messages tab?
> jdoe: what Messages tab??!
> epriestley: ughhhh
Test Plan: Clicked "Messages" tab, saw helpful "no messages" message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13846
Summary:
Fixes T7370. Two changes:
- Make the default to show nothing, instead of showing all the data. This is a better default because the data is sometimes sensitive. Workers should have to opt in to revealing it.
- For TargetWorkers, link to the target (technically the build, for now, since there's no dedicated target detail page).
Test Plan: {F698325}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7370
Differential Revision: https://secure.phabricator.com/D13845
Summary: Ref T8096. Like stop/start, autoplans are pushed into the system from outside (normally by `arc`) so it doesn't make any sense to run them manually.
Test Plan:
- Tried to run an autoplan from web UI, got an error.
- Ran a normal plan from web UI.
- Tried to run an autoplan from CLI, got an error.
- Ran a noraml plan from CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13844
Summary: Ref T6920, this adds a basic controller for marking an answer as helpful and removes the negative voting. Any current positive vote is kept as helpful. New UI is needed here, but there is a separate task for redesigning Ponder overall.
Test Plan: Mark an answer as helpful, see count go up, remove helpful, see count go down. Test endpoint manually.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T6920
Differential Revision: https://secure.phabricator.com/D13834
Summary: Updates the Daemon application calls
Test Plan: Click on various things in the Daemon app
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13835
Summary: Ref T6920, This removes the PonderVotableInterface from PonderQuestion and assocaited code. Also... never used?
Test Plan: Visit Ponder, See List, New Question, Add Answer.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T6920
Differential Revision: https://secure.phabricator.com/D13833
Summary: Ref T6920, This just removes the old voting UI from Ponder.
Test Plan: Visit a Question, no voting UI
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T6920
Differential Revision: https://secure.phabricator.com/D13827
Summary: This allows installs to essentially set a "moderator" for Ponder, who can clean up answers. Fixes T9098
Test Plan: Edit an answer I don't own.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9098
Differential Revision: https://secure.phabricator.com/D13818
Summary: These transactions were missed, add in all closed states.
Test Plan: Closed a Question, see title in Feed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13831
Summary: Ref T9096. This is a first cut at adding additional statuses, happy to add or subtract as needed... maybe even configurable? Also, the dialog doesn't seem to fire, I'll keep debugging.
Test Plan: Close and Reopen many questions. Test applicationSearch params by seeing resolved questions, all questions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9096
Differential Revision: https://secure.phabricator.com/D13826
Summary: Ref T6919, Just a basic herald adapter (new questions) for Ponder
Test Plan: Created a Personal Rule, got subscribed to new question, saw transcript.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T6919
Differential Revision: https://secure.phabricator.com/D13828
Summary: Fixes T9076. This adds the ability to search for active, archived, or all Pastes.
Test Plan: Search for active, archived, all Pastes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9076
Differential Revision: https://secure.phabricator.com/D13809
Summary: Ref T8096. Currently, we hide passing tests by default (they aren't interesting) but should also hide skipped tests by default (they aren't interesting either).
Test Plan: {F694485}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13821
Summary: Ref T9103. This application is only useful for developing Phabricator, and in general is not kept "production ready". Mark it as a prototype.
Test Plan: visit /applications/, see it marked as prototype.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9103
Differential Revision: https://secure.phabricator.com/D13822
Summary: We're using the wrong constants here, bad copy and paste job. Also add colors, icons.
Test Plan: Archive a Paste, check transactions.
Reviewers: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13817
Summary: Fixes T8657. "Auto" builds are pushed into Harbormaster by an external system (currently, `arc`) so it does not make sense to stop or resume them: Harbormaster has no way to control the external system.
Test Plan:
- Tried to restart an autobuild, got an error.
- Restarted a normal build.
- Did "Restart All" on a buildable, got restarts on non-autoplans and no restarts on autoplans.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8657
Differential Revision: https://secure.phabricator.com/D13812
Summary: Ref T8089. There's still some cleanup but none of it needs to block this.
Test Plan: Viewed `/applications/`, read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: devurandom, swisspol
Maniphest Tasks: T8089
Differential Revision: https://secure.phabricator.com/D13811
Summary:
Ref T8089. We have a lot of broken/confusing/prototype build steps that I want to hide from users when we unprototype Harbormaster.
The dialog is also just kind of unwieldy.
Organize this UI a little better and put all the sketchy junk in a "prototypes" group that you can't see unless prototypes are enabled.
This doesn't break anything (the old steps will still work fine), but should reduce user confusion.
Test Plan:
Old UI:
{F691439}
New UI (prototypes off):
{F691440}
New UI (prototypes on):
{F691441}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8089
Differential Revision: https://secure.phabricator.com/D13803
Summary: Ref T9076, adds basic plumbing for setting the state of a Paste.
Test Plan: Archive Paste, Activate Paste, New Paste
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9076
Differential Revision: https://secure.phabricator.com/D13801
