Summary:
Ref T9132. This is a quality-of-life improvement for new `application.edit` endpoints.
Instead of strictly requiring PHIDs, allow IDs or monograms. This primarily makes these endpoints easier to test and use.
Test Plan: Edited objects via API by passing IDs, PHIDs and monograms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14600
Summary:
Ref T9132. Currently, EditEngine had some branchy-`instanceof` code like this:
```
if ($object instanceof Whatever) {
do_magic();
}
if ($object instanceof SomethingElse) {
do_other_magic();
}
```
...where `Whatever` and `SomethingElse` are first-party applications like ProjectsInterface and SubscribersInterface.
This kind of code is generally bad because third-parties can't add new stuff, and it suggest something is kind of hacky in its architecture. Ideally, we would eventually get rid of almost all of this.
T9789 is a similar discussion of this for the next layer down (`TransactionEditor`) and plans to get rid of branchy-instanceofs there too.
Since I'm about to add more stuff here (for Custom Fields), split it out first so I'm not digging us any deeper than I already dug us.
Broadly, this allows third-party extensions to add fields to every EditEngine UI if they want, like we do for Policies, Subscribers, Projects and Comments today (and CustomFields soon).
Test Plan:
{F1007575}
- Observed that all fields still appear on the form and seem to work correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14599
Summary: Creates a new PhameBlogView which is more of a blog landing page with the latest posts. Management has moved to PhameManageController with a new timeline.
Test Plan:
Edit Blog, Publish, Subscribe, view posts.
{F1008400}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14608
Summary: Updates Herald to use modern methods.
Test Plan: View List, View Test Console, Run a test, View Results, View Rules, New Rule, Edit Rule, Check mobile menus.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D14596
Summary: Use modern methods in Pholio
Test Plan: View list, create mock, edit mock, view mobile menu
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D14595
Summary: Fixes T9869. This specific transaction endpoint was missing `shouldAllowPublic()`. Also modernize things a little.
Test Plan: Viewed a policy change by clicking the policy name from the transaction record on a public object while logged out.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9869
Differential Revision: https://secure.phabricator.com/D14606
Test Plan: warning is not in warnings list.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14594
Summary:
Ref T9132.
Let configurations be enabled/disabled. This doesn't do much right now.
Let configurations be marked as default entries in the application "Create" menu. This makes them show up in the application in a dropdown, so you can replace the default form and/or provide several forms.
In Maniphest, we'll do this to provide a menu something like this:
- New Bug Report
- New Feature Request
- ADVANCED TASK CREATION!!11~ (only available for Community members)
Test Plan: {F1005679}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14584
Summary:
Ref T9845. In Differential, this is not a remarkup block -- it's a mail section. `addTextSection()` has special magic behavior when handed a prebuilt section since D9375.
Swapping to `addRemarkupSection()` causes the error in T9845 and renders nothing in the comment section.
Even if it were a block of text, it would not be appropriate to add it as remarkup. This would incorrectly render comments in files like `__init__.py`, which are common on Python (the filename would render as "__init__.py"). Okay that's a bad example since it works fine but, uh, a file named `T123` would be no good or whatever.
I'll realign T9845 to clean this up and fix it more durably.
Test Plan: Sent myself some mail with inline comments, saw them in the mail.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9845
Differential Revision: https://secure.phabricator.com/D14589
Summary: Will use these more in the upcoming unbeta design of PhameBlog, likely. Also curious how this works.
Test Plan: Add an image to a blog, remove an image from a blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14587
Summary: Allows Blogs and Posts to be destroyed. Fixes T9756
Test Plan: Test `bin/remove destroy POST` and `bin/remove destroy BLOG` to great success.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9756
Differential Revision: https://secure.phabricator.com/D14586
Summary: This makes document views a little more automatic, and a little more style to the page. The Document itself remains on a pure white centered background, but footer and preceeding objects go back to the original body color. This provides a bit more depth and separation over content and definitions/comments.
Test Plan:
Tested Phriction, Diviner, Legalpad, Phame, Email Commands, HTTP Commands, with and without a footer.
{F1005853}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14582
Summary: Ref T9756, removes the ability to delete a PhamePost
Test Plan: See link removed, unpublish post, publish post, new post.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9756
Differential Revision: https://secure.phabricator.com/D14581
Summary: Caught these while re-reading.
Test Plan: Reading?
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14580
Summary: Provide a long-form description of why we require a CLA and the distinction between the individual and corporate CLAs. See Q219 and Q97.
Test Plan: Reading.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14578
Summary: Ref T9851. See T9860. This adds a missing capability to custom HeraldActions, to pave the way for removing the obsolete/undesirable WILLEDITTASK and DIDEDITTASK events.
Test Plan: See T9860 for a replacement action.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9851
Differential Revision: https://secure.phabricator.com/D14575
Summary: Column status cannot be null fix.
Test Plan: Create a new blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14574
Summary:
Fixes T9850. The `getComment()` test should be a `hasComment()` test, in order to discard empty comments.
Also backport a couple of future fixes which can get you into trouble if you reconfigure forms in awkward ways.
Test Plan: Created a new paste without a comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9850
Differential Revision: https://secure.phabricator.com/D14571
Summary: Also increase the timeout for the external process to complete the transform.
Test Plan: Careful inspection
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, cburroughs, chad, Korvin
Differential Revision: https://secure.phabricator.com/D14528
Summary: Moves to showing Legalpad previews using PHUIDocumentViewPro
Test Plan: Create a new document, edit an existing document
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14550
Summary: New icons
Test Plan: Use new icons
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14568
Summary: Ref T992. I noticed that `ManiphestTask` mail doesn't render Remarkup properly (instead, it renders Remarkup literally). I //think// this is because the code calls `addTextSection()` rather than `addRemarkupSection()`.
Test Plan: Created a new Maniphest Task and saw Remarkup in the generated self-email (inspect the email contents with `./bin/mail show-outbound`). I didn't test the other affected applications.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D14511
Summary: Reuse PHUIMarkupPreviewView in Phame for consistency, less custom code. Also, doesn't work (JS issue).
Test Plan: New Post, Edit Post, Save Post
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14552
Summary:
Fixes T9832. We currently refuse to recognize project hashtags in remarkup if they begin with a digit. This is motivated by attempting to not recognize them if they contain only digits.
I don't think we really gain anything by this. Although most `#123` in text are probably not project references, the cost of doing a lookup for them is quite small, and //some// of them are.
In cases where users use `#123` to refer to tasks in an external system, they can use a rule for that with higher precedence than this one or not give their projects conflicting hashtags.
Test Plan:
- This is well-covered by unit tests.
- Referenced a `#3u1`, per T9832.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9832
Differential Revision: https://secure.phabricator.com/D14547
Summary: These constants are incorrect.
Test Plan: Archive a blog, see feed story. Publish a blog, see another feed story.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14545
Summary: See D14536. Some time ago, linters changed to no longer receive these special/unusual file types as inputs by default.
Test Plan:
- Read new docs.
- Attempted to grep for other similar lies, although it's possible I missed some. I didn't find anything.
Reviewers: bgamari, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14543
Summary: This logic is flipped.
Test Plan:
- Before change: ran `bin/phd debug task`, saw queries to the config table every second.
- After change: ran `bin/phd debug task`, saw queries to the config table every 10 seconds.
Reviewers: chad, joshuaspence
Reviewed By: chad, joshuaspence
Differential Revision: https://secure.phabricator.com/D14542
Summary: Removes the exception, maybe there is a better way, but landing this for now. Fixes T9829
Test Plan: Test pages with and without a table of contents
Reviewers: epriestley, avivey
Subscribers: epriestley
Maniphest Tasks: T9829
Differential Revision: https://secure.phabricator.com/D14546
Summary:
Ref T9132. This adds an automatic "Comments" field, like the Subscribers/Projects/Policy fields.
The primary goals here are:
- Allow users to make comments via Conduit.
- In the future, get stackable action support.
As a side effect, this also allows you to put comments on create forms. This is a little silly but seems fine, and may be relevant on edit forms (which I'm not 100% sure how I want to handle yet). I've just hidden them by default for now.
Test Plan:
{F976036}
{F976037}
{F976038}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14515
Summary:
Ref T9132. Allows fields to be locked (shown, but not modifiable) and hidden (not shown).
In both cases, default values are still respected.
This lets you do things like create a form that generates objects with specific projects, policies, etc.
Test Plan:
- Set defaults.
- Locked and hid a bunch of fields.
- Created new objects using the resulting form.
{F975801}
{F975802}
{F975803}
{F975804}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14509
Summary: Ref T9132. Allow form configurations to include defaults (like default projects, spaces, policies, etc).
Test Plan:
Defaulted "Language" to "Rainbow", plus other adjustments:
{F975746}
{F975747}
{F975748}
{F975749}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14508
Summary:
Ref T9132. This just makes edited forms do //something//, albeit not anything very useful yet.
You can now edit a form and:
- Retitle it;
- add a preamble (instructions on top of the form); and
- reorder the form's fields.
Test Plan:
{F974632}
{F974633}
{F974634}
{F974635}
{F974636}
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14503
Summary: Fixes T9806. See some discussion there.
Test Plan:
- Edited a comment with `T12` in it.
- Before change: exception.
- After change: no exception.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9806
Differential Revision: https://secure.phabricator.com/D14539
Summary: Oops, I missed this -- when properties are `protected`, Lisk assumes they database properties which should be stored and read from the database. To make Lisk ignore a property, make it `private`.
Test Plan:
Should fix this:
{F990757}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14538
Summary:
Addresses T9814. Adds SVG files to Celerity maps. Adds a mask-icon.svg file that
I made by pulling the existing favicon into Illustrator and running trace on it.
This hardcodes the header color from the default theme, and doesn't pay attention
to customizations of the header.
Test Plan: I pinned the tab in Safari.
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: chad, Korvin
Maniphest Tasks: T9814
Differential Revision: https://secure.phabricator.com/D14527
Summary: Removes "delete" and uses "archive/activate" instead for Phame Blogs. Ref T9756
Test Plan: Archive a blog, see in search, activate blog, see in other search.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, Korvin
Maniphest Tasks: T9756
Differential Revision: https://secure.phabricator.com/D14465
Summary: Ref T6049, Add Phurl URL create capability
Test Plan:
- Change {nav Home > Applications > Phurl > Configure} to allow no one to create Phurl URLs
- Attempt {nav Phurl > Shorten URL}. Should not be able to create a Phurl.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D14510
Test Plan:
I didn't put any skill points in spelling since I need
combat skills to survive in a nuclear wasteland, but spell check says
this is better.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14522
Summary: Ref T6049, Phurl object view should display Phurl name or Phurl long url as header.
Test Plan:
- Create Phurl with no name. Header should show long url as header.
- Add name to Phurl. Header should be new Phurl name.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D14502
Summary:
Fixes T9808.
An instance imported a very large repository, generating approximately 4 million tasks over the course of a few days. A week later, these tasks started expiring and became candidates for garbage collection.
The GC works by deleting 100 rows at at time over and over again. It finds the rows it's going to delete by querying for old rows.
Currently, this query generates a `WHERE dateCreated < X ORDER BY id DESC` query. This query can not efficiently execute using a single key, because it relies on `dateCreated` order to find the rows, then on `id` order to sort them. With a table with 4M rows, this is slow.
This would still be OK, except that the query has to execute a lot of times since it only deletes 100 rows each time. Particularly, it needs to execute a total of ~40K times.
Instead, generate `WHERE dateCreated < X ORDER BY dateCreated DESC, id DESC`. This should have the same effect in general and the GC definitely doesn't care about the difference, but it should be more efficient at large scales.
Test Plan:
I had to `TRUNCATE` the problem table so I don't have a perfect repro to completely convincingly test this anymore. Both queries behave fine at small scales, which is why we haven't seen this before.
I was able to run the newer query in production before I nuked the table and have it complete in a reasonable amount of time, while the old query hung longer than I wanted to wait (several minutes?). The query plan for the new query was also a good one, while the query plan for the old query was terrible.
I loaded the daemon console and ran `bin/garbage collect --collector worker.tasks --trace`. I verified the queries looked reasonable and produced reasonable results in production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9808
Differential Revision: https://secure.phabricator.com/D14505
Summary: Ref T6049, remarkup links to use short URLs and make commenting on Phurl's actually work
Test Plan:
- Create Phurl `U123`
- Comment on that Phurl `((123))`
Comment should link to `/u/123`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D14477
Summary:
Fixes T9787. Currently, file PHID extraction logic happens very early, before we normalize/merge/etc the transactions.
In D14390, I changed how the CONTENT transaction works: before, callers would pass in a file PHID. Afterward, they just pass in the content.
Passing in the content is generaly easier and feels more correct, but inadvertenly broke PHID extraction because converting the content into a file PHID now happened after we extracted the PHID. So we'd extract the entire text of the paste as a "file PHID", which wouldn't work.
Instead, extract file PHIDs later. This impacts a couple of other applications (Conpherence, Pholio) which receive an object or have an unusual file-oriented transaction.
Test Plan:
- Made a new paste, verified the raw file attached to it properly.
- Made and updated a mock, verified all the files attached properly.
- Updated a Conpherence room image, verified the files attached properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9787
Differential Revision: https://secure.phabricator.com/D14494
Summary:
Ref T9787. To fix this, I want to change how file PHIDs are extracted slightly: specifically, I'm going to extract them later in the editing process.
Before doing this, clean up a couple of bad implementations:
- Owners extracts its description as a file PHID. This is an error.
- Extract the description as a remarkup block instead.
- Add an edge table so stuff like file attachment works properly.
- Differential has a no-op extract method. This is presumably just a copy/paste issue from long ago.
Test Plan:
- Edited a revision in Differential.
- Dropped a file into the description of an Owners package.
- Before change: this did not attach the file.
- After change: the file now attaches properly and shows up as "Attached" in the file details.
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Subscribers: joshuaspence
Maniphest Tasks: T9787
Differential Revision: https://secure.phabricator.com/D14493
Summary: Fixes T9757.
Test Plan: Created a Herald rule and then subscribed to it with a different account.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9757
Differential Revision: https://secure.phabricator.com/D14468
Summary: Move some `PhabricatorPolicyRule` implementations to a subdirectory of the parent application.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14478
Summary:
Fixes T9799. Currently, if you can't see an application like Paste, we fatal when trying to generate a result for `conduit.query`, because the new EditEngine-based `paste.edit` method doesn't "know" that it's a "Paste" method.
Straighten this out, and use policies and queries a little more correctly/consistently.
Test Plan:
- Called `conduit.query` as a user who does not have permission to use Paste.
- Before change: fatal.
- After change: results, excluding "paste.*" methods.
Reviewers: chad
Reviewed By: chad
Subscribers: cburroughs
Maniphest Tasks: T9799
Differential Revision: https://secure.phabricator.com/D14492
Summary:
Fixes T9798. That task has good repro instructions.
In sub-views, we don't link the "History" icon correctly -- we only link it to `history/README` instead of `history/path/to/README`. Add the full path.
Also canonicalize the paths in a slightly prettier and more consistenty way.
Test Plan: Viewed root and non-root browse tables, saw links show up properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9798
Differential Revision: https://secure.phabricator.com/D14491
Summary: As suggested in D14461.
Test Plan: Used `./bin/remove destroy` on an Almanac service with properties attached, saw entries removed from the `phabricator_almanac.almanac_property` table.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14490
Summary: Ref T9762. Currently it is not possible to destroy an Alamanac device because any associate bindings cannot be destroyed.
Test Plan: Destroyed an Almanac device.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9762
Differential Revision: https://secure.phabricator.com/D14461
Summary: Some linter messages, such as those produced by `ArcanistPHPCompatibilityXHPASTLinterRule`, contain backticks but are currently rendered as Remarkup literals. I think that it is generally desirable to allow lint messages to be rendered as Remarkup, although we should ideally have a way to render Remarkup for use on the command line (I actually think that this already exists, but I don't think that `arc lint` does this when rendering linter messages).
Test Plan: Resubmitted D14481 to my dev install and saw Remarkuped lint messages.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14485
Summary: Rename the XHPAST database from `{$NAMESPACE}_xpastview` to `{$NAMESPACE}_xhpast`.
Test Plan: Ran `./bin/storage --namespace test upgrade --no-quickstart`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14442
Summary: This logic is inverted. Re-vert it.
Test Plan: Write and publish a new post, see publish time.
Reviewers: epriestley, joshuaspence
Reviewed By: joshuaspence
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14464
Summary: I think `HeraldRule`s are the only objects which have monograms but are not accesible via `/{$monogram}`. This diff changes the `/herald/rule/{$id}` URI to `/{$monogram}`.
Test Plan: Clicked a bunch of links in Herald to ensure there were no dead links.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14469
Summary:
Current JIRA integration is quite noisy in terms of email, and makes users hunt and peck for the related revisions.
Teach it to create an Issue Link on the JIRA side, and allow to disable commenting.
Test Plan: comment on revision in each of the 4 settings, check JIRA end for expected result.
Reviewers: btrahan, eMxyzptlk, epriestley, #blessed_reviewers, avivey
Reviewed By: epriestley, #blessed_reviewers
Subscribers: avivey, vhbit, jra3, eMxyzptlk, frenchs, aik099, svemir, rmuslimov, cpa199, waynea, epriestley, Korvin, hach-que
Projects: #doorkeeper
Maniphest Tasks: T5422
Differential Revision: https://secure.phabricator.com/D9858
Summary: Fixes T9772. We now need an EditEngineConfiguration to do interesting things with EditEngine, but this public API wasn't properly making sure we have one.
Test Plan: Called `conduit.query` from web console. Fatal prior to patch; success afterward.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9772
Differential Revision: https://secure.phabricator.com/D14475
Summary: Ref T7053. Remove the `envHash` and `envInfo` fields, which are no longer used now that the daemons restart automagically. Depends on D14458.
Test Plan: Saw no more setup issues.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: tycho.tatitscheff, epriestley
Maniphest Tasks: T7053
Differential Revision: https://secure.phabricator.com/D14446
Summary: Fixes T7053. Depends on D14452.
Test Plan:
Created a custom daemon which dumps out the config hash (by querying `PhabricatorEnv::calculateEnvironmentHash()`). Ran this daemon with `./bin/phd debug PhabricatorDebugDaemon` and saw the config hash update within 30 seconds.
{P1886}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T7053
Differential Revision: https://secure.phabricator.com/D14458
Summary: Right now we're attaching the body of every Phame post on each comment, at least restrict it to newly created objects only.
Test Plan: Write a new post, get full email, leave a comment, get less email.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14459
Summary: We currently orphan posts when you delete a blog. Fixes some visibility and permission errors when that happens. Also... should allow you to archive posts.
Test Plan: Delete a blog, visit a post I made, still can see it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14457
Summary:
Ref T9132. This diff doesn't do anything interesting, it just lays the groundwork for more interesting future diffs.
Broadly, the idea here is to let you create multiple views of each edit form. For example, we might create several different "Create Task" forms, like:
- "New Bug Report"
- "New Feature Request"
These would be views of the "Create Task" form, but with various adjustments:
- A form might have additional instructions ("how to file a good bug report").
- A form might have prefilled values for some fields (like particular projects, subscribers, or policies).
- A form might have some fields locked (so they can not be edited) or hidden.
- A form might have a different field order.
- A form might have a limited visibility policy, so only some users can access it.
This diff adds a new storage object (`EditEngineConfiguration`) to keep track of all those customizations and represent "a form which has been configured to look and work a certain way".
This doesn't let these configurations do anything useful/interesting, and you can't access them directly yet, it's just all the boring plumbing to enable more interesting behavior in the future.
Test Plan:
ApplicationEditor forms now let you manage available forms and edit the current form:
{F959025}
There's a new (bare bones) list of all available engines:
{F959030}
And if you jump into an engine, you can see all the forms for it:
{F959038}
The actual form configurations have standard detail/edit pages. The edit pages are themselves driven by ApplicationEditor, of course, so you can edit the form for editing forms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14453
Summary: Via HackerOne. See D14025. I missed this comparison when making the original change.
Test Plan:
- Used `cat mail.txt | scripts/mail/mail_handler.php --process-duplicates` to pipe mail in a whole lot of times.
- Tried bad hashes, saw rejections.
- Tried good hash, saw mail accepted.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14455
Summary: Adds mail reply support to Phame Posts.
Test Plan: Comment on a post, get mail.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9746
Differential Revision: https://secure.phabricator.com/D14454
Summary: When accessing an invalid URL on the short Phurl domain, users should see informative message
Test Plan: Open URL in the previously configured Phurl short domain such as `https://www.zz.us` and see dialog with message. Open `https://www.zz.us/u/123` for a valid `U123` Phurl and access destination URL.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14450
Summary: Adds commenting to Phame Posts, also testing a new "document comment style". Unsure about it but Phame is a prototype so good place to explore.
Test Plan: Leave some comments, see some comments, test show/hide.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9746
Differential Revision: https://secure.phabricator.com/D14451
Summary:
This diff adds a new mode argument to the Diffusion Conduit API with two options:
- "overwrite": the default, maintains the current behavior of deleting all coverage
in the specified branch before uploading the new coverage
- "update": does not delete old coverage, but will overwrite previous
coverage information if it's for the same file and commit
`DiffusionRequest::loadCoverage` already loads a file's coverage from the
latest available commit, so uploading coverage for different files in different
commits with "update" will result in seeing the latest uploaded coverage in
Diffusion.
Test Plan: manual local verification
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14428
Summary: Crumbies
Test Plan: View post, see blog link, click on crumb, see blog
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14449
Summary: Cleaning up house, may revisit in a v2. Removes ability to set Disqus or Facebook comments as comment system on Phame Posts.
Test Plan: Create blog, create post, edit blog, view live pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: btrahan, Korvin
Maniphest Tasks: T9746
Differential Revision: https://secure.phabricator.com/D14448
Summary: Ref T8995, config option for Phurl short domain to share shortened URL's
Test Plan:
- Configure Phurl short domain to something like "zz.us"
- Navigate to `zz.us`; get 404
- Navigate to `zz.us/u/3` or `zz.us/u/alias` where `U3` is an existing Phurl; redirect to correct destination
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8995
Differential Revision: https://secure.phabricator.com/D14447
Summary: Drops Join Policy, uses Edit Policy where needed. Allows anyone with Blog Edit permissions to post and edit any post on that blog. Fixes T5371
Test Plan: Draft Post as chad, see post, log in with notchad, edit that post and publish it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5371
Differential Revision: https://secure.phabricator.com/D14444
Summary: Currently, a bunch of developers are using #xhpast for writing custom linter rules. As such, we end up with a fair few `XHPASTSyntaxErrorException` in our PHP error logs. I think that throwing an exception is not quite correct in this case because it is somewhat expected that invalid PHP may be entered. Instead, catch the exception and show the user a helpful message.
Test Plan: This doesn't quite work yet... the stream and tree views render as blank but the exceptions still propogate to the error logs. Mostly, I'm not sure how the exception should be rendered for display.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14028
Summary: Add some mailkeys, allow feed stories to be published.
Test Plan: New Blog, Edit Blog
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14434
Summary:
Fixes T9735. I changed how the TYPE_LANGUAGE transction works a little but that accidentally tripped an error condition in `paste.create`.
- Don't bail on no-effect transactions to `paste.create` (like not setting a language).
- When a transaction type has no tailored UI message, make it easier to figure out which transaction is problematic.
Test Plan: Ran `arc paste ...` locally. Got an error before the patch, clean paste creation afterward.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9735
Differential Revision: https://secure.phabricator.com/D14440
Summary: Fixes T9051, adds ability to edit blogs and posts and manually add subscribers. Also fixed bug granting tokens to posts.
Test Plan: Create a new blog, subcribe chad and notchad. Write a post, both are notified. Award token for hard work.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9051
Differential Revision: https://secure.phabricator.com/D14432
Summary:
Fixes T9732. We currently tokenize strings (like user realnames) in the default non-unicode mode, which can cause patterns like `\s` to work incorrectly.
Use `/u` to use unicode-aware tokenization instead.
Test Plan:
The behavior of "\s" depends upon environmental settings like LC_ALL.
With LC_ALL set to "C", `\xA0` is not considered a whitespace character.
With LC_ALL set to "en_US", it is:
```
$ php -r 'setlocale(LC_ALL, "C"); echo count(preg_split("/\s/", "\xE5\xBF\xA0")) . "\n";'
1
$ php -r 'setlocale(LC_ALL, "en_US"); echo count(preg_split("/\s/", "\xE5\xBF\xA0")) . "\n";'
2
```
To reproduce the original issue, I added an explicit:
```
setlocale(LC_ALL, "en_US");
```
...call before the `preg_split()` call. This caused "忠" to be improperly split.
I then added "/u", and observed proper tokenization.
Reviewers: chad
Reviewed By: chad
Subscribers: qiu8310
Maniphest Tasks: T9732
Differential Revision: https://secure.phabricator.com/D14441
Summary: Larger (open) installs may want to restrict Blog to formal entities, like with Phriction.
Test Plan: Set policy to administrators, have notchad try to create a blog. See error.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14433
Summary: fix T9718.
Test Plan: view project page when maniphest is and isn't. Look for Workboards.
Reviewers: #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: epriestley
Maniphest Tasks: T9718
Differential Revision: https://secure.phabricator.com/D14438
Summary: Sends out the body of the post along with the details.
Test Plan: Write a new post, see body in email.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14431
Summary: Ref T7951, Starting the Calendar user guide
Test Plan: Go to {nav Diviner > Phabricator User Docs > Calendar User Guide}, read about how fabulous the Calendar application is.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T7951
Differential Revision: https://secure.phabricator.com/D13496
Summary: Adds ability to set visibility when authoring a Post. New default is "Visible". If you write a post and save it as a Draft, and later click publish, a feed story and mail will go out.
Test Plan: Write a new Post, see feed story and get email. Write a new Draft, get nothing. Click Publish, see story and email.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14429
Summary: Ref T9722, Add Phurl Remarkup as `((id))` or `((alias))`
Test Plan: Add a comment to any object as `((id))` or `((alias))`. Make sure comment renders as a link.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9722
Differential Revision: https://secure.phabricator.com/D14427
Summary: Allows feed stories and mail for new Phame Posts.
Test Plan: Write Post, Get Mail
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14426
Summary: Ref T9546. I only got the title to always show the blog title (better than nothing) -- showing the post title properly isn't trivial and is more work than I want to do right now.
Test Plan:
- Description now has remarkup.
- Title now shows blog title (better than nothing).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9546
Differential Revision: https://secure.phabricator.com/D14423
Summary: Adds Remarkup rules and CSS, cleans up some spacing a color. Ref T9546
Test Plan: Review a blog post list, and a blog
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9546
Differential Revision: https://secure.phabricator.com/D14421
Summary: Updates Phame for new modern methods.
Test Plan: New blog, edit blog, new post, edit post, publish post.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14419
Summary: Updates "View Post" to use PHUIDocumentViewPro, updates calls to `newPage` and other minor modernizations. Edit Page updated to show proper document display as well. Ref T9545
Test Plan:
Write a blog post, edit it.
{F945897}
{F945896}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9545
Differential Revision: https://secure.phabricator.com/D14415
Summary: Ref T8992, Cleaning up and clarifying xaction titles for Phurl creation/updating.
Test Plan: Create a Phurl, update information, make sure xaction in the timeline makes sense.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8992
Differential Revision: https://secure.phabricator.com/D14414
Summary: Ref T8992, Make it impossible to save an empty string alias for a Phurl.
Test Plan:
- Create two Phurl's with non-empty aliases
- Delete aliases for both Phurl's
- Previously, this wouldn't allow to save the second Phurl because of a duplicate alias. Current diff should save empty alias as `null`, not empty string.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8992
Differential Revision: https://secure.phabricator.com/D14413
Summary: This is a bit too grey, and doesn't match our theme well (see sequence navs)
Test Plan: Remarkup reference article, sequence navs
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14408
Summary: Ref T9132. This allows you to prefill EditEngine forms with stuff like `?subscribers=epriestley`, and we'll figure out what you mean.
Test Plan:
- Did `/?subscribers=...` with various values (good, bad, mis-capitalized).
- Did `/?projects=...` with various values (good, bad, mis-capitalized).
- Reviewed documentation.
- Reviewed {nav Config > HTTP Parameter Types}.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14404
Summary:
Ref T9132. We have several places in the code that sometimes need to parse complex types. For example, we accept all of these in ApplicationSearch and now in ApplicationEditor:
> /?subscribers=cat,dog
> /?subscribers=PHID-USER-1111
> /?subscribers[]=cat&subscribers[]=PHID-USER-2222
..etc. The logic to parse this stuff isn't too complex, but it isn't trivial either.
Right now it lives in some odd places. Notably, `PhabricatorApplicationSearchEngine` has some weird helper methods for this stuff. Rather than give `EditEngine` the same set of weird helper methods, pull all this stuff out into "HTTPParameterTypes".
Future diffs will add "Projects" and "Users" types where all the custom parsing/lookup logic can live. Then eventually the Search stuff can reuse these.
Generally, this just breaks the code up into smaller pieces that have more specific responsibilities.
Test Plan: {F944142}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14402
Summary: Ref T9132. This just moves code around, breaks it up into some smaller chunks, tries to reduce duplication, and adds a touch of documentation.
Test Plan: Created and edited pastes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14398
Summary:
Fix T9662.
Record who initiated the build, and allow this information as a parameter.
In this implementation, a 're-run' keeps the original initiator, which we maybe not desired?
Test Plan:
Make a HTTP step with initiator.phid, trigger manually, via HM, via ./bin/harbormaster build.
Look at requests made.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9662
Differential Revision: https://secure.phabricator.com/D14380
Summary: Ref T8992, Validate alias text field length.
Test Plan: Create Phurl with alias of more than 64 characters. Get error. Reduce length of alias to successfully save Phurl.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8992
Differential Revision: https://secure.phabricator.com/D14403
Summary:
Exposes the serve-over-http and serve-over-ssh options for a repository
to the `bin/repository edit` endpoint.
Test Plan: Ran `bin/repository` with the new options over several hundred repos
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, chasemp, 20after4, epriestley
Differential Revision: https://secure.phabricator.com/D14250
Summary: Ref T8992, Phurl aliases must be unique. Otherwise throw an error.
Test Plan: Create two Phurl's both with alias 'asdf'. When saving second Phurl, form should show an error about the duplicate alias.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8992
Differential Revision: https://secure.phabricator.com/D14401
Summary: Closes T9703. This page has become redundant 10 months ago, at D10988.
Test Plan: Look at /settings page, don't see word "Certificate".
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: Korvin
Maniphest Tasks: T9703
Differential Revision: https://secure.phabricator.com/D14400
Summary: Ref T8992, Add an alias to Phurl URL's that can be used to redirect to link.
Test Plan: Add an alias to Phurl object, and navigate to `local.install.com/u/<newalias>`. This should redirect to the Phurl's URL.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, Korvin
Maniphest Tasks: T8992
Differential Revision: https://secure.phabricator.com/D14395
Summary: Use in MailCommands and HTTP Parameters
Test Plan: Tested MailCommands in Paste, HTTP Parameters in Paste, Legalpad, Diviner. Mobile and Desktop breakpoints.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14397
Summary:
Ref T9690. The "meta viewport" tag got dropped by accident because of the sort of weird logic on the old flow.
Make the default device-ready, then just turn it off for the tiny number of non-device pages.
Test Plan:
- Verified meta viewport tag appears on normal pages again.
- Verified it doesn't show up on non-mobile pages like Maniphest Reports.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D14396
Summary:
Ref T5873. Ref T9132. This is really rough and feels pretty flimsy at the edges (missing validation, generality, modularity, clean error handling, etc) but gets us most of the way toward generating plausible "whatever.edit" Conduit API methods from EditEngines.
These methods are full-power methods which can do everything the edit form can, automatically support the same range of operations, and update when new fields are added.
Test Plan:
- Used new `paste.edit` to create a new Paste.
- Used new `paste.edit` to update an existing paste.
- Applied a variety of different transactions.
- Hit a reasonable set of errors.
{F941144}
{F941145}
{F941146}
{F941147}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5873, T9132
Differential Revision: https://secure.phabricator.com/D14393
Summary:
Ref T9132. Although forms do generally support prefilling right now, you have to guess how to do it.
Provide an explicit action showing you which values are supported and how to prefill them. This is generated automatically when an application switches to ApplicationEditor.
Test Plan:
{F939804}
{F939805}
{F939806}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14392
Summary:
Ref T9132. Ref T4768. This is a rough v0 of ApplicationEditor, which replaces the edit workflow in Paste.
This mostly looks and works like ApplicationSearch, and is heavily modeled on it.
Roughly, we define a set of editable fields and the ApplicationEditor stuff builds everything else.
This has no functional changes, except:
- I removed "Fork Paste" since I don't think it's particularly useful now that pastes are editable. We could restore it if users miss it.
- Subscribers are now editable.
- Form field order is a little goofy (this will be fixed in a future diff).
- Subscribers and projects are now race-resistant.
The race-resistance works like this: instead of submitting just the new value ("subscribers=apple, dog") and doing a set operation ("set subscribers = apple, dog"), we submit the old and new values ("original=apple" + "new=apple, dog") then apply the user's changes as an add + remove ("add=dog", "remove=<none>"). This means that two users who do "Edit Paste" at around the same time and each add or remove a couple of subscribers won't overwrite each other, unless they actually add or remove the exact same subscribers (in which case their edits legitimately conflict). Previously, the last user to save would win, and whatever was in their field would overwrite the prior state, potentially losing the first user's edits.
Test Plan:
- Created pastes.
- Created pastes via API.
- Edited pastes.
- Edited every field.
- Opened a paste in two windows and did project/subscriber edits in each, saved in arbitrary order, had edits respected.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4768, T9132
Differential Revision: https://secure.phabricator.com/D14390
Summary: Ref T9690. I wanted to do an example of how to do these but it looks like most of them are trivial (no callsites) and the rest are a little tricky (weird interaction with frames, or in Releeph).
Test Plan:
- Used `grep` to look for callsites.
- Hit all applications locally, everything worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D14385
Summary:
Fixes T5752. This obsoletes a bunch of old patterns and I'll follow up on those with a big "go do a bunch of mechanical code changes" task. Major goals are:
- Don't load named queries multiple times on search pages.
- Don't require extra code to get standard navigation right on mobile.
- Reduce the amount of boilerplate in ListControllers.
- Reduce the amount of boilerplate around navigation/menus in all controllers.
Specifically, here's what this does:
- The StandardPage is now a smarter/more structured object with `setNavigation()` and `setCrumbs()` methods. More rendering decisions are delayed until the last possible moment.
- It uses this to automatically add crumb actions to the application menu.
- It uses this to automatically reuse one SearchEngine instead of running queries multiple times.
- The new preferred way to build responses is `$this->newPage()` (like `$this->newDialog()`), which has structured methods for adding stuff (`setTitle()`, etc).
- SearchEngine exposes a new convenience method so you don't have to do all the controller delegation stuff.
- Building menus is generally simpler.
Test Plan:
- Tested paste list, view, edit, comment, raw controllers for functionality, mobile menu, crumbs, navigation menu.
- Edited saved queries.
- Tested Differential, Maniphest (no changes).
- Verified the paste pages don't run any duplicate NamedQuery queries.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5752
Differential Revision: https://secure.phabricator.com/D14382
Summary: These fonts are functionally very similar, but in diagnosing a problem with mobile Safari/Chrome, it turned out that our use of "bold" with the "normal" font build created a "semibold" look when on desktop and a "normal" look on mobile. The "semibold" feel is more important, so finding a lighter "bold" font was the impetus for this font switch. As it turns out **Aleo** is built by the same author as **Lato** (our other font) and is intended as it's companion. So stylistically, this is the more correct font.
Test Plan:
Test Phriction, Legalpad, Diviner, Desktop and Mobile
{F938013}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14391
Summary: Closes T9691, Validate URL on Phurl objects for using valid protocols.
Test Plan: Create or edit URL. Change URL to "asdf" and observe error. Change back to "http://google.com" and observe no error.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9691
Differential Revision: https://secure.phabricator.com/D14389
Summary: Adds some basic style to new !!Remarkup Highlighter!! Ref T5560
Test Plan: Wait for next diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5560
Differential Revision: https://secure.phabricator.com/D14383
Summary: Ref T8989, Phurl URL should always show an info banner if the URL isn't valid
Test Plan: Phurl objects with URL "google.com" should show an error banner, but objects with URL "http://google.com" should not show banner.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8989
Differential Revision: https://secure.phabricator.com/D14386
Summary: Ref T8989, Phurl "Visit URL" should now route to an access controller that decides if the URL is valid whether to open it, or redirect back to Phurl object. New route is `local.install.com/u/1` to open link.
Test Plan:
- open Phurl object with invalid URL, "Visit URL" link should redirect back to object
- open Phurl object with valid URL, "Visit URL" link should open the link
- open `local.install.com/u/1` for `U1` with valid URL should open the link
- open `local.install.com/u/1` for `U1` with invalid URL should redirect to `local.install.com/U1`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, Korvin
Maniphest Tasks: T8989
Differential Revision: https://secure.phabricator.com/D14381
Summary: Ref T8989, Add a "Visit URL" link to Phurl items and make it actionable if the URI has a valid protocol.
Test Plan:
- Create a Phurl object with a URI of "google.com".
- "Visit URL" action in action view should be greyed out.
- Edit object to have URI "http://google.com" and save. "Visit URL" link should be available and should redirect to the intended URL.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin
Maniphest Tasks: T8989
Differential Revision: https://secure.phabricator.com/D14379
Summary: Rolls out PHUIDocumentViewPro to Legalpad. Minor tweaks to provide space around Preamble and Signature blocks. Otherwise, straight forward.
Test Plan:
Build a new document with and without Preamble, sign document.
{F933386}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14377
Summary:
This implements `PHUIDocumentViewPro` which should move to be the base for all documents (Phame, Phriction, Legalpad, Diviner). Overall this feels really good to me, but I'd like to roll it out into Diviner specifically first to work through the issues and then move into other apps and drop `PHUIDocumentView` once everything is converted. Some features are:
- White Background, no border on page
- Table of Contents is move to hidden menu (more space for documentation)
- Property List sits under the document
Some design decisions above are in anticipation of Phriction v3 and Unbeta Phame, specifically commenting and maybe some cool new Remarkup text layout options for Phame.
Test Plan:
Went through tons of pages on Diviner on Desktop, Tablet, Mobile. Bounce back to Phriction to make sure DocumentView CSS changes actually look better there.
{F930518}
{F930519}
{F930520}
{F930521}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: tycho.tatitscheff, joshuaspence, Korvin
Differential Revision: https://secure.phabricator.com/D14374
Summary: We haven't seen any issues here, remove the table and schema spec.
Test Plan: Not yet tested.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14067
Summary: These should be fine to land whenever.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14066
Summary: Sets the `$can_edit` value correctly (previously it was hardcoded to `true`).
Test Plan: Went to http://phabricator.local/harbormaster/step/view/1/ and saw "Edit Step" disabled.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14373
Summary: See IRC. A user had a database set to 8 hours ahead of their web host. Try to catch and warn about these issues.
Test Plan: Artificially adjusted skew, saw setup warning.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14371
Summary:
Fixes T9669. Two issues:
- We were using `repositoryPHIDs` instead of `blueprintPHIDs` for the list of allowed blueprints. Use the correct value.
- We weren't enforcing `allowedBlueprintPHIDs` fully correctly. We //did// require an authorization, so the net effect was correct in nearly all cases, but we could have selected from too large a pool in the case where the application itself was doing the authorization (e.g., from the command line).
Test Plan: Ran a build through Drydock/Harbormaster locally.
Reviewers: chad, tycho.tatitscheff
Reviewed By: chad, tycho.tatitscheff
Subscribers: tycho.tatitscheff
Maniphest Tasks: T9669
Differential Revision: https://secure.phabricator.com/D14368
Summary: We are greedily hoarding this for ourselves, when we could enrich the world.
Test Plan: Used `{icon cog spin}`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14369
Summary:
Fixes T9672. This was never turned into a custom field, for no particular reason. Convert it into one.
This is substantially similar to the existing "Apply Patch" field, which does the same thing (only shows a command).
We might rethink or remove this eventually (e.g., in a post-"Land Revision" world) but this makes it easier, at the very least.
Test Plan:
- Viewed a non-accepted revision (no hint).
- Viewed an accepted revision from a raw diff source (no hint).
- Viewed an accepted revision from Git (`arc land` hint).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9672
Differential Revision: https://secure.phabricator.com/D14367
Summary: Fixes T9674. This was wrong to start with (URI is `/edit/X/`, not `/X/edit/`) but we have a new view page anyway.
Test Plan:
- Visited an exmaple URI in my browser.
- Followed a build step link from "Authorized By: ..." in Drydock.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9674
Differential Revision: https://secure.phabricator.com/D14366
Test Plan: chain another call after this
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14364
Summary: This makes PHUIPropertyList display wider when an ActionList isn't present.
Test Plan: Review Diff Details in a Diff. Test mobile and desktop layouts.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13568
Summary: Better formatting for object lists when in a dialog (like subscribers).
Test Plan:
Test a subscription list.
{F911522}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14353
Summary: This is //hilarious//.
Test Plan: Test icon on local install.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14351
Summary: I didn't test the positive version of this -- the constant has value `2` but when we read it from the database it's `"2"` or whatever. Just do this for now and maybe someday we'll use strings.
Test Plan: will do production things
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14352
Summary:
Ref T182. Make the disabled state of the button more accurately reflect whether clicking it will work.
Don't allow "land" to proceed unless the revision is accepted.
Test Plan: Saw button in disabled state, clicked it, got "only accepted revisions" message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14350
Summary:
Ref T182. Ref T9252.
- Adds a "Test" repository operation that just runs `git status` to see if things work.
- Adds a button for it in Edit Repository.
- Shows operation status on the operation detail view to make this workflow work a little better.
- Adds a lot of words. Words words words words.
Test Plan:
- Tested repository operation.
- Read words.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182, T9252
Differential Revision: https://secure.phabricator.com/D14349
Summary:
Ref T182. When viewing a revision, if there are several error operations and then a success operation, we currently show the last error. This is misleading.
Instead, don't show anything if there's a success (this may require tuning eventually if you can land multiple times onto different branches or whatever, but should be reasonable for now).
Also make the table a little nicer, particularly for merge failure output.
Test Plan: {F910385}
Reviewers: chad, Mnkras
Reviewed By: Mnkras
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14348
Summary: Without this change PHP throws because idx() is passed null as the property is not intialzied
Test Plan: arc unit --everything
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14345
Summary:
Ref T182. This command should never actually generate a commit because `--squash` prevents that, but `git` seems to sometimes hit a check for username/email configuration (maybe when merging a non-fastforward?).
Give it some dummy values to placate it. This command shouldn't commit anything so these values should never actually be used.
Test Plan: Landed rGITTESTd8c8643cb02bbe60048c6c206afc2940c760a77e.
Reviewers: chad, Mnkras
Reviewed By: Mnkras
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14347
Summary:
Ref T182. I lifted this logic out of `arc`, but the context is a little different there, and this option is too strict in "Land Revision".
Specifically, it prevents `git` from merging unless the merge is //strictly// a fast-foward, even with `--squash`. That means revisions can't merge unless they're rebased on the current `master`, even if they have no conflicts.
(This whole process will probably need additional refinement, but the behavior without this flag is more reasonable overall than the behavior with it for now.)
Test Plan: Will land stuff in production~~
Reviewers: chad, Mnkras
Reviewed By: Mnkras
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14346
Summary:
Ref T182. We just show "an error happened" right now. Improve this behavior.
This error handling chain is a bit ad-hoc for now but we can formalize it as we hit other cases.
Test Plan:
{F910247}
{F910248}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14343
Summary:
Ref T182. Couple of minor improvements here:
- Show the Drydock lease when viewing a Repository Operation detail screen. This just makes it easier to jump around between relevant objects.
- When tasks are waiting for a lease, awaken them when it breaks or is released, not just when it is acquired. This makes the queue move forward faster when errors occur.
Test Plan:
- Viewed a repository operation and saw a link to the lease.
- Did a bad land (intentional merge problem) and got an error in about ~3 seconds instead of ~17.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14341
Summary:
Ref T182.
- We just show the oldest operation right now, but we usually care about the oldest non-failure.
- Only query for actual land operations when rendering the revision operations dialog (maybe eventually we'll show more stuff?).
- For now, prevent multiple lands / repeated lands or queueing up lands while other lands are happening.
Test Plan: Landed a revision. Tried to land it more / again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14338
Summary:
Ref T182. Currently, the "RepositoryLand" operation is responsible for performing merges when landing a revision.
However, we'd like to be able to perform these merges in a larger set of cases in the future. For example:
- After Releeph is revamped, when someone says "I want to merge bug fix X into stable branch Y", it would probably be nice to make that a Buildable and let tests run against it without requring that it actually be pushed anywhere.
- Same deal if we want a merge-from-Diffusion or cherry-pick-from-Diffusion operation.
- Similar deal if we want a "random web UI edits from Diffusion".
Move the merging part into WorkingCopy so more applications can share/use it in the future.
A big chunk of this is me making stuff up for now (the ol' undocumented dictionary full of arbitrary magic keys), but I anticipate formalizing it as we move along.
Test Plan: Pushed rGITTEST0d58eef3ce0fa5a10732d2efefc56aec126bc219 up from my local install via "Land Revision".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14337
Summary:
Ref T9252. Right now, we have very strict limits on Drydock: one lease per host, and one working copy per working copy blueprint.
These are silly and getting in the way of using "Land Revision" more widely, since we need at least one working copy for each landable repository.
For now, just remove the host limit and put a simple limit on working copies. This might need to be fancier some day (e.g., limit working copies per-host) but it is generally reasonable for the use cases of today.
Also add a `--background` flag to make testing a little easier.
(Limits are also less important nowadays than they were in the past, because pools expand slowly now and we seem to have stamped out all the "runaway train" bugs where allocators go crazy and allocate a million things.)
Test Plan:
- With a limit of 5, ran 10 concurrent builds and saw them finish after allocating 5 total resources.
- Removed limit, raised taskmaster concurrency to 128, ran thousands of builds in blocks of 128 or 256.
- Saw Drydock gradually expand the pool, allocating a few more working copies at first and a lot of working copies later.
- Got ~256 builds in ~140 seconds, which isn't a breakneck pace or anything but isn't too bad.
- This stuff seems to be mostly bottlenecked on `sbuild` throttling inbound SSH connections. I haven't tweaked it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14334
Summary:
Fixes T9519. Right now, build steps go straight from the build to the edit screen.
This means that there's no way to see their edit history or review details without edit permission. In particular, this makes it a bit harder to catch the Drydock Blueprint authorization warnings from T9519.
- Add a standard view screen.
- Add a little warning callout to blueprint authorizations.
This also does a bit of a touchup on the weird dropshadow element from T9586. Maybe not totally design-approved now but it's less ugly, at least.
Test Plan:
{F906695}
{F906696}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9519
Differential Revision: https://secure.phabricator.com/D14330
Summary:
Ref T9614. Currently, a lot of Build Plan behavior is covered by a global "can manage" policy.
One install in particular is experiencing difficulty with warring factions within engineering aborting one another's builds.
As a first step to remedy this, and also generally make Harbormaster more flexible and bring it in line with other applications in terms of policy power:
- Give Build Plans normal view/edit policies.
- Require "Can Edit" to run a plan manually.
Having "Can View" on plans may be a little weird in some cases (the status of a Buildable might be bad because of a build you can't see) but we can cross that bridge when we come to it.
Next change here will require "Can Edit" to abort a build. This will reasonably allow installs to reserve pause/abort for administrators/adults. (I might let anyone restart a plan, though?)
Test Plan:
- Created a new build plan.
- Verified defaults were inherited from application defaults (swapped them around, too).
- Saved build plan.
- Edited policies.
- Verified autoplans get the right policies.
- Verified old plans got migrated properly.
- Tried to run a plan I couldn't edit (denied).
- Ran a plan from CLI with `bin/harbormaster`.
- Tried to create a plan with an unprivileged user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9614
Differential Revision: https://secure.phabricator.com/D14321
Summary: Fixes T9631. Build steps created before I added this option may not have it specified, which could throw later. Make handling a little more robust.
Test Plan: Will ask @yelirekim to report back.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T9631
Differential Revision: https://secure.phabricator.com/D14336
Summary: Ref T9628. The porting feature has been fairly stable for a while, so make some reasonable effort to document how it works and some of the tradeoffs it involves.
Test Plan: Generated and read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9628
Differential Revision: https://secure.phabricator.com/D14335
Summary:
Fixes T9610.
- We currently permit you to `bin/auth recover` users who can not establish web sessions (but this will never work). Prevent this.
- We don't emit a tailored error if you follow one of these links. Tailor the error.
Even with the first fix, you can still hit the second case by doing something like:
- Recover a normal user.
- Make them a mailing list in the DB.
- Follow the recovery link.
The original issue here was an install that did a large migration and set all users to be mailing lists. Normal installs should never encounter this, but it's not wholly unreasonable to have daemons or mailing lists with the administrator flag.
Test Plan:
- Tried to follow a recovery link for a mailing list.
- Tried to generate a recovery link for a mailing list.
- Generated and followed a recovery link for a normal administrator.
{F906342}
```
epriestley@orbital ~/dev/phabricator $ ./bin/auth recover tortise-list
Usage Exception: This account ("tortise-list") can not establish web sessions, so it is not possible to generate a functional recovery link. Special accounts like daemons and mailing lists can not log in via the web UI.
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9610
Differential Revision: https://secure.phabricator.com/D14325
Summary: Ref T9625. I want this to be fixed ASAP hence here's the patch.
Test Plan:
- ~~Apply D14323~~ (This patch was made before it was merged)
- Apply this patch
- voila! Now I see the Ponder answer has correct logo.
{F906357}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, revi
Maniphest Tasks: T9625
Differential Revision: https://secure.phabricator.com/D14331
Summary: Ref T9625. This is an example of how to fill in the missing calls.
Test Plan:
- Verified that an icon is now shown for feed stories.
- Verified that an icon is now shown in the "PHID Types" module panel in Config.
{F906325}
{F906326}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9625
Differential Revision: https://secure.phabricator.com/D14324
Summary: Ref T9625. Some PHID types are missing application or icon specifications. This makes it easier to spot them.
Test Plan: {F906321}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9625
Differential Revision: https://secure.phabricator.com/D14323
Summary:
These are a little out of date:
- Link to Starmap since it explicitly exists now.
- Link to "Planning" instead of the old task.
- Link to "Prioritization" instead of telling anyone to build stuff themselves.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14328
Summary:
Currently, Version numbers are sort of randomly shown on "All Settings" beacuse we didn't have any better place to put them.
Now that we have modules, expose them as a config module.
Test Plan:
{F906426}
Grepped for "all settings" to look for other references to the old location, but didn't get any relevant hits.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14327
Summary: Fixes T9598.
Test Plan:
- Used "Send Message" as a logged-in user.
- Used "Send Message" as a logged-out user. The action was disabled and clicking it popped up a login dialog.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9598
Differential Revision: https://secure.phabricator.com/D14326
Summary:
It's hard for us to predict how long patches and migrations will take in the general case since it varies a lot from install to install, but we can give installs some kind of rough heads up about longer patches. I'm planning to just put a sort of hint for things in the changelog, something like this:
{F905579}
To make this easier, start storing how long stuff took. I'll write a little script to dump this into a table for the changelog.
Test Plan:
Ran `bin/storage status`:
{F905580}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14320
Summary:
Fixes T6707. Users can currently do this:
- Log in to a service (like Facebook or Google) with account "A".
- Link their Phabricator account to that account.
- Log out of Facebook, log back in with account "B".
- Refresh the account link from {nav Settings > External Accounts}.
When they do this, we write a second account link (between their Phabricator account and account "B"). However, the rest of the codebase assumes accounts are singly-linked, so this breaks down elsewhere.
For now, decline to link the second account. We'll permit this some day, but need to do more work to allow it, and the need is very rare.
Test Plan:
- Followed the steps above, hit the new error.
- Logged back in to the proper account and did a link refresh (which worked).
{F905562}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6707
Differential Revision: https://secure.phabricator.com/D14319
Summary:
We don't use these for anything, we're inconsistent about recording them, and there's some mild interaction with privacy concerns and data retention. Every other log we store any kind of information in can be given a custom retention policy after recent GC changes.
If we did put this back eventually it would probably be better to store a session identifier anyway, since that's more granular and more detailed.
You can fetch this info out of access logs anyway, too.
Test Plan: Left a couple of comments.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14315
Summary:
Ref T182. Replace the total mess we had before with a sort-of-reasonable element.
This automatically updates using "javascript".
Test Plan:
{F901983}
{F901984}
Used "Land Revision", saw the land status go from "Waiting" -> "Working" -> "Landed" without having to mash reload over and over again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14314
Summary:
An attempt to resolve T9600.
- `PhabricatorPasteQuery` builds truncated snippet when requested using `needSnippet()`.
- `PhabricatorPasteSearchEngine` uses Paste snippet istead of content.
- `PhabricatorSourceCodeView` accepts truncated source and type instead of line limit.
Test Plan: Generated some content for Paste application and also added huge JSON oneliner. Checked Paste application pages in browser.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9600
Differential Revision: https://secure.phabricator.com/D14313
Summary: Ref T9532.
Test Plan: I don't have this configured locally but this seems very likely to be the correct fix. This list should be a list of PHIDs, but is a list of PHIDs followed by one PhabricatorRepository object.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T9532
Differential Revision: https://secure.phabricator.com/D14311
Summary: Ref T8628. Updates Search.
Test Plan: Did various searches, saved new queries, reordered, ran new queries.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8628
Differential Revision: https://secure.phabricator.com/D14268
Summary:
Fixes T9118. When populating some policy controls like "Default Can View" for repositories, we do some special logic to add object policies which are valid for the target object type.
For example, it's OK to set the default policy for an object which has subscribers to "Subscribers".
However, this logic incorrectly //removed// custom policies, so the form input ended up blank.
Instead, provide both object policies and custom policies.
Test Plan:
- Set default view policy to a custom policy.
- Hit "Edit" again, saw control correctly reflect custom policy after change.
- Set default edit policy to a different custom policy.
- Saved, edited, verified both policies stuck.
- Set both policies back.
- Checked some other object types to make sure object policies still work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9118
Differential Revision: https://secure.phabricator.com/D14310
Summary: We often just setError as an array even if it's only one error. This just makes the UI a little cleaner in these cases.
Test Plan: Remove all reviewers from a diff, see status error without list styling.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14308
Summary:
Basically similar to D13941 but a little more extreme:
- Really strongly emphasize reproducibility for bug reports, and set users up for rejection if they don't satisfy this.
- Really strongly emphasize problem descriptions for feature requests, and set users up for rejection.
- Get rid of various "please give us feedback"; we get plenty of feedback these days.
- Some modernization tweaks.
- Split the support document into:
- Stuff we actually support for free (security / good bug reports / feature requests).
- Stuff you can pay us for (hosting / consulting / prioritization).
- A nebulous "community" section, with appropriate (low) expectations that better reflects reality.
My overall goals here are:
- Set expectations better, so users don't show up in IRC expecting it to be a "great place to get amazing support" or whatever the docs said in 2011.
- Possibly move the needle slightly on bug reports / feature request quality, maybe.
Test Plan: Read changes carefully.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14305
Summary:
I'm going to do some version of D13941. Clean up extra links to the old document first.
These were just randomly links from various places that we no longer really want feedback on and/or are now better covered by other documents.
Test Plan:
- `grep`
- Reviewed Config/Welcome screen.
- Reviewed `uri.allowed-editor-protocols`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14303
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: Fixes T9573. This incorrectly affected Phriction. I could restore it for only projects, but you didn't like the rule very much anyway and I don't feel strongly about it.
Test Plan: Unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9573
Differential Revision: https://secure.phabricator.com/D14287
Summary: Ref T9336. Links the timeline photo to user profile. Presume this always exists?
Test Plan: Review a few timelines, click on heads.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9336
Differential Revision: https://secure.phabricator.com/D14283
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: Ref T182. Ref T9519. Some of what this describes doesn't exist yet, but should soon.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182, T9519
Differential Revision: https://secure.phabricator.com/D14258
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: Fixes T9538. Ref T9408. `cowsay` and `figlet` Remarkup rules are being mangled in HTML mail right now. Put them in <pre> to unmangle them.
Test Plan:
Sent myself a cow + figlet in mail.
Used `bin/mail show-outbound --id ... --dump-html > dump.html` + open that HTML file in Safari to preview HTML mail.
Saw linebreaks and monospaced formatting.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9538, T9408
Differential Revision: https://secure.phabricator.com/D14248
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 T9514. I missed these when I swapped out the console stuff recently.
Test Plan: Ran `bin/storage probe`, saw bold instead of escape sequences.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9514
Differential Revision: https://secure.phabricator.com/D14240
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, `queueTask()` accepts `$priority` as its third argument. Allow it to take a full range of `$options` instead. This API just never got updated after we expanded avialable options.
Arguably this whole API should be some kind of "TaskQueueRequest" object but I'll leave that for another day.
Test Plan:
- Grepped for `queueTask()` and verified no other callsites are affected by this API change.
- Ran some daemons.
- See also next diff.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14235
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. Provide some general descriptions of Drydock in the docs.
Test Plan: Reading.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14215
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: Fixes T9483. This bookname is `phabcontrib`, not `contributor`.
Test Plan: `grep` / clicked these links.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9483
Differential Revision: https://secure.phabricator.com/D14195
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. The handling in D14183 didn't deal with new field values properly.
Make all this handling more consistent.
Test Plan: Created a new WorkignCopy build plan with some repos.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9123
Differential Revision: https://secure.phabricator.com/D14184
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 T5991. See D14116. We are consistent but nonstandard in our use of exit codes. This document explains what we use exit codes for and why we do this.
Test Plan: Read it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5991
Differential Revision: https://secure.phabricator.com/D14173
Summary: Fixes T9260. That task has a good description of the issue.
Test Plan: Followed steps in T9260 to reproduce the issue. Applied patch; issue went away.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9260
Differential Revision: https://secure.phabricator.com/D14169
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
