RetryPolicy: include service name in 401 Unauthorized error messages#1532
Open
RetryPolicy: include service name in 401 Unauthorized error messages#1532
Conversation
Contributor
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);
}
Comment on lines
+34
to
38
| 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
offbyone
force-pushed
the
o1/push-lqwlqnrruqpk
branch
from
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.
offbyone
force-pushed
the
o1/push-lqwlqnrruqpk
branch
from
3ea44c3 to
9b1a27b
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)