Skip to content

Commit

Permalink
Fixed DoesOrgExist to not retry when org exists (#952)
Browse files Browse the repository at this point in the history
<!--
For the checkboxes below you must check each one to indicate that you
either did the relevant task, or considered it and decided there was
nothing that needed doing
-->

- [x] Did you write/update appropriate tests
- [x] Release notes updated (if appropriate)
- [x] Appropriate logging output
- [x] Issue linked
- [x] Docs updated (or issue created)
- [x] New package licenses are added to `ThirdPartyNotices.txt` (if
applicable)

Fixes #944 

`GithubApi.DoesOrgExist()` was working, but it was unnecessarily
retrying 5 times when the org did exist before finally returning the
correct response of `true`.

It was likely written this way because it was copied from the
implementation of `DoesRepoExist()`.

The difference is when we check for the existence of the target repo we
expect it to NOT exist (and if it does that API call will get retried 5
times probably unnecessarily, but should be rare enough that it's fine).
But when checking for the target org we expect that it DOES exist, and
don't want to retry in the case of success.

<!--
For docs we should review the docs at:

https://docs.github.com/en/early-access/github/migrating-with-github-enterprise-importer
and the README.md in this repo

If a doc update is required based on the changes in this PR, it is
sufficient to create an issue and link to it here. The doc update can be
made later/separately.

The process to update the docs can be found here:
https://github.com/github/docs-early-access#opening-prs

The markdown files are here: 

https://github.com/github/docs-early-access/tree/main/content/github/migrating-with-github-enterprise-importer
-->
  • Loading branch information
dylan-smith authored Apr 11, 2023
1 parent 96a3433 commit b2d2709
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 11 deletions.
2 changes: 1 addition & 1 deletion RELEASENOTES.md
Original file line number Diff line number Diff line change
@@ -1 +1 @@

- Fixed a bug when migrating from GHES where we would do a bunch of unnecessary retries at the start making things slower than they needed to be
8 changes: 4 additions & 4 deletions src/Octoshift/Services/GithubApi.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,12 @@ public virtual async Task<bool> DoesOrgExist(string org)
var url = $"{_apiUrl}/orgs/{org.EscapeDataString()}";
try
{
await _client.GetNonSuccessAsync(url, HttpStatusCode.NotFound);
return false;
await _client.GetAsync(url);
return true;
}
catch (HttpRequestException ex) when (ex.StatusCode == HttpStatusCode.OK)
catch (HttpRequestException ex) when (ex.StatusCode == HttpStatusCode.NotFound)
{
return true;
return false;
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -348,12 +348,12 @@ await FluentActions
}

[Fact]
public async Task DoesTargetOrgExist_Returns_True_When_200()
public async Task DoesOrgExist_Returns_True_When_200()
{
// Arrange
var url = $"https://api.github.com/orgs/{GITHUB_ORG}";

_githubClientMock.Setup(m => m.GetNonSuccessAsync(url, HttpStatusCode.NotFound)).ThrowsAsync(new HttpRequestException(null, null, HttpStatusCode.OK));
_githubClientMock.Setup(m => m.GetAsync(url, null)).ReturnsAsync("OK");

// Act
var result = await _githubApi.DoesOrgExist(GITHUB_ORG);
Expand All @@ -363,12 +363,12 @@ public async Task DoesTargetOrgExist_Returns_True_When_200()
}

[Fact]
public async Task DoesTargetOrgExist_Returns_False_When_404()
public async Task DoesOrgExist_Returns_False_When_404()
{
// Arrange
var url = $"https://api.github.com/orgs/{GITHUB_ORG}";

_githubClientMock.Setup(m => m.GetNonSuccessAsync(url, HttpStatusCode.NotFound)).ReturnsAsync("Not Found");
_githubClientMock.Setup(m => m.GetAsync(url, null)).ThrowsAsync(new HttpRequestException(null, null, HttpStatusCode.NotFound));

// Act
var result = await _githubApi.DoesOrgExist(GITHUB_ORG);
Expand All @@ -378,12 +378,12 @@ public async Task DoesTargetOrgExist_Returns_False_When_404()
}

[Fact]
public async Task DoesTargetOrgExist_Throws_On_Unexpected_Response()
public async Task DoesOrgExist_Throws_On_Unexpected_Response()
{
// Arrange
var url = $"https://api.github.com/orgs/{GITHUB_ORG}";

_githubClientMock.Setup(m => m.GetNonSuccessAsync(url, HttpStatusCode.NotFound)).ThrowsAsync(new HttpRequestException(null, null, HttpStatusCode.Unauthorized));
_githubClientMock.Setup(m => m.GetAsync(url, null)).ThrowsAsync(new HttpRequestException(null, null, HttpStatusCode.Unauthorized));

// Act
await FluentActions
Expand Down

0 comments on commit b2d2709

Please sign in to comment.