Summary: Depends on D19063. Ref T13054. Prepare for the addition of a new `PREPARING` status by getting rid of the "scattered mess of switch statements" pattern of status management.
Test Plan: Searched/browsed buildables. Viewed buildables. Viewed revisions. Grepped for all affected symbols.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19064
Summary:
Fixes T10011.
- Modernize searching for buildables.
- Prepare for `harbormaster.buildable.search`.
- Allow users to query by status (see T10011).
- Collapse the four weird "commit / diff / revision / repository" fields into two slightly less weird fields with more UI hinting?
Test Plan: {F1131918}
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10011
Differential Revision: https://secure.phabricator.com/D15356
Summary:
Ref T10457. This makes diffs/revisions show the revision as the buildable title, and commits show the commit as the title.
Previously, the title was "Buildable X".
Also makes icons/colors/labels more consitent.
Test Plan: {F1131885}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15355
Summary:
Ref T10457. We currently have these weird, out-of-place methods on Harbormaster queries that just load handles. These were written before HandlePool, and HandlePool is now more convenient, simpler, and more efficient.
Drop this stuff in favor of using handle pools off `$viewer`.
Test Plan: Looked at buildable list, looked at buildable detail, grepped for removed methods.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15354
Summary: Ref T4245. These are all straightforward to remove.
Test Plan:
- Edited paths in a package.
- Ran `bin/audit delete --repositories ...` with various identifiers.
- Searched by repository for `R3`, `rAAAA` in Harbormaster.
- Did a Herald dry run on a commit.
- Browsed commits, made comments.
- Viewed a Releeph product list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14927
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: Ref T8099, Fixes T8341. Switches all remaining callsites to setBarColor to use setStatusIcon (sans workboards).
Test Plan: Test each of the applications I changed as I could (not Releeph).
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8341, T8099
Differential Revision: https://secure.phabricator.com/D13059
Summary: Fixes T7118. This does the basic "filter the list" thing, though it ends up being a little manual since I guess this hasn't come up before? There is also potential weird behavior if the user was using an app and lost access to it - they will have nothing selected on edit - but I think this is actually correct behavior in this circumstance.
Test Plan:
used a user who couldn't get access to the "quick create" apps and noted that the dropdown list on dashboard panel create was missing the expected engines
ran `arc unit --everything` to verify abstract method implemented everywhere
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7118
Differential Revision: https://secure.phabricator.com/D11687
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorApplicationSearchEngine` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11242
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: This implements showing the buildable status in Diffusion and unifies some of the logic used to calculate and render build and buildable statuses.
Test Plan: Looked at diffs and commits with statuses, they rendered fine. Looked at Diffusion and saw buildable status appear (with a manual buildable and manual buildables included in the query).
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9496
Summary: Ref T4986. Instead of requiring users to know the name of an application search engine class, let them select from a list.
Test Plan:
Created a new panel.
{F165468}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9500
Summary: This went smoother than expeced. Makes the rounded Card the default, also tweaked selected state a little.
Test Plan:
Test UIExamples, Maniphest, Home, Differential, Harbormaster, Audit. Everything seems normal
{F163971}
{F163973}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9408
Summary:
Updates policy, headers, typeaheads to FA over policy icons
Need advice - can't seem to place where icons come from on Typeahead? Wrong icons and wrong colors.... it is late
Test Plan:
- grepped for SPRITE_STATUS
- grepped for sprite-status
- grepped for setStatus for headers
- grepped individual icons names
Browsed numerous places, checked new dropdowns, see pudgy people.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4739
Differential Revision: https://secure.phabricator.com/D9179
Summary: Ref T4986. These are mostly mechanical now, I skipped a couple of slightly tricky ones. Still a bunch to go.
Test Plan:
For each engine:
- Viewed the application;
- created a panel to issue the query.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9017
Summary:
Ref T4809. Currently, buildables have a status field but nothing populates it. Populate it:
- When builds change state, update the Buildable state.
- Use the new Buildable state on the web UI.
- Return the new Buildable state from Conduit.
To make it easier to debug/test this:
- Provide `bin/harbormaster update Bxxx ...` to force foreground update of a Buildable.
Test Plan:
- Used `bin/harbormaster update Bxxx --force --trace` to update buildables.
- Looked at buidlable list, saw statuses reported properly.
- Used Conduit to read statuses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8799
Summary:
Ref T1049. Generally, it's useful to separate test/trial/manual runs from production/automatic runs.
For example, you don't want to email a bunch of people that the build is broken just because you messed something up when writing a new build plan. You'd rather try it first, then promote it into production once you have some good runs.
Similarly, test runs generally should not affect the outside world, etc. Finally, some build steps (like "wait for other buildables") may want to behave differently when run in production/automation than when run in a testing environment (where they should probably continue immediately).
So, formalize the distinction between automatic buildables (those created passively by the system in response to events) and manual buildables (those created explicitly by users). Add filtering, and stop the automated parts of the system from interacting with the manual parts (for example, we won't show manual results on revisions).
This also moves the "Apply Build Plan" to a third, new home: instead of the sidebar or Buildables, it's now on plans. I think this generally makes more sense given how things have developed. Broadly, this improves isolation of test environments.
Test Plan: Created some builds, browsed around, used filters, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7824
Summary: Ref T1049. Adds "Repository", "Revision", "Diff" and "Commit" as searchable fields.
Test Plan: Used all the fields to filter things.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7823
Summary: Ref T1049. Nothing fancy, but shows red for fail/error and green for pass. See discussion in D7502.
Test Plan: {F78839}
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7512
Summary:
Depends on D7501.
This just renders the buildable's actual object name onto the list, so you can see at a glance what the buildable represents. I'd like to also pull across a list of builds of this buildable and change the bar color, but I'm not quite sure how to do that in the search architecture without N+1 querying.
Test Plan:
Looked at the buildable list and it looked like this:
{F78555}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7502
Summary:
Ref T1049. I don't really want to sink too much time into this right now, but a seemingly reasonable architecture came to me in a dream. Here's a high-level overview of how things fit together:
- **"Build"**: In Harbormaster, "build" means any process we want to run against a working copy. It might actually be building an executable, but it might also be running lint, running unit tests, generating documentation, generating symbols, running a deploy, setting up a sandcastle, etc.
- `HarbormasterBuildable`: A "buildable" is some piece of code which build operations can run on. Generally, this is either a Differential diff or a Diffusion commit. The Buildable class just wraps those objects and provides a layer of abstraction. Currently, you can manually create a buildable from a commit. In the future, this will be done automatically.
- `HarbormasterBuildStep`: A "build step" is an individual build operation, like "run lint", "run unit", "build docs", etc. The step defines how to perform the operation (for example, "run unit tests by executing 'arc unit'"). In this diff, this barely exists.
- `HarbormasterBuildPlan`: This glues together build steps into groups or sequences. For example, you might want to "run unit", and then "deploy" if the tests pass. You can create a build plan which says "run step "unit tests", then run step "deploy" on success" or whatever. In the future, these will also contain triggers/conditions ("Automatically run this build plan against every commit") and probably be able to define failure actions ("If this plan fails, send someone an email"). Because build plans will run commands, only administrators can manage them.
- `HarbormasterBuild`: This is the concrete result of running a `BuildPlan` against a `Buildable`. It tracks the build status and collects results, so you can see if the build is running/successful/failed. A `Buildable` may have several `Build`s, because you can execute more than one `BuildPlan` against it. For example, you might have a "documentation" build plan which you run continuously against HEAD, but a "unit" build plan which you want to run against every commit.
- `HarbormasterBuildTarget`: This is the concrete result of running a `BuildStep` against a `Buildable`. These are children of `Build`. A step might be able to produce multiple targets, but generally this is something like "Unit Tests" or "Lint" and has an overall status, so you can see at a glance that unit tests were fine but lint had some issues.
- `HarbormasterBuildItem`: An optional subitem for a target. For lint, this might be an individual file. For unit tests, an individual test. For normal builds, an executable. For deploys, a server. For documentation generation, there might just not be subitems.
- `HarbormasterBuildLog`: Provides extra information, like command/execution transcripts. This is where stdout/stderr will get dumped, and general details and other messages.
- `HarbormasterBuildArtifact`: Stores side effects or results from build steps. For example, something which builds a binary might put the binary in "Files" and then put its PHID here. Unit tests might put coverage information here. Generally, any build step which produces some high-level output object can use this table to record its existence.
This diff implements almost nothing and does nothing useful, but puts most of these object relationships in place. The two major things you can't easily do with these objects are:
1) Run arbitrary cron jobs. Jenkins does this, but it feels tacked on and I don't know of anyone using it for that. We could create fake Buildables to get a similar effect, but if we need to do this I'd rather do it elsewhere in general. Build and cron/service/monitoring feel like pretty different problems to me.
2) Run parameterized/matrix steps (maybe?). Bamboo has this plan/stage/task/job breakdown where a build step can generate a zillion actual jobs, like "build client on x86", "build server on x86", "build client on ARM", "build server on ARM", etc. We can sort of do this by having a Step map to multiple Targets, but I haven't really thought about it too much and it may end up being not-great. I'd guess we have like an 80% chance of getting a clean implementation if/when we get there. I suspect no one actually needs this, or when they do they'll just implement a custom Step and it can be parameterized at that level. I'm not too worried about this overall.
The major difference between this and Jenkins/Bamboo/TravisCI is that all three of those are **plan-centric**: the primary object in the system is a build plan, and the dashboard shows you all your build plans and the current status. I don't think this is the right model. One disadvantage is that you basically end up with top-level messaging that says "Trunk is broken", not "Trunk was broken by commit af32f392f". Harbormaster is **buildable-centric**: the primary object in the system is stuff you can run build operations against (commits/branches/revisions), and actual build plans are secondary. The main view will be "recent commits on this branch, and whether they're good or not" -- which I think is what's most important in a larger/more complex product -- not the pass/fail status of all jobs. This also makes it easier and more natural to integrate with Differential and Diffusion, which both care about the overall status of the commit/revision, not the current status of jobs.
Test Plan: Poked around, but this doesn't really do anything yet.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, chad, aran, seporaitis
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7368
