Page MenuHomePhabricator

Land to GitHub + support stuff
ClosedPublic

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

Details

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
Unit Tests Skipped

Event Timeline

epriestley requested changes to this revision.Nov 12 2013, 1:03 AM

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

  1. Load all of the "GitHub" accounts.
  2. 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.

avivey updated this revision to Unknown Object (????).Nov 13 2013, 6:02 AM

Updated according to feedback.

epriestley accepted this revision.Nov 14 2013, 1:24 AM

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>?

epriestley closed this revision.Nov 14 2013, 1:25 AM

Closed by commit rPdcf909ba56bf (authored by @avivey, committed by @epriestley).

avivey added inline comments.Nov 14 2013, 1:27 AM
src/applications/differential/landing/DifferentialLandingToGitHub.php
142

yes!
although, at least for my install, it'll be github.<company>/api/v3/, so not easy to guess in any case.

We could add an option to the GitHub provider when we fix it so you can add a provider directly against an Enterprise Edition.