Summary:
We've been having trouble with viewing diffs timing out when there's a lot of unit test failures. It was caused by formatting userdata for every single failure. The expensive part of this was actually creating the engine for every result, so moved the construction outside of the loop.
Diffs that timed out (2 min) loading before load in around 6 seconds now.
Test Plan: Loaded diffs that used to time out. Verified that details still looked right when Show Full Unit Test Results Is Clicked.
Reviewers: epriestley, keegancsmith, lifeihuang, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, andrewjcg
Differential Revision: https://secure.phabricator.com/D7581
Summary:
Ref T4110. This denormalized field used to power "Group By: Assigned" got dropped in the T2217 migration at some point.
Restore its population, and fix all the data in the database.
Test Plan: Ran migration, verified database came out reasonable-looking. Reassigned a task, verified database. Ran a "Group By: assigned" query.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4110
Differential Revision: https://secure.phabricator.com/D7602
Summary: Fixes T4123. If you click "Profile" on a page, we already profile all the ajax requests it generates. Do the same for "Analyze Query Plans".
Test Plan: Viewed a page with Ajax requests using "Analyze Query Plans", and not using "Analyze Query Plans".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4123
Differential Revision: https://secure.phabricator.com/D7601
Summary:
Fixes T4109. If a revision has a bad `repositoryPHID` (for example, because the repository was deleted), `DifferentialRevisionQuery` calls `didRejectResult()` on it, which raises a policy exception, even if the viewer is omnipotent. This aborts the `MessageParser`, because it does not expect policy exceptions to be raised for an omnipotent viewer.
Fix this in two ways:
# Never raise a policy exception for an omnipotent viewer. I think this is the expected behavior and a reasonable rule.
# In this case, load the revision for an omnipotent viewer.
This feels a little gross, but it's the only place where we do this in the codebase right now. We can clean this up later on once it's more clear what the circumstances of checks like these are.
Test Plan: Set a revision to have an invalid `repositoryPHID`, ran message parser on it, got a clean parse.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4109
Differential Revision: https://secure.phabricator.com/D7603
Summary:
Ref T2230. When fully set up, we have up to three users who all need to write into the repositories:
- The webserver needs to write for HTTP receives.
- The SSH user needs to write for SSH receives.
- The daemons need to write for "git fetch", "git clone", etc.
These three users don't need to be different, but in practice they are often not likely to all be the same user. If for no other reason, making them all the same user requires you to "git clone httpd@host.com", and installs are likely to prefer "git clone git@host.com".
Using three different users also allows better privilege separation. Particularly, the daemon user can be the //only// user with write access to the repositories. The webserver and SSH user can accomplish their writes through `sudo`, with a whitelisted set of commands. This means that even if you compromise the `ssh` user, you need to find a way to escallate from there to the daemon user in order to, e.g., write arbitrary stuff into the repository or bypass commit hooks.
This lays some of the groundwork for a highly-separated configuration where the SSH and HTTP users have the fewest privileges possible and use `sudo` to interact with repositories. Some future work which might make sense:
- Make `bin/phd` respect this (require start as the right user, or as root and drop privileges, if this configuration is set).
- Execute all `git/hg/svn` commands via sudo?
Users aren't expected to configure this yet so I haven't written any documentation.
Test Plan:
Added an SSH user ("dweller") and gave it sudo by adding this to `/etc/sudoers`:
dweller ALL=(epriestley) SETENV: NOPASSWD: /usr/bin/git-upload-pack, /usr/bin/git-receive-pack
Then I ran git pushes and pulls over SSH via "dweller@localhost". They successfully interacted with the repository on disk as the "epriestley" user.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7589
Summary: Although I don't want to end up with 20 of these again, this is a reasonable default to provide, particularly for installs where a large portion of the userbase primarily reports bugs and does not interact with them directly.
Test Plan: Hit `/maniphest/`, saw "Subscribed", clicked it, saw the tasks I'm subscribed to.
Reviewers: jbrown, btrahan
Reviewed By: jbrown
CC: aran
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D7586
Summary:
A usable, Land to GitHub flow.
Still to do:
- Refactor all git/hg stratagies to a sane structure.
- Make the dialogs Workflow + explain why it's disabled.
- Show button and request Link Account if GH is enabled, but user is not linked.
- After refreshing token, user ends up in the settings stage.
Hacked something in LandController to be able to show an arbitrary dialog from a strategy.
It's not very nice, but I want to make some more refactoring to the controller/strategy/ies anyway.
Also made PhabricatorRepository::getRemoteURIObject() public, because it was very useful in getting
the domain and path for the repo.
Test Plan:
Went through these flows:
- load revision in hosted, github-backed, non-github backed repos to see button as needed.
- hit land with weak token - sent to refresh it with the extra scope.
- Land to repo I'm not allowed - got proper error message.
- Successfully landed; Failed to apply patch.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7555
Summary:
Ref T4039. This fixes an issue where a user with the ability to create repositories could view repositories he is otherwise not permitted to see, by following these steps:
- Suppose you want to see repository "A".
- Create a repository with the same VCS, called "B".
- Edit the local path, changing "/var/repo/B" to "/var/repo/A".
- Now it points at a working copy of a repository you can't see.
- Although you won't be able to make it through discovery (the pull will fail with the wrong credentials), you can read some information out of the repository directly through the Diffusion UI, probably?
I'm not sure this was really practical to execute since there are a bunch of sanity checks along most/all of the major pathways, but lock it down since normal users shouldn't be editing it anyway. In the best case, this would make a mess.
Test Plan: {F81391}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D7580
Summary:
Ref T4039. This is mostly to deal with that, to prevent the security issues associated with mutable local paths. The next diff will lock them in the web UI.
I also added a confirmation prompt to `bin/repository delete`, which was a little scary without one.
See one comment inline about the `--as` flag. I don't love this, but when I started adding all the stuff we'd need to let this transaction show up as "Administrator" it quickly got pretty big.
Test Plan: Ran `bin/repository edit ...`, saw an edit with a transaction show up on the web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D7579
Summary:
Fixes T4095. Fixes T3817.
- The batch editor has some funky handle code which misses projects, share that.
- Remove some hacks for T3817 that should be good now.
Test Plan: Looked at batch editor, saw projects. Looked at task list.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, martin.schulz
Maniphest Tasks: T3817, T4095
Differential Revision: https://secure.phabricator.com/D7578
Summary:
Fixes T3741. The flag is respected in terms of actually creating the account, but the UI is a bit unclear.
This can never occur naturally, but installs can register an event which locks it.
Test Plan:
Artificially locked it, verified I got more reasonable UI;
{F81282}
Reviewers: btrahan, datr
Reviewed By: datr
CC: aran
Maniphest Tasks: T3741
Differential Revision: https://secure.phabricator.com/D7577
Summary:
- Add an option for the queue.
- By default, enable it.
- Dump new users into the queue.
- Send admins an email to approve them.
Test Plan:
- Registered new accounts with queue on and off.
- As an admin, approved accounts and disabled the queue from email.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7576
Summary:
- If you're an administrator and there are users waiting for approval, show a count on the home page.
- Sort out the `isUserActivated()` access check.
- Hide all the menu widgets except "Logout" for disabled and unapproved users.
- Add a "Log In" item.
- Add a bunch of unit tests.
Test Plan: Ran unit tests, clicked around as unapproved/approved/logged-in/logged-out users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D7574
Summary:
Nothing fancy here, just:
- UI to show users needing approval.
- "Approve" and "Disable" actions.
- Send "Approved" email on approve.
- "Approve" edit + log operations.
- "Wait for Approval" state for users who need approval.
There's still no natural way for users to end up not-approved -- you have to write directly to the database.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7573
Summary:
Mailbox sometimes (?) changes the case of the email address (?). Be more liberal in what we accept.
Also fix a minor output bug.
Test Plan: Sent mail to `e1+...` instead of `E1+...`, verified it arrived.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7575
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:
- Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
- Migrate all the existing users.
- When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
- Just make the checks look at the `isEmailVerified` field.
- Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
- Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
- When the queue is enabled, registering users are created with `isApproved = false`.
- Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
- They go to the web UI and approve the user.
- Manually-created accounts are auto-approved.
- The email will have instructions for disabling the queue.
I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.
Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.
Test Plan:
- Ran migration, verified `isEmailVerified` populated correctly.
- Created a new user, checked DB for verified (not verified).
- Verified, checked DB (now verified).
- Used Conduit, People, Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7572
Summary:
Ref T3472. Currently, if an install only allows "@mycompany.com" emails and you try to register with an "@personal.com" account, we let you pick an "@mycompany.com" address instead. This is secure: you still have to verify the email. However, it defies user expectation -- it's somewhat confusing that we let you register. Instead, provide a hard roadblock.
(These accounts can still be linked, just not used for registration.)
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3472
Differential Revision: https://secure.phabricator.com/D7571
Summary: See private chatter. Make it explicitly clear when adding a provider that anyone who can browse to Phabricator can register.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7570
Summary: We don't actually support this yet, so hide the configuration.
Test Plan: Edited branches for an hg repo.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7563
Summary:
Ref T2230. As far as I can tell, getting SVN working over HTTP is incredibly complicated. It's all DAV-based and doesn't appear to have any kind of binary we can just execute and pass requests through to. Don't support it for now.
- Disable it in the UI.
- Make sure all the error messages are reasonable.
Test Plan: Tried to HTTP an SVN repo. Tried to clone a Git repo with SVN, got a good error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7562
Summary:
Fixes T3034. This is obsoleted by modern policies.
This was written by a Facebook intern and is rarely used -- the Hive install might be the only use in the wild. It has never really worked correctly.
Test Plan: `grep`; browsed Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3034
Differential Revision: https://secure.phabricator.com/D7568
Summary: Fixes T3535. Also, flip flop on that spacing thing and make the spaces purdy
Test Plan: got an arcanist projected phid in the json dict
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3535
Differential Revision: https://secure.phabricator.com/D7565
Summary: adds FIELD_PROJECTS and deploys it to Maniphest Task Herald Adapter. Went with "projects" because it feels like that could go well in other Adapters that want to conditionalize based on project.
Test Plan: made a new herald rule to be cc'd if project foo was on a task. it worked!
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7564
Summary:
Ref T2230. Very rarely, even though we've flushed the connection and sent all the data, we'll close the connection before Git is happy with it and it will flip out with an error like this:
fatal: The remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed
This is hard to reproduce because it depends on the order of read/write operations we can't directly control. I only saw it about 2% of the time, by just running `git pull` over and over again.
Waiting for Git to close its side of the connection seems to fix it.
Test Plan: Ran `git clone` a ton of times without seeing the error again. Ran `git push` a ton of times with new commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7558
Summary:
Ref T2230. The SVN protocol has a sensible protocol format with a good spec here:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/libsvn_ra_svn/protocol
Particularly, compare this statement to the clown show that is the Mercurial wire protocol:
> It is possible to parse an item without knowing its type in advance.
WHAT A REASONABLE STATEMENT TO BE ABLE TO MAKE ABOUT A WIRE PROTOCOL
Although it makes substantially more sense than Mercurial, it's much heavier-weight than the Git or Mercurial protocols, since it isn't distributed.
It's also not possible to figure out if a request is a write request (or even which repository it is against) without proxying some of the protocol frames. Finally, several protocol commands embed repository URLs, and we need to reach into the protocol and translate them.
Test Plan: Ran various SVN commands over SSH (`svn log`, `svn up`, `svn commit`, etc).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7556
Summary:
Ref T2230. This is substantially more complicated than Git, but mostly because Mercurial's protocol is a like 50 ad-hoc extensions cobbled together. Because we must decode protocol frames in order to determine if a request is read or write, 90% of this is implementing a stream parser for the protocol.
Mercurial's own parser is simpler, but relies on blocking reads. Since we don't even have methods for blocking reads right now and keeping the whole thing non-blocking is conceptually better, I made the parser nonblocking. It ends up being a lot of stuff. I made an effort to cover it reasonably well with unit tests, and to make sure we fail closed (i.e., reject requests) if there are any parts of the protocol I got wrong.
A lot of the complexity is sharable with the HTTP stuff, so it ends up being not-so-bad, just very hard to verify by inspection as clearly correct.
Test Plan:
- Ran `hg clone` over SSH.
- Ran `hg fetch` over SSH.
- Ran `hg push` over SSH, to a read-only repo (error) and a read-write repo (success).
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7553
Summary: Ref T2230. Fixes T4079. As it turns out, this is Git being weird. See comments for some detials about what's going on here.
Test Plan: Created shallow and deep Git clones.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4079, T2230
Differential Revision: https://secure.phabricator.com/D7554
Summary: Fixes T4084. See that task for discussion.
Test Plan: Did `git clone`. My setup doesn't precisely reproduce the original issue, but hopefully @enko can confirm this is a fix.
Reviewers: btrahan, enko
Reviewed By: enko
CC: enko, aran
Maniphest Tasks: T4084
Differential Revision: https://secure.phabricator.com/D7561
Summary:
Ref T2230. In Git, we can determine if a command is read-only or read/write from the command itself, but this isn't the case in Mercurial or SVN.
For Mercurial and SVN, we need to proxy the protocol that's coming over the wire, look at each request from the client, and then check if it's a read or a write. To support this, provide a more flexible version of `passthruIO`.
The way this will work is:
- The SSH IO channel is wrapped in a `ProtocolChannel` which can parse the the incoming stream into message objects.
- The `willWriteCallback` will look at those messages and determine if they're reads or writes.
- If they're writes, it will check for write permission.
- If we're good to go, the message object is converted back into a byte stream and handed to the underlying command.
Test Plan: Executed `git clone`, `git clone --depth 3`, `git push` (against no-write repo, got error), `git push` (against valid repo).
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, asherkin, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7551
Summary: Missing some `break;`, pretty sure this is causing the issue on `secure.phabricator.com`.
Test Plan: Will push.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7559
Summary: This CAN_EDIT capability doesn't exist. `PhabricatorMacroCapabilityManage::CAPABILITY` (checked on line 15) is used instead.
Test Plan: Disabled, then re-enabled a macro.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Differential Revision: https://secure.phabricator.com/D7550
Summary:
Now that diffs have PHIDs we can create buildables for them.
This also adds `buildable.diff` in the variables list so the diff ID is available, and it also fixes the Cancel button on "Edit Plan" page so it redirects to the right place.
Test Plan: Created a buildable from a diff, ran a build plan against it that had `echo ${buildable.diff}` and got the right ID. Also tested the "Edit Plan" cancel redirect.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7546
Summary:
This uses an event listener to render the status of builds on their buildables. The revision and commit view now renders out the status of each of the builds.
Currently the revision controller has the results for the latest diff rendered out. We might want to show the status of previous diffs in the future, but for now I think the latest diff should do fine.
There's also a number of bug fixes in this diff, including a particularly nasty one where builds would have a build plan PHID generated for them, which resulted in handle lookups always returning invalid objects.
Test Plan: Ran builds against diffs and commits, saw them appear on the revision and commit view controllers.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7544
Test Plan: This is one of the rare moments where unit tests for views would be useful.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7547
Summary:
Ref T1049. This is very minimal, but does what it says.
I merged the variable replacement code so Remote + HTTP can share more stuff.
Test Plan:
Ran "HTTP" and "Remote" build plans.
{F79886}
{F79887}
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: zeeg, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7541
Summary: This prevents a crash in applying build plans when more than one buildable exists for the same object. It also adds a check into the "New Manual Build" page to ensure that users can't create a buildable for an object that already has one.
Test Plan: Tried to create a buildable for an object that already has one and a nice friendly error appeared. Applied a build plan to a buildable whose object has two buildables and didn't get a crash any more.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7543
Summary:
I just want to make sure that this is the style we want.
It seems less readable to me in some cases.
Test Plan: Looked at DarkConsole with errors.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7533
Summary: This adds a `build.id` variable, cleans up the naming convention of other variables and also fixes an issue in the remote command to read the buffers after the command finishes.
Test Plan: Ran a build with `/bin/echo ${build.id}` and saw the build ID come through.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7540
Summary: This puts back the stronger variable replacement that was missed the last update to D7519.
Test Plan: Re-ran a remote build that had variables in the command and everything worked as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7539
Summary: This fixes an issue where content would be discarded when the content to append is larger than the chunk size limit.
Test Plan: Tested running a remote command that does `I=0; while true; do echo "$I"; I=$[$I+1]; done` and all of the outputted numbers matched the line numbers in the logs.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7537
Summary: This adds a build step implementation for running a command on a remote machine over SSH. It supports merging in various variables about the build (such as the commit hash / revision ID, repository call sign, version control type and clone URI).
Test Plan: Configured a build plan to run `/bin/true` on localhost and the build passed. Configured a build plan to run `/bin/false` on localhost and the build failed.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7519
Summary:
Depends on D7519.
This implements support for build logs in Harbormaster. This includes support for appending to a log from the "Run Remote Command" build step.
It also adds the ability to cancel builds.
Currently the build view page doesn't update the logs live; I'm sure this can be achieved with Javelin, but I don't have enough experience with Javelin to actually make it poll from updates to content in the background.
{F79151}
{F79153}
{F79150}
{F79152}
Test Plan:
Tested this by setting up SSH on a Windows machine and using a Remote Command configured with:
```
C:\Windows\system32\cmd.exe /C cd C:\Build && mkdir Build_${timestamp} && cd Build_${timestamp} && git clone --recursive https://github.com/hach-que/Tychaia.git && cd Tychaia && Protobuild.exe && C:\Windows\Microsoft.NET\Framework\v4.0.30319\MSBuild.exe Tychaia.Windows.sln
```
and observed the output of the build stream from the Windows machine into Phabricator.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7521
Summary: these transactions should //never// merge since they are always created for a 1:1 replacement. (ie any merging would be implicitly erroneous). Fixes T4081
Test Plan: made a mock with three images and replaced all three successfully. replaced image A with image B, did not save, replaced image B with image C, then saved and verified transaction correctly showed image A replaced with image C.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4081
Differential Revision: https://secure.phabricator.com/D7536
Summary:
Depends on D7500.
This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).
Test Plan: Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, hwinkel
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7501
Summary: another little piece here that basically just adds some permissions to source editing. serving it up before I do anything too complicated to make sure it seems kosher. in terms of what comes next this form needs to be dynamic based on source type so there'll be some fun there. That said, I plan to implement a more simple "phabricator form" only version to start here and flesh out a few other things like queues with that.
Test Plan: set permission to no one for source edit and got a nice error page.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7535
Summary: I've kept this as close as possible to the Git version for ease of review and later refactoring of them both together. At minimum, the functions to get the working dir should probably be cleaned up one day.
Test Plan: Landed a revision.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7534
Summary:
See <https://github.com/facebook/phabricator/issues/433>. We were missing a "^" here.
This should be moved over to transactions soon and then we can get rid of the duplication. :/
Test Plan: Tried to create a repository with callsign "9X", got a helpful error about "ALL UPPERCASE LETTERS".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7531
Summary:
Fixes T4061. Following the instructions in the documentation with Apache 2.4 (which is installed
with Ubuntu 13.10 and other distributions) will result in a "403 forbidden" error.
The instruction provides information on how to fix it.
Test Plan: Tested on apache 2.4 install
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, asherkin
Maniphest Tasks: T4061
Differential Revision: https://secure.phabricator.com/D7529
Summary:
After upgrading to PHP 5.5, the conduit list was not fully visible because
INF was being treated as "0" for some reason. Fixed by making it a PHP_MAX_INT
Test Plan: Checked on PHP 5.5
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7530
Summary:
Fixes T4067. The way `DiffusionCommitQuery` works prevents it from loading SVN identifiers in some cases without additional constraints, since "12345" might be an SVN revision 12345, or it might be the first 5 characters of a Git commit hash.
Introduce `withRepository()` as a shorthand for `withDefaultRepository()` + `withRepositoryIDs()`. This tells the query to:
- Only look in the given repository; and
- use the more liberal identifier resolution rules while doing so.
The practical impact this has is that blame tooltips in SVN work again. The other queries which are fixed here were never run in SVN (which doesn't have first-class branches or tags); I've cleaned them up only for completeness.
Test Plan:
- Viewed blame in SVN, saw information again instead of empty tooltip.
- Viewed brnaches/tags in Mercurial and Git.
{F79226}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4067
Differential Revision: https://secure.phabricator.com/D7523
Summary:
we were just checking if projects/ was in the URI before barfing. Use some more fun utility functions such that we only complain if there is no project.
Fixes T4071.
Test Plan: made a subpage under a project - success! tried to make a project wiki page where there was no project - successful failure! tried to make a project wiki sub page where there was no project - successful failure!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4071
Differential Revision: https://secure.phabricator.com/D7527
Summary:
See discussion in <https://github.com/facebook/phabricator/issues/430>.
(If we end up with more than like 5 of these we should probably make this a warning or something instead, the only goal is to prevent user error.)
Test Plan:
{F79196}
{F79197}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, enko
Differential Revision: https://secure.phabricator.com/D7522
Summary: The "Reviewers" condition in Differential Revision rules has the wrong typeahead and can't select projects, but should be able to.
Test Plan: {F79273}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7526
Summary: Ref T2230. This is easily the worst thing I've had to write in a while. I'll leave some notes inline.
Test Plan: Ran `hg clone http://...` on a hosted repo. Ran `hg push` on the same. Changed sync'd both ways.
Reviewers: asherkin, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7520
Summary: This is starting to get a bit sizable and it turns out Mercurial is sort of a beast, so split the VCS serve stuff into a separate controller.
Test Plan: Pushed and pulled an authenticated Git repository.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, hach-que
Differential Revision: https://secure.phabricator.com/D7494
Summary: Ref T4068. Partly, this moves discovery to the more unit-testable PhabricatorRepositoryDiscoveryEngine. It also fixes some issues, see inlines.
Test Plan: In a Mercurial repository, ran `bin/repository discover --repair`, verified commits came out topographically sorted. Ran without `--repair` and in various other contexts, like with no commits to discover and some-but-not-all commits to discover.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4068
Differential Revision: https://secure.phabricator.com/D7518
Summary:
I updated the wiki too - https://secure.phabricator.com/w/projects/pebkac/ - with what I am thinking right now. Rough plan here is
- next diff:
- implement editors and transactions
- implement "web type" for contact source
- /pebkac/item/new/ will be the entry point for this
- implement "actions" on a contact
- probably some "polish" on the scaffolding laid out here; like "create" permissions maybs
- diffs after that:
- implement "twitter" type for source
- implement email reply handler stuff for item and source
Probs a great time to blast huge holes in all this stuff. :D
Test Plan: these pages load and arc lint doesn't complain
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D7465
Summary:
Ref T1049. Ref T2222. `DifferentialDiff` does not currently have a PHID, but we need it for Harbormaster and ApplicationTransactions. See some discussion in D7501.
(I split the SQL into two sections so we can't fail in the middle. At some point, I'd like to do a pass on the migration stuff and get this happening automatically, and also simplify the PatchList.)
Test Plan:
- Ran `bin/storage upgrade`.
- Checked for valid PHIDs in the database.
- Used `phid.query` to look up a diff by PHID.
- Created a new diff and verified it got a PHID.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2222, T1049
Differential Revision: https://secure.phabricator.com/D7513
Summary: Ref T4068. Adds a command to list all commits in an "importing" status. This will allow users to use `reparse.php` to diagnose and repair issues.
Test Plan:
- Ran `bin/repository importing P`, etc.
- Used `reparse.php` to reparse some commit stages and saw status update correctly.
- Ran on a repo with no importing commits.
- Ran with `... --simple | xargs`, which saves us having to put an `awk` or something in there for users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4068
Differential Revision: https://secure.phabricator.com/D7515
Summary:
Ref T4068. In some cases like that one, I anticipate a repository not fully importing when a handful of random commits are broken. In the long run we should just deal with that properly, but in the meantime provide an administrative escape hatch so you can mark the repository as imported and get it running normally.
The major reason to do this is that Herald, Feed, Harbormaster, etc., won't activate until a repository is "imported".
Test Plan:
- Tried to mark an imported repository as imported, got an "already imported" message.
- Same for not-imported.
- Marked a repository not-imported.
- Marked a repository imported.
- Marked a repository not-imported, then waited for the daemons to mark it imported again automatically.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, kbrownlees
Maniphest Tasks: T4068
Differential Revision: https://secure.phabricator.com/D7514
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: Going to remove the workboard shadow area to keep the design less complex visually, shrink icons on mobile view
Test Plan: test uiexamples for new layout changes
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7516
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: Cleans up some CSS while adding lots of other... Mainly, this allow min-width "tables" that trigger a scroll-bar, but go full width if larger than min.
Test Plan: Tested Workboard Examples and some Project pages, Chrome, Tablet and Mobile Layouts
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7509
Summary: This is a little funky but fixes an issue with Git repos that are
non-bare needing "origin/" to resolve branches other than "master". Eventually
this should get cleaned up.
Test Plan: Reporting user verified this fixed their issue.
Auditors: btrahan
Summary: Ref T4064. The response code here isn't normally relevant, but we can hit these via `git clone http://../`, etc., and it's clearly more correct to use HTTP 500.
Test Plan: Added a fake `throw new Exception()` and verified I got an HTTP 500 response.
Reviewers: jamesr, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4064
Differential Revision: https://secure.phabricator.com/D7507
Summary: This implements an interface for adding new build steps, editing existing build steps and deleting build steps from build plans. It uses the settings definitions on the build implementation to work out what fields should be displayed on the edit page.
Test Plan:
See screenshots:
{F78529}
{F78532}
{F78528}
{F78531}
{F78527}
{F78530}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7500
Summary:
Depends on D7498.
This implements support for a "build step implementation". Build steps have an associated class name (which makes the class in PHP) and a details field, which is serialized JSON (same as PhabricatorRepository).
This also implements a SleepBuildStepImplementation which just pauses the build for a specified period of seconds.
Test Plan:
Inserted a build step with `insert into harbormaster_buildstep (phid, buildPlanPHID, className, details, dateCreated, dateModified) values ('', 'PHID-HMCP-zkh5w6czfbfpk2gxwdeo', 'SleepBuildStepImplementation', '{"seconds":5}', NOW(), NOW());` (adjusting the build plan PHID as appropriate).
Started the daemon and applied the build plan to a buildable, and saw the daemon take a 5 second delay after creating `SleepBuildStepImplementation`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7499
Summary:
ref T182.
Simple approach of clone, patch, push. While waiting for drydock, implement a hackish mutex
setup for the workspace, which should work ok as long as there's only one committer who is
carefull about theses things.
Less obvious note: This is taking the both author and commiter's 'primary email' for the commit -
which might rub some people wrong.
Test Plan:
With a hosted repo, created some diffs and landed them.
Also clicked button for some error cases, got the right error message.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: hach-que, Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7486
Summary: This implements a basic Harbormaster daemon that takes pending builds and builds them (currently just sleeps 15 seconds before moving to passed state). It also implements an interface to apply a build plan to a buildable, so that users can kick off builds for a buildable.
Test Plan: Ran `bin/phd debug PhabricatorHarbormasterBuildDaemon` and used the interface to start some builds by applying a build plan. Observed them move from 'pending' to 'building' to 'passed'.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7498
Summary:
Fixes T4060. The logic here is:
- When you take several actions at once, we show a single feed story for all of them.
- We choose the "most interesting" title for the feed story. For example, "close task" is more interesting than "add CC".
Currently, the issue with this is:
- "Add comment" is the //least interesting// title. I think this is correct: all other actions are more interesting than the fact that you added a comment.
- We try to conserve the number of objects we need to load by rendering only the most interesting transaction.
To fix this:
- Stop being so conservative; load all of the transactions and all of their PHIDs.
- Add bodies from any transactions which render bodies. In all cases (I think?) this is a maximum of one comment adding a body.
The end result is a story like this:
epriestley closed T123: the building is on fire.
"Okay guys I put the fire out"
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran, asherkin
Maniphest Tasks: T4060
Differential Revision: https://secure.phabricator.com/D7504
Summary: Fixes the width and some other minor issues.
Test Plan: Tested Mobile and Desktop
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7496
Summary:
Expands on D7488, which looks way better than the config checks. I'm leaving the config checks for now, but maybe we should just get rid of them? This advice is delivered in a far more timely way.
- Check for normal VCS binaries too.
- Link to `environment.append-paths`.
- Get rid of untranslated names (I think they're probably not too useful?)
Test Plan: See screenshots.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Differential Revision: https://secure.phabricator.com/D7495
Summary:
Currently if 'git-http-backend' is not on the PATH, there is no visible message to the user other than "info/refs: is this a valid git repository?" when trying to clone. This adds a setup check so that if there are any Git repositories in use, it will check for the existance of the "git-http-backend" binary in the PATH.
I believe this is shipped by default alongside the git package on most distros, but in some (such as OpenSUSE), this binary isn't on the PATH by default.
Test Plan: Removed `/usr/lib/git` from my `environment.append-paths` and saw the message appear. Added it back and the message went away.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4050
Differential Revision: https://secure.phabricator.com/D7488
Summary:
Ref T1493. Diffusion has some garbagey behavior for things we can't resolve. Common cases are:
- Looking at a branch that doesn't exist.
- Looking at a repository with no branches.
- Looking at a commit that doesn't exist.
- Looking at an empty repository.
In these cases, we generally fatal unhelpfully. I want to untangle this mess.
This doesn't help much, but does clean things up a bit. We currently have two separate query paths, "stablecommitname" and "expandshortcommit". These are pretty much doing the same thing -- taking some ref like "master" or "default" or a tag name or part of a commit name, and turning it into a full commit name. Merge them into a single "resolverefs" method.
This simplifies the code a fair bit, and gives us better error messages. They still aren't great, but they're like this now:
Ref "7498aec194ecf2d333e0e2baddd9d5cdf922d7f1" is ambiguous or does not exist.
...instead of just:
ERR-INVALID-COMMIT
Test Plan: Looked at Git, Mercurial and Subversion repositories that were empty and non-empty. Looked at branches/heads. Tried to look at invalid commits. Looked at tags. All of this still works, and some behaviors are a bit better than they used to be.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D7484
Summary: Fixes the junk I broke in D7484. Before that, tag content was a side effect of resolving the ref name. Now, fetch it explicitly in `diffusion.tagsquery`.
Test Plan: Looked at a tag, saw the annotation/message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7485
Summary: This puts Conduit calls into the "Services" tab. They aren't always real service calls, but I think they're big enough to belong there and be useful.
Test Plan: Viewed "Services" tab, saw conduit calls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7482
Summary: Ref T1493. Consolidate these a bit; they might need some more magic once we do `--noupdate` checkouts. Mostly just trying to clean up and centralize this code a bit.
Test Plan: Viewed and `bin/repository discover`'d Mercurial repos with and without any branches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D7480
Summary: The idea is to have all `phtize` definitions in applications to allow their separation.
Test Plan: Clicked View Options after mangling the translation.
Reviewers: epriestley
Reviewed By: epriestley
CC: btrahan, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7345
Summary: Adds summary (description) and test plan icons to make these area's more unique and differentiated over general sections.
Test Plan: Test a diff, a commit, a task
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7493
Summary: When we stack property sections without a header, we still want a thin line. This adds a line on non-initial sections but not when headers exist. Fixes T4051
Test Plan: Tested Macro, File, Diffusion, a Whacked Profile Page and Maniphest. All lines look right
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4051
Differential Revision: https://secure.phabricator.com/D7492
Summary: This disables CSRF checking around the `$repository->writeStatusMessage` so that pushing changes over HTTP to Git repositories doesn't fail miserably.
Test Plan: Applied this fix and I could `git push` to hosted repositories again.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4052
Differential Revision: https://secure.phabricator.com/D7490
Summary: This fixes an issue where Git authentication would always fail on an install with `policy.allow-public` set to false. This is because when public access is allowed, anonymous users can query the user list. However, when public access is not allowed, you have to be authenticated before you can read any of the user objects.
Test Plan:
Prior to this fix, I get:
```
james@james-laptop:~/git/8> git clone http://phabricator.local/diffusion/TEST/
Cloning into 'TEST'...
fatal: unable to access 'http://phabricator.local/diffusion/TEST/': The requested URL returned error: 403
```
when `policy.allow-public` is false. After this fix I get:
```
james@james-laptop:~/git/8> git clone http://phabricator.local/diffusion/TEST/
Cloning into 'TEST'...
remote: Counting objects: 102, done.
remote: Compressing objects: 100% (71/71), done.
remote: Total 102 (delta 6), reused 0 (delta 0)
Receiving objects: 100% (102/102), 9.89 KiB | 0 bytes/s, done.
Resolving deltas: 100% (6/6), done.
Checking connectivity... done
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4049
Differential Revision: https://secure.phabricator.com/D7489
