Summary: Fixes T8659. This isn't //explicitly// documented but I'm going to wait for a bit until the "Harbormaster" doc splits into internal/external builds to add docs for it. There's other similar stuff coming soon anyway.
Test Plan:
{F716439}
{F716440}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8659
Differential Revision: https://secure.phabricator.com/D13903
Summary:
Ref T8659. In the general case, this eventually allows build processes to do things like:
- Upload build results (like a ".app" or ".exe" or other binary).
- Pass complex results between build steps (e.g., build step A does something hard and build step B uses it to do something else).
Today, we're a long way away from having the infrastructure for that. However, it is useful to let third party build processes (like Jenkins) upload URIs that link back to the external build results.
This adds `harbormaster.createartifact` so they can do that. The only useful thing to do with this method today is have your Jenkins build do this:
params = array(
"uri": "https://jenkins.mycompany.com/build/23923/details/",
"name": "View Build Results in Jenkins",
"ui.external": true,
);
harbormaster.createartifact(target, 'uri', params);
Then (after the next diff) we'll show a link in Differential and a prominent link in Harbormaster. I didn't actually do the UI stuff in this diff since it's already pretty big.
This change moves a lot of code around, too:
- Adds PHIDs to artifacts.
- It modularizes build artifact types (currently "file", "host" and "URI").
- It formalizes build artifact parameters and construction:
- This lets me generate usable documentation about how to create artifacts.
- This prevents users from doing dangerous or policy-violating things.
- It does some other general modernization.
Test Plan:
{F715633}
{F715634}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8659
Differential Revision: https://secure.phabricator.com/D13900
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.
Test Plan: Poked around a bunch of pages.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13589
Summary:
Ref T8096. Hit this on IRC:
> epriestley: what's in the Messages tab?
> jdoe: what Messages tab??!
> epriestley: ughhhh
Test Plan: Clicked "Messages" tab, saw helpful "no messages" message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13846
Summary:
Fixes T7370. Two changes:
- Make the default to show nothing, instead of showing all the data. This is a better default because the data is sometimes sensitive. Workers should have to opt in to revealing it.
- For TargetWorkers, link to the target (technically the build, for now, since there's no dedicated target detail page).
Test Plan: {F698325}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7370
Differential Revision: https://secure.phabricator.com/D13845
Summary: Ref T8096. Like stop/start, autoplans are pushed into the system from outside (normally by `arc`) so it doesn't make any sense to run them manually.
Test Plan:
- Tried to run an autoplan from web UI, got an error.
- Ran a normal plan from web UI.
- Tried to run an autoplan from CLI, got an error.
- Ran a noraml plan from CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13844
Summary: Fixes T8657. "Auto" builds are pushed into Harbormaster by an external system (currently, `arc`) so it does not make sense to stop or resume them: Harbormaster has no way to control the external system.
Test Plan:
- Tried to restart an autobuild, got an error.
- Restarted a normal build.
- Did "Restart All" on a buildable, got restarts on non-autoplans and no restarts on autoplans.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8657
Differential Revision: https://secure.phabricator.com/D13812
Summary: Ref T8089. There's still some cleanup but none of it needs to block this.
Test Plan: Viewed `/applications/`, read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: devurandom, swisspol
Maniphest Tasks: T8089
Differential Revision: https://secure.phabricator.com/D13811
Summary:
Ref T8089. We have a lot of broken/confusing/prototype build steps that I want to hide from users when we unprototype Harbormaster.
The dialog is also just kind of unwieldy.
Organize this UI a little better and put all the sketchy junk in a "prototypes" group that you can't see unless prototypes are enabled.
This doesn't break anything (the old steps will still work fine), but should reduce user confusion.
Test Plan:
Old UI:
{F691439}
New UI (prototypes off):
{F691440}
New UI (prototypes on):
{F691441}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8089
Differential Revision: https://secure.phabricator.com/D13803
Summary: Fixes T7419. This doesn't really do anything, just adds documentation.
Test Plan:
- Read the documentation:
{F688899}
- Created a build plan which makes an HTTP request to `example.com` and waits for a result.
- Ran that build plan manually.
- Called `harbormaster.sendmessage` manually with the example lint/unit values to provide a result.
- Saw the results report correctly and the message ("fail") process as expected:
{F688902}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7419
Differential Revision: https://secure.phabricator.com/D13789
Summary: Use `PhutilClassMapQuery` where appropriate.
Test Plan: Browsed around the UI to verify things seemed somewhat working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13429
Summary: Fixes T8736. It's OK for duration to be something that comes out of JSON as an int, like `1`.
Test Plan: Will make @hach-que verify.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T8736
Differential Revision: https://secure.phabricator.com/D13531
Summary: Ref T8099, functionally I prefer to be able to set anything 'table-like' with `setTable` for design consistency. This looses the restriction and did some light grepping for other missed cases.
Test Plan: Test new UI, grep for other missing cases.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13471
Summary: Ref T8650. This should stop the problem, but isn't a root cause fix. See discussion on the task.
Test Plan: Made some local diffs, but this is a bit hard to reproduce reliably.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8650
Differential Revision: https://secure.phabricator.com/D13441
Summary: Ref T8670. The spec for "coverage" is incorrect, it's a map of paths to coverage information.
Test Plan: See next diff.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8670
Differential Revision: https://secure.phabricator.com/D13435
Summary:
Ref T8096. Various tweaks here:
- Sort result lists by importance (even lint -- "errors first" seems better than "alphabetical by file", I think?).
- Do sane stuff with display limits.
- Add a "view all" view.
- Don't show a huge table of passing tests in Differential.
- Link to full results.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13407
Summary: Ref T8096. We don't currently link to the buildable, which I think contributes to Harbormaster feeling a little scattered.
Test Plan: {F528095}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13405
Summary: Fixes T8095. Still needs UI/UX work (see T8096) but this has all the core features now.
Test Plan: Saw Harbormaster lint/unit data as though it was Differential lint-unit data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13401
Summary: Ref T8096. Show basic lint/unit info on builds. This is still pretty rough.
Test Plan: {F524839}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13383
Summary:
Ref T8096. Fixes a few bugs and glitches.
- Set build completion time when handling a message.
- Format duration information in a more human-readable way.
- Use a table for build variables.
- Fix up container PHIDs on diffs (a touch hacky, should be OK for now though).
Test Plan: Browsed around the UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13382
Summary:
Ref T8095. When build results are reported for a target, allow them to include unit and lint results.
There is no real way to see this stuff in the UI yet, either in Harbormaster or Differential.
Test Plan: Manually called this method with some results, saw Harbormaster update appropriately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13380
Summary:
Ref T8095. Same as D13377, but for unit results.
This is a bit rough and there's some duplication between this and unit results. I'll likely merge them later, but I think some of it is superficial since these iterations are still a little crude.
Test Plan: {F523499}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13378
Summary:
Ref T8095. Render lint results in a future-ready way.
This makes the renderer accept `HarbormasterBuildLintMessage` objects. If we have legacy data instead, it converts it into `HarbormasterBuildLintMessage` objects.
Design is a bit rough but will be cleaned up later after T7739.
This moves away from "postponed linters", which are obsolete after Harbormaster (and were only ever used by Facebook).
Test Plan: {F523429}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13377
Summary:
Ref T8095. Two general problems:
- I want Harbormaster to own all lint and unit test results.
- I don't want users to have to configure anything for `arc` to keep working automatically.
These are in conflict because generic lint/unit test ownership in Harbormaster requires that build targets exist which we can attach build results to. However, we can't currently create build targets on demand: Harbormaster assumes it is responsible for creating targets, then running code or making third-party service calls to actually run the builds.
I considered two broad approaches to let `arc` push results into Harbormaster without requiring administrators to configure some kind of "arc results" build plan:
# Add magic target PHIDs like `PHID-MAGIC-this-is-really-arc-unit`.
# Add new code to build real targets with real PHIDs.
(1) is probably a bit less work to get off the ground, but I think it's worse overall and very likely to create more problems in the long run. I particularly worry that it will lead to a small amount of special casing in a very large number of places, which seems more fragile.
(2) is more work upfront but I think does a better job of putting all the special casing in one place that we can, e.g., more reasonably unit test, and letting the rest of the code rarely/never care about this case since it's just dealing with normal plans/steps/targets as far as it can tell.
This diff introduces "autoplans", which are source templates for plans/steps. This let us "push" these targets into Harbormaster. Hypthetically, any process "like" arc can use autoplans to upload test/lint/etc results. In practice, probably only `arc` will ever use this, but I think it's still quite a bit cleaner than the alternative despite all the generality.
Workflow is basically:
- `arc` creates a diff.
- `arc` calls `harbormaster.queryautotargets`, passing the diff PHID and saying "I have some lint and unit results I want to stick on this thing".
- Harbormaster builds the plan, steps, and targets (if any of them don't already exist), and hands back the target PHIDs so `arc` has a completely standard-looking place to put results.
- `arc` uploads the test results to the right targets, as though Harbormaster had asked it to run unit/lint in the first place.
(This doesn't actually do any of that yet, just sets things up.)
I'll maybe doc turn that ^^^^^^ into a doc for posterity since I think it's hard to guess what an "autotarget" is, but I'm going to grab some lunch first.
Test Plan:
- Added unit tests to make sure we can build these things properly.
- Used `harbormaster.queryautotargets` to build autotargets for a bunch of diffs.
- Verified targets come up in "waiting for message" state.
- Verified plans and steps are not editable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13345
Summary:
Ref T8095. This is just general groundwork for more exciting changes:
- Use more modern conventions around controllers, UI elements, and dialogs.
- Provide real CAN_EDIT policies and policy checks (they just don't do anything yet).
Test Plan:
- Used all affected controllers.
- Faked CAN_EDIT to POLICY_NOONE and verified everything was greyed out and unselectable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13344
Summary:
Ref T8095.
Harbormaster has a `BuildItem` class, but it has no table and is unused. This was an earlier idea about representing lint/unit results and some other possible types of messages, but I think we want to be more specific than this.
Remove `BuildItem` and add `Lint` and `Unit` storage. These tables roughly parallel how we store lint/unit messages today, with some guesses about how where they'll go in the future.
Test Plan: Ran `bin/storage upgrade` and got a clean adjust out of it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13329
Summary: Ref T8096. This shows the build plan name on the Harbormaster build plan view controller. Without this, the name is not displayed anywhere on the page when you're viewing a build plan's configuration (which makes things confusing if you're updating a bunch of build plans at once).
Test Plan: Viewed a build plan, saw the build plan name on the page.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Projects: #harbormaster
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13356
Summary: Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?
Test Plan: Review and do a search in each application changed. Grep for all call sites.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13332
Summary:
Fixes T5138. Some of the "revision" properties are really "diff" properties, but we only show the properties for the most recent / current diff.
- Immediately, this makes it hard or impossible to review, e.g., lint/unit results for older diffs.
- Longer-term, these limits will become more problematic with more data on diffs after Harbormaster.
Instead, separate "revision" from "diff" properties.
(In the long term, it might make sense to show more diffs in this panel -- e.g., tabs for the 8 most recent updates or something -- but I went with the simplest approach for now since I don't have a clean way to deal with 100-update revisions offhand.)
Test Plan: {F500480}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T5138
Differential Revision: https://secure.phabricator.com/D13282
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
