Refresh GitHub git archive URLs if they have expired#1439
Conversation
- Add tests
dpmex4527
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds retry logic for expired archive URLs during GitHub migrations. When download URLs expire (indicated by 403 errors), the system now regenerates fresh URLs and retries the download operation.
- Implements automatic retry logic for both git and metadata archive downloads when 403 errors occur
- Adds comprehensive test coverage for retry scenarios including individual and combined archive failures
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs | Added try-catch blocks around archive downloads to handle 403 errors and regenerate fresh URLs |
| src/OctoshiftCLI.Tests/gei/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs | Added three test methods covering git archive retry, metadata archive retry, and both archives retry scenarios |
- Fix lint errors
Unit Test Results 1 files 1 suites 10m 24s ⏱️ Results for commit c418f8b. ♻️ This comment has been updated with latest results. |
- Check error against exception status code
dpmex4527
force-pushed
the
dpmex4527/refresh-archive-urls-if-expired
branch
from
c56d8be to
fb2205b
Compare
- Add tests
- Gracefully handle empty results when fetching IDP groups - Add tests
synthead
left a comment
synthead
left a comment
There was a problem hiding this comment.
Awesome work here!! This will be very helpful when customers enqueue a lot of migrations!
| // Assert | ||
| // Verify that GetArchiveMigrationUrl was called twice for git archive (once during generation, once during retry) | ||
| _mockSourceGithubApi.Verify(x => x.GetArchiveMigrationUrl(SOURCE_ORG, GIT_ARCHIVE_ID), Times.Exactly(2)); | ||
|
|
||
| // Verify that DownloadToFile was called twice for git archive (original URL failed, fresh URL succeeded) | ||
| _mockHttpDownloadService.Verify(x => x.DownloadToFile(GIT_ARCHIVE_URL, GIT_ARCHIVE_FILE_PATH), Times.Once); | ||
| _mockHttpDownloadService.Verify(x => x.DownloadToFile(freshGitArchiveUrl, GIT_ARCHIVE_FILE_PATH), Times.Once); | ||
|
|
||
| // Verify metadata archive was only downloaded once (no retry needed) | ||
| _mockHttpDownloadService.Verify(x => x.DownloadToFile(METADATA_ARCHIVE_URL, METADATA_ARCHIVE_FILE_PATH), Times.Once); |
|
Oh also, one more idea you might entertain is using theories, like so: gh-gei/src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs Lines 71 to 76 in a723ac3 They work a lot like RSpec's shared examples. Since there's a lot of overlap between the metadata and Git archive URL tests, you might be able to say "this URL returns a 403 first", and pass a URL into your mocks. This could possibly clean up your tests in a DRY way, but keep an eye out for making them too complex with abstractions, too 👍 |
This reverts commit 9638298.
|
This PR fixes an issue when sometimes for GitHub migrations the metadata export takes a long time and it causes the gitArchiveUrl that was generated right before starting with the metadata export to expire. What we now do is check to see if the git archive URL expires (by catching 403 errors for migrations backed by Azure/S3 or 404 errors when backed by GHES local storage) and if so, re-fresh the download URL to allow the download to continue.
ThirdPartyNotices.txt(if applicable)