Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix target repo existence check in GHES migrations so it doesn't explode if the target repo used to exist, but has been renamed #1084

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

timrogers
Copy link
Contributor

@timrogers timrogers commented Jul 25, 2023

When migrating from GHES with migrate-repo, we perform a quick validation check before starting the export to ensure that the target repo doesn't already exist. If it does already exist, we can fail fast and not run the export at all.

If the repo used to exist but has been renamed, then this causes the CLI to error, because the REST API returns 301 Moved Permanently and that's not something we handle or expect.

This updates our code to handle 301 Moved Permanently and treat it correctly as "the repo doesn't exist".

Fixes #1074.

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

…ode if the target repo used to exist, but has been renamed

When migrating from GHES with `migrate-repo`, we perform a quick
validation check before starting the export to ensure that the
target repo doesn't already exist. If it does already exist, we
can fail fast and not run the export at all.

If the repo used to exist but has been renamed, then this causes
the CLI to error, because the REST API returns `301 Moved
Permanently` and that's not something we handle or expect.

This updates our code to handle `301 Moved Permanently` and treat
it correctly as "the repo doesn't exist".

Fixes #1074.
@timrogers timrogers self-assigned this Jul 25, 2023
@@ -158,6 +158,10 @@ public virtual async Task<bool> DoesRepoExist(string org, string repo)
{
return true;
}
catch (HttpRequestException ex) when (ex.StatusCode == HttpStatusCode.MovedPermanently)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative approach would have been to update GetNonSuccessAsync (see lines above) to allow multiple "allowed HTTP statuses". But that turned out to be reasonably complex, and I'm not sure how likely it is that we'll need this again, so I took the easy way out.

@timrogers
Copy link
Contributor Author

I considered covering this in the tests for OctoshiftCLI.GithubEnterpriseImporter.Commands.MigrateRepo.MigrateRepoCommandHandler too, but I decided not to as this is a very small change and I trust the unit tests. But I'm open to changing my approach here.

@github-actions
Copy link

Unit Test Results

788 tests   788 ✔️  24s ⏱️
    1 suites      0 💤
    1 files        0

Results for commit 41cfb7e.

@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
Octoshift 86% 75% 1216
bbs2gh 79% 75% 638
ado2gh 84% 82% 617
gei 80% 74% 528
Summary 83% (6596 / 7925) 76% (1533 / 2004) 2999

Copy link
Contributor

@hfishback01 hfishback01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉

I'm fine looking specifically for the MovedPermanently as opposed to getting a NonSuccess.

@timrogers timrogers merged commit 504837d into main Jul 25, 2023
30 checks passed
@timrogers timrogers deleted the timrogers/fix-1074 branch July 25, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support migrating to repository already renamed
2 participants