RetryPolicy: include service name in 401 Unauthorized error messages#1532
RetryPolicy: include service name in 401 Unauthorized error messages#1532
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the shared RetryPolicy so that 401 Unauthorized errors include a service identifier, making credential failures easier to diagnose across the Octoshift CLI’s various source/target integrations.
Changes:
- Added optional
serviceNamesupport toRetryPolicy(plusWithServiceName) and included it in 401 Unauthorized exception messages. - Updated production factories (ADO/BBS/GitHub) to create service-tagged
RetryPolicyinstances for their clients instead of using an untagged singleton. - Updated integration tests to pass service-tagged retry policies into manually constructed clients/APIs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Octoshift/RetryPolicy.cs | Adds service name threading and includes it in 401 Unauthorized messages. |
| src/Octoshift/Factories/GithubApiFactory.cs | Creates a GitHub-tagged retry policy for the GithubClient (but currently not applied consistently to API/uploader). |
| src/ado2gh/Factories/AdoApiFactory.cs | Tags retry policy for Azure DevOps client usage. |
| src/ado2gh/Factories/AdoPipelineTriggerServiceFactory.cs | Tags retry policy for Azure DevOps client usage in pipeline trigger path. |
| src/bbs2gh/Factories/BbsApiFactory.cs | Tags retry policy for Bitbucket Server client usage (basic + kerberos). |
| src/OctoshiftCLI.IntegrationTests/AdoToGithub.cs | Passes tagged retry policies into ADO/GitHub clients/APIs in integration tests. |
| src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs | Passes tagged retry policies into BBS/GitHub clients/APIs in integration tests. |
| src/OctoshiftCLI.IntegrationTests/GhesToGithub.cs | Passes tagged retry policies into GHES/GitHub clients/APIs in integration tests. |
| src/OctoshiftCLI.IntegrationTests/GithubToGithub.cs | Passes tagged retry policies into GitHub client/API in integration tests. |
Comments suppressed due to low confidence (2)
src/Octoshift/Factories/GithubApiFactory.cs:60
- Same issue in the target GitHub factory path:
ArchiveUploaderandGithubApiare still receiving_retryPolicyinstead of the service-taggedclientRetryPolicy, so unauthorized errors from API/uploader retries won't include which service/credential failed. PassclientRetryPolicythrough here as well.
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub");
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, clientRetryPolicy, _dateTimeProvider, targetPersonalAccessToken);
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider);
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
}
src/Octoshift/Factories/GithubApiFactory.cs:49
- Same issue as above:
clientRetryPolicyis service-tagged, but_retryPolicyis still passed intoArchiveUploader/GithubApi, so 401 Unauthorized errors originating from those components won't include the service context. UseclientRetryPolicyconsistently for the uploader and API instance created here.
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub");
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("NoSSL"), _versionProvider, clientRetryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider);
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
}
| var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub"); | ||
| var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, clientRetryPolicy, _dateTimeProvider, sourcePersonalAccessToken); | ||
| var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider); | ||
| return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader); | ||
| } |
There was a problem hiding this comment.
clientRetryPolicy is created with a service name, but ArchiveUploader and GithubApi are still constructed with _retryPolicy. Any 401s thrown from _retryPolicy.Retry/HttpRetry inside GithubApi/ArchiveUploader will therefore not include the service name (and this also contradicts the PR goal of using per-service policies). Pass clientRetryPolicy to ArchiveUploader and GithubApi here so all GitHub operations use the tagged policy.
This issue also appears in the following locations of the same file:
- line 45
- line 56
f31a39a to
3ea44c3
Compare
Unit Test Results 1 files 1 suites 10m 25s ⏱️ Results for commit 9b1a27b. ♻️ This comment has been updated with latest results. |
Thread an optional serviceName parameter through RetryPolicy so that 401 Unauthorized errors identify which service/credential failed (e.g. "Unauthorized (Azure DevOps). Please check your token and try again."). Production factories (GithubApiFactory, AdoApiFactory, AdoPipelineTriggerServiceFactory, BbsApiFactory) now create per-service RetryPolicy instances instead of sharing the DI singleton. Integration tests also wire service names through their manually-constructed clients.
3ea44c3 to
9b1a27b
Compare
jfine
left a comment
There was a problem hiding this comment.
Unless you want to address what
noted it looks good to me!
Thread an optional serviceName parameter through RetryPolicy so that 401 Unauthorized errors identify which service/credential failed (e.g. "Unauthorized (Azure DevOps). Please check your token and try again.").
Production factories (GithubApiFactory, AdoApiFactory, AdoPipelineTriggerServiceFactory, BbsApiFactory) now create per-service RetryPolicy instances instead of sharing the DI singleton. Integration tests also wire service names through their manually-constructed clients.
ThirdPartyNotices.txt(if applicable)