Land to GitHub + support stuff
ClosedPublic
Actions

Authored by avivey on Nov 11 2013, 3:09 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T182: Commit into repository directly from differential

Commits

Restricted Diffusion Commit
rPdcf909ba56bf: Land to GitHub + support stuff

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.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

In theory, it would be nice if the GitHub logic looked more like:

Load all of the "GitHub" accounts.
Look for one with the same domain as the remote URI.

This shouldn't be too hard to implement, but will do a good job of future-proofing this.

Overall:

Use %P to mask the command credentials.
Consider future-proofing account management (i.e., make all code assume the user may have credentials on more than one install of GitHub. Practically, this corresponds to a link to both github.com and one or more enterprise installs). I'm fine with either approach on this, but it might actually be simpler.
Consider simplifying the permissions future; I don't think the edge case perf is that significant for the moderate additional complexity.

This looks pretty reasonable in general.

src/applications/differential/controller/DifferentialRevisionLandController.php
84	This should be `pht()`'d, but should also be the default button label?
src/applications/differential/landing/DifferentialLandingToGitHub.php
38–39	I'd just do the whole permissions request here, maybe? Rather than using Futures? This should be a rare case and it's OK if the user waits for an extra second, right?
123–128	Instead of doing this, use `%P` for the sensitive parameter and just `execx`: $workspace->execxLocal('push %P HEAD:master', $remote); This will mask the authentication information. Without this, the token will still appear in, e.g., service call logs and other places you can't control.

I can skip the search, and just look for the right one:

$this->account = id(new PhabricatorExternalAccountQuery())
  ->withAccountTypes(array("github"))
  ->withAccountDomains(array($repo_domain));

This ignores the case where the provider is configured, but the user is not linked.

src/applications/differential/controller/DifferentialRevisionLandController.php
84	not sure where this came from.

Updated according to feedback.

Nice, thanks!

src/applications/differential/controller/DifferentialRevisionLandController.php
45	goodbye moo you will be missed
src/applications/differential/landing/DifferentialLandingToGitHub.php
105	Ah, cool, this is much cleaner. I do wonder if we'll hit some "www.github.com" vs "github.com" issues, but we can wait for those to hit.
142	Should this (try to) use `api.<whatever flavor of github is going on>`?