Consume URI in PUT response for GitHub-owned storage for GEI #1371
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Unit Test Results 1 files 1 suites 21s ⏱️ Results for commit 3dca950. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the multipart upload flow for GEI archives to consume the URI directly from the PUT response instead of interpolating it from the GUID. The changes implement parsing of a JSON response that contains metadata including the gei://archive/{guid} URI.
- Modifies the
CompleteUploadmethod to parse JSON response and extract the URI - Updates all test cases to mock the new JSON response format
- Removes the dependency on parsing GUID from query parameters and manual URI construction
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Octoshift/Services/ArchiveUploader.cs | Updates CompleteUpload to parse JSON response and return URI directly |
| src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs | Updates test mocks to return JSON response and adds Newtonsoft.Json.Linq import |
|
|
||
| return $"gei://archive/{guid}"; | ||
| return geiUri.ToString(); |
There was a problem hiding this comment.
The variable geiUri is a Uri object, but calling ToString() on it is redundant since the method return type is string. This should return geiUri.ToString() or change the return type to Uri and return geiUri directly.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This suggestion produces this error:
error CS0029: Cannot implicitly convert type 'System.Uri' to 'string' [src/Octoshift/Octoshift.csproj]
| _log.LogInformation("Finished uploading archive"); | ||
|
|
||
| return new Uri((string)responseData["uri"]); |
There was a problem hiding this comment.
The cast to (string) could throw an exception if the 'uri' field is null or not a string. Consider using responseData["uri"]?.ToString() or add null checking to provide a more helpful error message if the expected field is missing.
| return new Uri((string)responseData["uri"]); | |
| var uriString = responseData["uri"]?.ToString(); | |
| if (string.IsNullOrWhiteSpace(uriString)) | |
| { | |
| throw new OctoshiftCliException("The response from the server did not contain a valid 'uri' field."); | |
| } | |
| return new Uri(uriString); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
With our API, it's not possible for a 201 to be returned from uploads and not have a URI in the response.
|
Closes https://github.ghe.com/github/octoshift/issues/10765!
This PR consumes the new JSON response from the PUT "complete" request for the multipart upload flow when uploading archives to GitHub-owned storage for GEI!
This is an example of a response from the PUT response:
{ "guid": "363b2659-b8a3-4878-bfff-eed4bcb54d35", "node_id": "MA_kgDaACQzNjNiMjY1OS1iOGEzLTQ4NzgtYmZmZi1lZWQ0YmNiNTRkMzU", "name": "git-archive.tar.gz", "size": 33287, "uri": "gei://archive/363b2659-b8a3-4878-bfff-eed4bcb54d35", "created_at": "2024-11-13T12:35:45.761-08:00" }With the changes in this PR, the JSON body ☝️ is consumed, and the GEI URI doesn't need to be interpolated anymore:
gh-gei/src/Octoshift/Services/ArchiveUploader.cs
Lines 82 to 85 in f9ee191
Here's an example of these changes at work:
ThirdPartyNotices.txt(if applicable)