Summary:
Updates table design to use new standards, work well in PHUIObjectBox. Fixes T4142
Comma
Test Plan: Tested on Diffusion, Settings, will roll out to more places soon
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran
Maniphest Tasks: T4142
Differential Revision: https://secure.phabricator.com/D7901
Summary: See github issue 413. This diff adds color to the commit view as the user expects *AND* adds green to both audit and commit views. I looked in the history (D6184) and I can't tell how expected green was, but it feels nice to me given differential color coding.
Test Plan: looked at lists of audits and commits with pretty colors.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7900
Summary: Ref T1049. Creates convenience actions at the Buildable level to stop, resume, or restart all builds.
Test Plan:
- Stopped all builds.
- Resumed all builds.
- Restarted all builds.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7899
Summary:
Ref T1049. Improves the UI:
- Pending commands, like "stopping", are shown separately from the current status.
- Pending commands are shown on the list view.
- Builds can be restarted, stopped and resumed from the list view.
- Add a missing crumb.
Test Plan:
{F99022}
{F99023}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7898
Summary: Ref T1049. The logic in the BuildEngine is a little different from the logic on the Build itself. Make these more consistent, and make queued commands more private.
Test Plan: Restarted, stopped, and resumed a build.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7897
Summary:
Ref T1049. Currently you can cancel a build, but now that we're tracking a lot more state we can stop, resume, and restart builds.
When the user issues a command against a build, I'm writing it into an auxiliary queue (`HarbormasterBuildCommand`) and then reading them out in the worker. This is mostly to avoid race messes where we try to `save()` the object in multiple places: basically, the BuildEngine is the //only// thing that writes to Build objects, and it holds a lock while it does it.
Test Plan:
- Created a plan which runs "sleep 2" a bunch of times in a row.
- Stopped, resumed, and restarted it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7892
Summary:
Ref T1049. Currently, the Harbormaster worker looks like this:
foreach (step) {
run_step(step);
}
This means steps can't ever be run in parallel. Instead, split it into two workers. The "Build" worker starts things off, and basically does:
update_build();
(We could theoretically do this in the original process because it should never take very long, but since there's a lock and it's a little bit complex it seemed cleaner to separate it.)
The "Target" worker runs an individual target (like a command, or an HTTP request, or whatever), then updates the build:
run_one_step(step);
update_build();
The new `update_build()` mechanism in `HarbormasterBuildEngine` does this, roughly:
figure_out_overall_status_of_all_steps();
if (build is done) { done(); }
if (build is fail) { fail(); }
foreach (step that is ready to run) {
queue_target_worker_for_step(step);
}
So, overall:
- The part of the code that updates Builds is completely separated from the part of the code that updates Targets.
- Targets can run in parallel.
Test Plan:
- Ran a bunch of builds via `bin/harbormaster build`.
- Ran a bunch of builds via web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7890
Summary:
If you have private replies on and a Macro reply handler set, we try to access `getMailKey()` and fail. See P1039 for a trace.
(Thanks to @Korvin for picking this up.)
Test Plan: Set configuration, repro'd the exception, applied the patch, then disabled/enabled a macro.
Reviewers: btrahan
Reviewed By: btrahan
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7896
Summary: "Run" is clearer than "Apply". This has already been changed in Harbormaster itself.
Test Plan: used eyeballs
Reviewers: btrahan, zeeg
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7889
Summary:
Fixes T4276. This adds "Change is enormous" to pre-commit content rules so we can, e.g., just reject these and not worry about them elsewhere.
Also, use the same numeric limits across the mechanisms so there's a consistent definition of an "enormous" changeset.
Test Plan:
- Set enormous limit to 15 bytes, pushed some changes, got blocked by a rule.
- Set it back, pushed OK.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7887
Summary:
pretty simple. did the bare minimum in the editor, etc. to be able to create an item from the conduit console.
I put the work in the editor for initializing new values, rather than some initializeNewItem method, mainly because Items don't have policy directly but instead policy will be defined by the queue(s) the item is in. The editor is definitely going to host this work, so it felt like it might be better to do it this way in time...? anyway, easy to make an initializeNew method instead if you want to have that paradigm going all the time.
Test Plan: made an item from teh conduit console - success. verified errors for missing data as well
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7879
Summary: if you rename a project in such a way that the old slug and the new slug are the same, there are errors when the phriction document is updated. detect this case and don't bother updating the document since there is no change. Fixes Github issue 474.
Test Plan: made a project "testTest". Viewed the wiki page. Created the wiki page. Renamed the project "TestTest". Before patch, this error'd, post patch it works!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7888
Summary:
Ref T4276. When a change is larger than 2GB, PHP can not read the entire change into a string, so Herald can not process it.
Additionally, we already have a time limit for practical reasons, but it's huge (probably incorrectly). To deal with these things:
- Add an optional byte limit to `diffusion.rawdiffquery`.
- Make the query with a 1GB limit.
- Reduce the diff timeout from 15 hours to 15 minutes.
- Add a "Changeset is enormous" field. This field is true for changes which are too large to process.
This generally makes behaviors more sane:
- We'll always make progress in Herald in a reasonable amount of time.
- Installs can write global rules to handle (or reject) these types of changes.
Test Plan: Set limit to 25 bytes instead of 1GB and ran test console on various changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7885
Summary:
Fixes T4189. Ref T4151. Allows repositories to have additional custom hooks for operations which can't be expressed with Herald (one such operation is lint).
This adds only local hook directories, since they're easier to use with existing hooks than global directories. I might add global directories eventually.
This doesn't support Mercurial since we have no demand for it and it's more complicated (we lose compatibility and power by just dropping a `hooks.d/` somewhere).
Test Plan:
- Pulled hosted SVN and Git repos to verify the hook directories generate correctly.
- Added a variety of hooks to the hook directories (echo + pass, fail).
- Pushed commits and verified the hooks fired (output expected info, or failed).
- Verified push log reflected the correct error code ("3", external) and detail ("nope.sh") when rejecting.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4151, T4189
Differential Revision: https://secure.phabricator.com/D7884
Summary:
Fixes T4264. Adds:
- New "Repository's projects" field to Herald pre-commit rules, so you can write global rules which act based on projects.
- Allows pre-ref/pre-content rules to bind to projects, and fire for all repositories in that project, so users with limited power can write rules which apply to many repositories.
- The pre-ref and pre-content classes were starting to share a fair amount of code, so I made them both extend an abstract base class.
Test Plan: Wrote new pre-ref and pre-content rules bound to projects, then pushed commits into repositories in those projects and not in those projects. The "repository projects" field populated, and the rules fired for repositories in the relevant projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7883
Summary: Ref T4264. Ref T4262. Ref T2628. Ref T3190. To write Herald object rules which bind to a project, I want to take the low budget approach and have the user just type `#project` into a text field. Formally recognize `#project` as an object name, by moving all the existing stuff from the remarkup rule to the PHID type declaration.
Test Plan: Typed `#project` into jump nav and `phid.lookup` in Conduit. Typed `#project` into Remarkup.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3190, T4264, T2628, T4262
Differential Revision: https://secure.phabricator.com/D7882
Summary: Ref T4264. Ref T2628. Ref T3102. Allows you to associate repositories with projects. In the future, you'll be able to write Herald object rules against projects, use Herald fields like "Repository's projects", and search by project.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3102, T4264, T2628
Differential Revision: https://secure.phabricator.com/D7881
Summary:
~~Set PATH for repository's hook, so the environment.append-paths can used~~
repository's hook may can't find php path if user's profile like bash_profile is not loaded.
Test Plan: check the hook generated is contain the right path
Reviewers: epriestley, #blessed_reviewers
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7743
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.
Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.
Test Plan:
- Reloaded pages.
- Browsed around Differential.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7876
Summary:
Repositories currently have a no-UI "shortcut" feature which is only used by Facebook (and I'm not sure it's even used). As implemented, this feature is policy-oblivious and kind of nonsensical. Throw it away.
I'm open to reimplementing this, but I want to see some level of interest in it before I do. The new implementation would add shortcuts to each repository, similar to how mirrors work. My original plan was to follow this up with such an implementation (it's half-implemented in my sandbox), but as I worked through it I'm not sure it's really valuable.
Test Plan: Browsed repository list, grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D7862
Summary: Ref T4222. This doesn't actually support multiple sources yet, but moves us closer by getting rid of some dead and exceedingly-singletoney code.
Test Plan: Browsed around, looked at Phame blogs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7874
Summary:
Ref T4222. This fixes some issues with Phame's resource construction.
Phame requires a fully virtual resource source, and since I want to run wordpress templates unmodified some day I don't want to build resource maps for skins.
Move all the stuff that depends on resource lists being discoverable at build time to `CelerityPhysicalResources`, and only generate maps for subclasses.
The root `CelerityResources` can now construct virtual resources; construct a virtual resource for Phame and use it.
Test Plan: Off-domain blogs work correctly now. On-domain blogs with custom skins work correctly now.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7873
Summary:
Ref T4222.
- Removes the old map and changes the CelerityResourceMap API to be entirely driven by the new map.
- The new map is about 50% smaller and organized more sensibly.
- This removes the `/pkg/` URI component. All resources are now required to have unique names, so we can tell if a resource is a package or not by looking at the name.
- Removes some junky old APIs.
- Cleans up some other APIs.
- Added some feedback for `bin/celerity map`.
- `CelerityResourceMap` is still a singleton which is inextricably bound to the Phabricator map; this will change in the future.
Test Plan:
- Reloaded pages.
- Verified packaging works by looking at generated includes.
- Forced minification on and verified it worked.
- Forced no-timestamps on and verified it worked.
- Rebuilt map.
- Ran old script and verified error message.
- Checked logs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: chad, aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7872
Summary: Ref T4222. These are the last two "return a big ball of mud" methods. Make the API stronger so I can swap out the implementations.
Test Plan: Reloaded pages.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7871
Summary: Ref T4222. Same deal as D7867, but for this other super nebulous "return a blob of stuff" method.
Test Plan: Regenerated map, browsed around, etc.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7868
Summary: Ref T4222. This was used by Facebook while developing Releeph, but should no longer be necessary since Releeph is in the upstream. I can't get an answer out of Facebook about whether they still use it or not (see T4227), so nuke it. We're going to replace it with a more general mechanism (see T4222).
Test Plan: Regenerated celerity map. Browsed some pages, still got resources.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7863
Summary: This removes the people box and adds a members property list item it's place. We may want some show/hide/see all if a project has more than //n// members, but these seems more reasonable than previous layout.
Test Plan: Tested a project with and without members, grepped for removed CSS, and tested mobile and desktop layouts.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7870
Summary: SInce we added the background color, no need to add extra padding for ObjectItemList.
Test Plan: reload a project page, no white padding in object box.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7866
Summary: Currently we markup `rXabcd`, but not `rX` on its own. Mark these up as repository object names.
Test Plan: Typed `rPOEMS`, `rPOEMS1`, `rPOEMS139893189`, etc.
Reviewers: btrahan, dctrwatson
Reviewed By: btrahan
CC: aran, poop
Differential Revision: https://secure.phabricator.com/D7859
Summary: Ref T4136. After Passphrase, user policies work correctly in this dropdown. Providing this option improves consistency and makes it easier to create, e.g., a private repository (where "no one" does not include the viewer, because they don't own the resulting object).
Test Plan: Set an object's policy to my user policy.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4136
Differential Revision: https://secure.phabricator.com/D7858
Summary:
Fixes T4242. It's currently possible to set nonsense defaults and create repositories with unintended policies, because policy configuration isn't part of creation. Instead:
- put a policy page into the creation workflow;
- require the selection of valid policies (i.e., prevent creating a repository you can't view / edit).
Test Plan:
- Created imported and hosted repositories, hit policy selection.
- Edited policies of existing repositories.
- Tried to set nonsense policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4242
Differential Revision: https://secure.phabricator.com/D7856
Summary:
Ref T4264. Allows you to create "Object" rules, in addition to Global and Personal rules. If you choose to create an Object rule, you'll be prompted to select an object on a new screen. You must be able to edit and object in order to create rules for it.
Ref T3506. This makes "All" the default filter for the transcript view, which should reduce confusion on smaller installs.
Test Plan:
- Created non-object rules.
- Created object rules.
- Triggered object rules against matching and unmatching objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3506, T4264
Differential Revision: https://secure.phabricator.com/D7853
Summary: Ref T4264. Lays the groundwork for new "Object" rule types. Prevents personal "Hook" rules, which don't make any sense.
Test Plan: Created new Maniphest (global/personal available) and Ref Hook (global only) rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7852
Summary:
If the repo isn't bare, than we need copy it's remote instead of using it.
This will probably not work if an SSH key is provided to phabricator, and in any case you must delete
all workspaces that were already created.
This will make landing those repos slower; I plan to just delete and re-clone all repos on my instance.
It will probably be simpler to just make a bare-repo a requirement of all the git-landing work.
Test Plan: landed from hosted and un-hosted repos, checked git-remote url in each newly cloned workspace.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7592
Summary: Some discussion on IRC. This is more consistent with other disabled items, which are click-to-explain.
Test Plan: Viewed UI, clicked link.
Reviewers: btrahan, dctrwatson, asherkin
Reviewed By: asherkin
CC: aran
Differential Revision: https://secure.phabricator.com/D7857
Summary:
Fixes T4270. When you download raw file content, diffs, and patches we currently give them default (all users) visibility.
Instead, bind them to the repository or revision in question.
(This code could use a bit of cleanup at some point.)
Test Plan: Hit the patch and content download links in Diffusion and the patch download link in Differential, got restricted files with accurate policy bindings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4270
Differential Revision: https://secure.phabricator.com/D7849
Summary:
Ref T4264. This gets most of the plumbing in for "object" rules, which will bind to a specific object, like a repository or project.
It does not yet let you actually create these rules.
Test Plan: Ran `storage upgrade`, created/edited rules, browsed Herald.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7847
Summary: Ref T4264. Instead of a dropdown, make this step more informative.
Test Plan: {F93928}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7846
Summary:
Ref T4264. Currently, you choose a rule's content type (revision, commit, hook) and rule type (global, personal) on the same screen.
- I want to make some rule types unavailable for some content types (e.g., personal hooks make little sense).
- I want to make content type selection use a radio control instead of a dropdown, so it can explain what the content types do in more detail.
- For new "object" hooks, I want to add a third step where you'll pick an object to bind to.
Split rule creation out into two steps. I think this won't get complicated enough for `PHUIPagedFormView`, but maybe I'll swap it in if this gets messier than I think.
Test Plan: Created some Herald rules, used back/cancel/etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7845
Summary: If `0` isn't an ancestor of the current branch, the `0::x` construction fails. This is uncommon, but not wildly unreasonable. The `ancestors()` construction is simpler anyway.
Test Plan: Viewed some `hg` repos locally (change history, file history) without anything suspicious cropping up.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7844
Summary:
Ref T4195. A legitimate rule which needs this field is "do not allow commits as root". Interestingly, we have exactly one commit as root in each Phabricator, Arcanist and libphutil.
Since the committer and author don't need to be Phabricator accounts (just the Pusher), the existing "Committer" and "Author" fields can't express this rule (they'll be empty).
Test Plan: {F93406}
Reviewers: btrahan
Reviewed By: btrahan
CC: SEJeff, aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7841
Summary:
Ref T2015. Not directly related to Drydock, but I bumped into this. All these scripts currently enumerate their workflows explicitly.
Instead, use `PhutilSymbolLoader` to automatically discover workflows. This reduces code duplication and errors (see all the bad `extends` this diff fixes) and lets third parties add new workflows (not clearly valuable?).
Test Plan: Ran `bin/x help` for each modified script.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7840
Summary:
Ref T2015. Not directly related to Drydock, but I've wanted to do this for a bit.
Introduce a common base class for all the workflows in the scripts in `bin/*`. This slightly reduces code duplication by moving `isExecutable()` to the base, but also provides `getViewer()`. This is a little nicer than `PhabricatorUser::getOmnipotentUser()` and gives us a layer of indirection if we ever want to introduce more general viewer mechanisms in scripts.
Test Plan: Lint; ran some of the scripts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7838
Summary: Ref T2015. All the Drydock query classes share the application method; move it into a shared base class to slightly shrink the codebase.
Test Plan: Browsed query UIs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7837
Summary:
Ref T2015. Moves a bunch of raw object loads into modern policy-aware queries.
Also straightens out the Log and Lease policies a little bit: there are legitimate states where these objects are not attached to a resource (particularly, while a lease is being acquired). Handle these more gracefully.
Test Plan: Lint / browsed stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7836
Summary:
Ref T2015. Currently, Drydock has a `wait-for-lease` workflow which is invoked in the background by the `lease` workflow.
The goal of this mechanism is to allow `bin/drydock lease` to print out logs as the lease is acquired. However, this predates the `runAllTasksInProcess` flags, and they provide a simpler and more robust way (potentially with `--trace` and `PhutilConsole`) to do synchronous execution and debug logging.
Simplify this whole mechanism: just run everything in-process in `bin/drydock lease`, and do logging via `--trace`. We could thread a `PhutilConsole` through things too, but this seems good enough for now.
Also various cleanup/etc.
Test Plan: Ran `bin/drydock lease`. Ran `bin/harbormaster build X --plan Y`, for `Y` being a Drydock-dependent build plan.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7835
Summary: Ref T4266. This implements rules similar to the old rules. With D7842, maybe this is reasonable? I think it's not like grotesquely bad, at least.
Test Plan: See screenshot.
Reviewers: chad, wrotte
Reviewed By: chad
CC: aran
Maniphest Tasks: T4266
Differential Revision: https://secure.phabricator.com/D7843
Summary:
Ref T2015. After introducing ApplicationSearch, the left nav turned into a soupy mess. Split the major sections into four separate areas, and unify them with a simple console.
This also reverts all the prefix stuff, since the results were awful and I don't anticipate it ever being the best solution to any UX problem.
Test Plan:
Browsed blueprints, resources, leases and logs.
Here's the new console:
{F93279}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7833
Summary: Ref T2015. Update DrydockLog for policy awareness and give it a policy query.
Test Plan: Browsed all the log interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7831
