Refactoring ConnectionManager/RepositoryHost(s) #1280
Conversation
grokys
changed the base branch from
refactor/connections-master
to
refactor/connections/local-repositories
| { | ||
| var user = await loginManager.LoginFromCache(Address, ApiClient.GitHubClient); | ||
| var accountCacheItem = new AccountCacheItem(user); | ||
| usage.IncrementLoginCount().Forget(); |
There was a problem hiding this comment.
TODO: Need to work out if a login from cache counts as a login, and if so put this in `ConnectionManager.
There was a problem hiding this comment.
Done this now in ConnectionManager.
| { | ||
| var user = await loginManager.Login(Address, ApiClient.GitHubClient, usernameOrEmail, password); | ||
| var accountCacheItem = new AccountCacheItem(user); | ||
| usage.IncrementLoginCount().Forget(); |
There was a problem hiding this comment.
TODO: Need to put this in ConnectionManager.
There was a problem hiding this comment.
Done this now in ConnectionManager.
| .Select(h => LogOut(h)) | ||
| .Merge().ToList().Select(_ => Unit.Default).Subscribe()); | ||
|
|
||
| connectionManager.ConnectionCreated = ConnectionAdded; |
There was a problem hiding this comment.
This is a temporary hack, that performs the same purpose as connectionManager.DoLogin, but renamed (as it no longer does a login) and using Task instead of an observable.
It's needed because creating a RepositoryHost is async meaning that for a short time a connection can exist without a repository host, but other older parts of the code get confused by this. This callback allows RepositoryHosts to hook into the creation of the connection to make sure a host is available by the time the connection is made available.
The longer-term goal is to remove RepositoryHost(s) so this can go away.
| // start tracking changes to the EnterpriseHost property and persist every change to the db | ||
| disposables.Add(persistEntepriseHostObs.Subscribe()); | ||
| loaded = new TaskCompletionSource<object>(); | ||
| disposables.Add(connectionManager.Connections.ForEachItem(_ => { }, ConnectionRemoved, ConnectionsReset)); |
There was a problem hiding this comment.
Here we'd ideally just call ConnectionAdded as the first callback in ForEachItem which would also handle initialization, but because of the hack mentioned above we have to initialize explicitly and use ConnectionCreated to create the RepositoryHost.
| ////{ | ||
| //// log.Info("Received AuthorizationException reading pull requests", ex); | ||
| //// return repositoryHost.LogOut(); | ||
| ////}) |
There was a problem hiding this comment.
I think this code should be removed @shana - as far as I can see there's no good reason for the PR list to be messing with removing connections?
| @@ -12,33 +15,40 @@ public static class ConnectionManagerExtensions | |||
| public static IObservable<bool> IsLoggedIn(this IConnectionManager cm, IRepositoryHosts hosts) | |||
There was a problem hiding this comment.
These methods should be changed to return async Task<T> but I didn't want to have to update all the call-sites in the PR.
| @@ -16,115 +16,4 @@ | |||
|
|
|||
| public class RepositoryHostTests | |||
| { | |||
| public class TheLoginMethod : TestBaseClass | |||
There was a problem hiding this comment.
RepositoryHost no longer has any functionality, so removed these tests.
grokys
changed the title
…connection-manager Conflicts: src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs src/UnitTests/GitHub.VisualStudio/Services/ConnectionManagerTests.cs
…connection-manager Conflicts: src/GitHub.App/Models/RepositoryHost.cs src/GitHub.App/Models/RepositoryHosts.cs src/GitHub.App/ViewModels/PullRequestListViewModel.cs src/GitHub.VisualStudio/Services/ConnectionManager.cs
|
Merged #1283 which was based on and superseded this PR - not sure why this PR wasn't automatically marked as merged when I merged that one. |
shana
changed the base branch from
refactor/connections/local-repositories
to
refactor/connections-master
This PR is the first step on the way to removing
RepositoryHosts/RepositoryHost.Ever since #845 we've been "logging in" cached connections when
ConnectionManagerstarts up to make sure we have valid credentials, and then "logging in" again fromRepositoryHost.This PR moves all of the logic around connections into
ConnectionManagerand makesRepositoryHostsa simple "wrapper" aroundConnectionManager.Connectionsthat exposes aGitHubHostand anEnterpriseHost.As this PR is a first step, there are a few hacks needed. See inline PR comments.
Fixes #1277
Fixes #1242
Depends on #1279