diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 35e95e0e0..2359c5c29 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -1,2 +1,3 @@ - Added validation to detect and return clear error messages when a URL is provided instead of a name for organization, repository, or enterprise arguments (e.g., `--github-org`, `--github-target-org`, `--source-repo`, `--github-target-enterprise`) +- Fixed an issue where repository existence checks would incorrectly retry on expected responses (200/404/301), causing unnecessary delays during migrations. The check now only retries on transient server errors (5xx status codes) while responding immediately to deterministic states. - Added `--target-api-url` as an optional arg to the `add-team-to-repo` command diff --git a/src/Octoshift/Services/GithubClient.cs b/src/Octoshift/Services/GithubClient.cs index 1dd86089b..128fa3b75 100644 --- a/src/Octoshift/Services/GithubClient.cs +++ b/src/Octoshift/Services/GithubClient.cs @@ -45,7 +45,13 @@ public GithubClient(OctoLogger log, HttpClient httpClient, IVersionProvider vers } } - public virtual async Task GetNonSuccessAsync(string url, HttpStatusCode status) => (await GetWithRetry(url, expectedStatus: status)).Content; + public virtual async Task GetNonSuccessAsync(string url, HttpStatusCode status) + { + return (await _retryPolicy.HttpRetry( + async () => await SendAsync(HttpMethod.Get, url, expectedStatus: status), + ex => ex.StatusCode.HasValue && (int)ex.StatusCode.Value >= 500 + )).Content; + } public virtual async Task GetAsync(string url, Dictionary customHeaders = null) => (await GetWithRetry(url, customHeaders)).Content; diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs index 506daad70..3032de6a3 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs @@ -1063,27 +1063,150 @@ public async Task GetNonSuccessAsync_Logs_The_GitHub_Request_Id_Header_Value() } [Fact] - public async Task GetNonSuccessAsync_Retries_On_Non_Success() + public async Task GetNonSuccessAsync_Does_Not_Retry_On_Expected_Status() { // Arrange var handlerMock = new Mock(); handlerMock .Protected() - .SetupSequence>( + .Setup>( "SendAsync", ItExpr.Is(req => req.Method == HttpMethod.Get), ItExpr.IsAny()) - .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.InternalServerError)) - .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.Found, content: EXPECTED_RESPONSE_CONTENT)); + .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.NotFound, content: "Not Found")); using var httpClient = new HttpClient(handlerMock.Object); var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN); // Act - var result = await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.Found); + var result = await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound); + + // Assert + result.Should().Be("Not Found"); + handlerMock.Protected().Verify( + "SendAsync", + Times.Once(), + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()); + } + + [Fact] + public async Task GetNonSuccessAsync_Does_Not_Retry_On_Unexpected_Success_Status() + { + // Arrange + var handlerMock = new Mock(); + handlerMock + .Protected() + .Setup>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.OK, content: "Success")); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN); + + // Act & Assert - should throw immediately without retry + await FluentActions + .Invoking(async () => await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound)) + .Should() + .ThrowExactlyAsync() + .WithMessage("*OK*"); + + handlerMock.Protected().Verify( + "SendAsync", + Times.Once(), + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()); + } + + [Fact] + public async Task GetNonSuccessAsync_Retries_On_Server_Error() + { + // Arrange + var handlerMock = new Mock(); + handlerMock + .Protected() + .SetupSequence>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.InternalServerError, content: "Server Error")) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.NotFound, content: "Not Found")); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN); + + // Act - should retry on 5xx and eventually succeed + var result = await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound); // Assert - result.Should().Be(EXPECTED_RESPONSE_CONTENT); + result.Should().Be("Not Found"); + handlerMock.Protected().Verify( + "SendAsync", + Times.Exactly(2), + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()); + } + + [Fact] + public async Task GetNonSuccessAsync_Does_Not_Retry_On_Client_Error() + { + // Arrange + var handlerMock = new Mock(); + handlerMock + .Protected() + .Setup>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.BadRequest, content: "Bad Request")); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN); + + // Act & Assert - should throw immediately without retry on 4xx + await FluentActions + .Invoking(async () => await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound)) + .Should() + .ThrowExactlyAsync() + .WithMessage("*BadRequest*"); + + handlerMock.Protected().Verify( + "SendAsync", + Times.Once(), + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()); + } + + [Fact] + public async Task GetNonSuccessAsync_Does_Not_Retry_On_Redirect_Status() + { + // Arrange + var handlerMock = new Mock(); + handlerMock + .Protected() + .Setup>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.MovedPermanently, content: "Moved")); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN); + + // Act & Assert - should throw immediately without retry on redirect + await FluentActions + .Invoking(async () => await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound)) + .Should() + .ThrowExactlyAsync() + .WithMessage("*MovedPermanently*"); + + handlerMock.Protected().Verify( + "SendAsync", + Times.Once(), + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()); } [Fact]