Skip to content

Commit 9e4f343

Browse files
offbyoneoffbyone
authored
Merge pull request #1532 from github/o1/push-lqwlqnrruqpk
RetryPolicy: include service name in 401 Unauthorized error messages
2 parents 4fbc5e5 + 9b1a27b commit 9e4f343

9 files changed

Lines changed: 58 additions & 26 deletions

File tree

‎src/Octoshift/Factories/GithubApiFactory.cs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ GithubApi ISourceGithubApiFactory.Create(string apiUrl, string uploadsUrl, strin
3131
apiUrl ??= DEFAULT_API_URL;
3232
uploadsUrl ??= DEFAULT_UPLOADS_URL;
3333
sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken();
34-
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, _retryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
34+
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub");
35+
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, clientRetryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
3536
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider);
3637
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
3738
}
@@ -41,7 +42,8 @@ GithubApi ISourceGithubApiFactory.CreateClientNoSsl(string apiUrl, string upload
4142
apiUrl ??= DEFAULT_API_URL;
4243
uploadsUrl ??= DEFAULT_UPLOADS_URL;
4344
sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken();
44-
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("NoSSL"), _versionProvider, _retryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
45+
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub");
46+
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("NoSSL"), _versionProvider, clientRetryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
4547
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider);
4648
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
4749
}
@@ -51,7 +53,8 @@ GithubApi ITargetGithubApiFactory.Create(string apiUrl, string uploadsUrl, strin
5153
apiUrl ??= DEFAULT_API_URL;
5254
uploadsUrl ??= DEFAULT_UPLOADS_URL;
5355
targetPersonalAccessToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
54-
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, _retryPolicy, _dateTimeProvider, targetPersonalAccessToken);
56+
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub");
57+
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, clientRetryPolicy, _dateTimeProvider, targetPersonalAccessToken);
5558
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider);
5659
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
5760
}

‎src/Octoshift/RetryPolicy.cs‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,26 @@ namespace OctoshiftCLI
1010
public class RetryPolicy
1111
{
1212
private readonly OctoLogger _log;
13+
private readonly string _serviceName;
1314
internal int _httpRetryInterval = 1000;
1415
internal int _retryInterval = 4000;
1516

16-
public RetryPolicy(OctoLogger log)
17+
public RetryPolicy(OctoLogger log, string serviceName = null)
1718
{
1819
_log = log;
20+
_serviceName = serviceName;
1921
}
2022

23+
/// <summary>
24+
/// Returns a new RetryPolicy with the same configuration as this one, but tagged with a service name
25+
/// for more actionable error messages (e.g. on 401 Unauthorized).
26+
/// </summary>
27+
public RetryPolicy WithServiceName(string serviceName) => new(_log, serviceName)
28+
{
29+
_httpRetryInterval = _httpRetryInterval,
30+
_retryInterval = _retryInterval
31+
};
32+
2133
public async Task<T> HttpRetry<T>(Func<Task<T>> func, Func<HttpRequestException, bool> filter)
2234
{
2335
var policy = Policy.Handle(filter)
@@ -26,7 +38,15 @@ public async Task<T> HttpRetry<T>(Func<Task<T>> func, Func<HttpRequestException,
2638
_log?.LogVerbose($"Call failed with HTTP {((HttpRequestException)ex).StatusCode}, retrying...");
2739
});
2840

29-
return await policy.ExecuteAsync(func);
41+
try
42+
{
43+
return await policy.ExecuteAsync(func);
44+
}
45+
catch (HttpRequestException ex) when (ex.StatusCode == System.Net.HttpStatusCode.Unauthorized)
46+
{
47+
ThrowUnauthorizedException(ex);
48+
throw; // unreachable, but required by compiler
49+
}
3050
}
3151

3252
public async Task<PolicyResult<T>> RetryOnResult<T>(Func<Task<T>> func, Func<T, bool> resultPredicate, string retryLogMessage = null)
@@ -53,15 +73,20 @@ private AsyncRetryPolicy CreateRetryPolicyForException<TException>() where TExce
5373
_log?.LogVerbose($"[HTTP ERROR {(int?)httpEx.StatusCode}] {ex}");
5474
if (httpEx.StatusCode == System.Net.HttpStatusCode.Unauthorized)
5575
{
56-
// We should not retry on an unathorized error; instead, log and bubble up the error
57-
throw new OctoshiftCliException($"Unauthorized. Please check your token and try again", ex);
58-
};
76+
ThrowUnauthorizedException(httpEx);
77+
}
5978
}
6079
else
6180
{
6281
_log?.LogVerbose(ex.ToString());
6382
}
6483
_log?.LogVerbose("Retrying...");
6584
});
85+
86+
private void ThrowUnauthorizedException(HttpRequestException ex)
87+
{
88+
var serviceContext = _serviceName is not null ? $" ({_serviceName})" : "";
89+
throw new OctoshiftCliException($"Unauthorized{serviceContext}. Please check your token and try again", ex);
90+
}
6691
}
6792
}

‎src/OctoshiftCLI.IntegrationTests/AdoToGithub.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,14 @@ protected AdoToGithub(ITestOutputHelper output, string adoServerUrl = "https://d
2828
_versionClient = new HttpClient();
2929
var adoToken = Environment.GetEnvironmentVariable(adoPatEnvVar);
3030
_adoHttpClient = new HttpClient();
31-
var retryPolicy = new RetryPolicy(logger);
31+
var retryPolicy = new RetryPolicy(logger, $"Azure DevOps ({adoPatEnvVar})");
3232
var adoClient = new AdoClient(logger, _adoHttpClient, new VersionChecker(_versionClient, logger), retryPolicy, adoToken);
3333
var adoApi = new AdoApi(adoClient, adoServerUrl, logger);
3434

3535
var githubToken = Environment.GetEnvironmentVariable("GHEC_PAT");
3636
_githubHttpClient = new HttpClient();
37-
var githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), githubToken);
38-
var githubApi = new GithubApi(githubClient, "https://api.github.com", new RetryPolicy(logger), null);
37+
var githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger, "GitHub (GHEC_PAT)"), new DateTimeProvider(), githubToken);
38+
var githubApi = new GithubApi(githubClient, "https://api.github.com", new RetryPolicy(logger, "GitHub (GHEC_PAT)"), null);
3939

4040
Tokens = new Dictionary<string, string>
4141
{

‎src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,14 @@ public BbsToGithub(ITestOutputHelper output)
5555
_versionClient = new HttpClient();
5656

5757
_sourceBbsHttpClient = new HttpClient();
58-
_sourceBbsClient = new BbsClient(_logger, _sourceBbsHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger), sourceBbsUsername, sourceBbsPassword);
58+
_sourceBbsClient = new BbsClient(_logger, _sourceBbsHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger, "Bitbucket Server (BBS_USERNAME/BBS_PASSWORD)"), sourceBbsUsername, sourceBbsPassword);
5959

6060
_targetGithubHttpClient = new HttpClient();
61-
_targetGithubClient = new GithubClient(_logger, _targetGithubHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger), new DateTimeProvider(), targetGithubToken);
62-
var retryPolicy = new RetryPolicy(_logger);
61+
_targetGithubClient = new GithubClient(_logger, _targetGithubHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger, "GitHub (GHEC_PAT)"), new DateTimeProvider(), targetGithubToken);
62+
var retryPolicy = new RetryPolicy(_logger, "GitHub (GHEC_PAT)");
6363
var environmentVariableProvider = new EnvironmentVariableProvider(_logger);
6464
_archiveUploader = new ArchiveUploader(_targetGithubClient, UPLOADS_URL, _logger, retryPolicy, environmentVariableProvider);
65-
_targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(_logger), _archiveUploader);
65+
_targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(_logger, "GitHub (GHEC_PAT)"), _archiveUploader);
6666

6767
_blobServiceClient = new BlobServiceClient(_azureStorageConnectionString);
6868

‎src/OctoshiftCLI.IntegrationTests/GhesToGithub.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,17 +48,17 @@ public GhesToGithub(ITestOutputHelper output)
4848
};
4949

5050
_versionClient = new HttpClient();
51-
var retryPolicy = new RetryPolicy(logger);
51+
var retryPolicy = new RetryPolicy(logger, "GHES (GHES_PAT)");
5252
var environmentVariableProvider = new EnvironmentVariableProvider(logger);
5353

5454
_sourceGithubHttpClient = new HttpClient();
55-
_sourceGithubClient = new GithubClient(logger, _sourceGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), sourceGithubToken);
55+
_sourceGithubClient = new GithubClient(logger, _sourceGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger, "GHES (GHES_PAT)"), new DateTimeProvider(), sourceGithubToken);
5656
_archiveUploader = new ArchiveUploader(_targetGithubClient, UPLOADS_URL, logger, retryPolicy, environmentVariableProvider);
57-
_sourceGithubApi = new GithubApi(_sourceGithubClient, GHES_API_URL, new RetryPolicy(logger), _archiveUploader);
57+
_sourceGithubApi = new GithubApi(_sourceGithubClient, GHES_API_URL, new RetryPolicy(logger, "GHES (GHES_PAT)"), _archiveUploader);
5858

5959
_targetGithubHttpClient = new HttpClient();
60-
_targetGithubClient = new GithubClient(logger, _targetGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), targetGithubToken);
61-
_targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(logger), _archiveUploader);
60+
_targetGithubClient = new GithubClient(logger, _targetGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger, "GitHub (GHEC_PAT)"), new DateTimeProvider(), targetGithubToken);
61+
_targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(logger, "GitHub (GHEC_PAT)"), _archiveUploader);
6262

6363
_blobServiceClient = new BlobServiceClient(_azureStorageConnectionString);
6464

‎src/OctoshiftCLI.IntegrationTests/GithubToGithub.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ public GithubToGithub(ITestOutputHelper output)
3333

3434
_githubHttpClient = new HttpClient();
3535
_versionClient = new HttpClient();
36-
_githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), githubToken);
37-
_githubApi = new GithubApi(_githubClient, "https://api.github.com", new RetryPolicy(logger), null);
36+
_githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger, "GitHub (GHEC_PAT)"), new DateTimeProvider(), githubToken);
37+
_githubApi = new GithubApi(_githubClient, "https://api.github.com", new RetryPolicy(logger, "GitHub (GHEC_PAT)"), null);
3838

3939
_helper = new TestHelper(_output, _githubApi, _githubClient);
4040
}

‎src/ado2gh/Factories/AdoApiFactory.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ public virtual AdoApi Create(string adoServerUrl, string personalAccessToken)
2727
{
2828
adoServerUrl ??= DEFAULT_API_URL;
2929
personalAccessToken ??= _environmentVariableProvider.AdoPersonalAccessToken();
30-
var adoClient = new AdoClient(_octoLogger, _client, _versionProvider, _retryPolicy, personalAccessToken);
30+
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("Azure DevOps");
31+
var adoClient = new AdoClient(_octoLogger, _client, _versionProvider, clientRetryPolicy, personalAccessToken);
3132
return new AdoApi(adoClient, adoServerUrl, _octoLogger);
3233
}
3334

‎src/ado2gh/Factories/AdoPipelineTriggerServiceFactory.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ public virtual AdoPipelineTriggerService Create(string adoServerUrl, string pers
2727
{
2828
adoServerUrl ??= DEFAULT_API_URL;
2929
personalAccessToken ??= _environmentVariableProvider.AdoPersonalAccessToken();
30-
var adoClient = new AdoClient(_octoLogger, _client, _versionProvider, _retryPolicy, personalAccessToken);
30+
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("Azure DevOps");
31+
var adoClient = new AdoClient(_octoLogger, _client, _versionProvider, clientRetryPolicy, personalAccessToken);
3132
var adoApi = new AdoApi(adoClient, adoServerUrl, _octoLogger);
3233
return new AdoPipelineTriggerService(adoApi, _octoLogger, adoServerUrl);
3334
}

‎src/bbs2gh/Factories/BbsApiFactory.cs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,15 +28,17 @@ public virtual BbsApi Create(string bbsServerUrl, string bbsUsername, string bbs
2828

2929
var httpClient = noSsl ? _clientFactory.CreateClient("NoSSL") : _clientFactory.CreateClient("Default");
3030

31-
var bbsClient = new BbsClient(_octoLogger, httpClient, _versionProvider, _retryPolicy, bbsUsername, bbsPassword);
31+
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("Bitbucket Server");
32+
var bbsClient = new BbsClient(_octoLogger, httpClient, _versionProvider, clientRetryPolicy, bbsUsername, bbsPassword);
3233
return new BbsApi(bbsClient, bbsServerUrl, _octoLogger);
3334
}
3435

3536
public virtual BbsApi CreateKerberos(string bbsServerUrl, bool noSsl = false)
3637
{
3738
var httpClient = noSsl ? _clientFactory.CreateClient("KerberosNoSSL") : _clientFactory.CreateClient("Kerberos");
3839

39-
var bbsClient = new BbsClient(_octoLogger, httpClient, _versionProvider, _retryPolicy);
40+
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("Bitbucket Server");
41+
var bbsClient = new BbsClient(_octoLogger, httpClient, _versionProvider, clientRetryPolicy);
4042
return new BbsApi(bbsClient, bbsServerUrl, _octoLogger);
4143
}
4244
}

0 commit comments

Comments
 (0)