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

PART 3: Add UploadArchiveToGithubStorageWithMultiPart method #1271

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

begonaguereca
Copy link
Contributor

@begonaguereca begonaguereca commented Sep 25, 2024

This pull request introduces a new feature for uploading archives to GitHub storage and includes corresponding unit tests.

We are also updating the content types of Stream and Multipart uploads in the SendAsync method.

  • 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)

Copy link

github-actions bot commented Sep 25, 2024

Unit Test Results

815 tests   815 ✅  21s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit cd88309.

♻️ This comment has been updated with latest results.

@begonaguereca begonaguereca marked this pull request as ready for review September 30, 2024 19:05
@begonaguereca begonaguereca marked this pull request as draft October 7, 2024 19:31
@begonaguereca begonaguereca marked this pull request as ready for review October 8, 2024 17:29
@begonaguereca begonaguereca marked this pull request as draft October 8, 2024 17:40
@begonaguereca begonaguereca changed the title PART 3: Add UploadArchiveToGithubStorageWithMultiPart method PART 4: Add UploadArchiveToGithubStorageWithMultiPart method Oct 8, 2024
@begonaguereca begonaguereca changed the title PART 4: Add UploadArchiveToGithubStorageWithMultiPart method PART 3: Add UploadArchiveToGithubStorageWithMultiPart method Oct 8, 2024
@begonaguereca begonaguereca marked this pull request as ready for review October 8, 2024 17:48
Copy link

github-actions bot commented Oct 8, 2024

Code Coverage

Package Line Rate Branch Rate Complexity Health
gei 79% 70% 519
ado2gh 84% 78% 627
Octoshift 87% 76% 1288
bbs2gh 78% 73% 651
Summary 83% (6860 / 8218) 75% (1541 / 2056) 3085

@begonaguereca begonaguereca changed the base branch from main to git-storage-option-feature October 8, 2024 19:15
@begonaguereca begonaguereca merged commit cd6311a into git-storage-option-feature Oct 8, 2024
30 checks passed
@begonaguereca begonaguereca deleted the github-api-upload branch October 8, 2024 19:37
Copy link
Collaborator

@ArinGhazarian ArinGhazarian left a comment

Choose a reason for hiding this comment

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

Dropped some comments mostly around simplifying the code and unit tests. Tests need to be added for GithubClient too

Comment on lines +1084 to +1087
using var content = new MultipartFormDataContent
{
{ httpContent, "archive", archiveName }
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Initializing a using variable with an object initializer might lead to problems if an exception is thrown during the initialization and the using variable may never get disposed so here it's better to add the content with the Add() method:

Suggested change
using var content = new MultipartFormDataContent
{
{ httpContent, "archive", archiveName }
};
using var content = new MultipartFormDataContent();
content.Add(streamContent, "archive", archiveName);

{
var url = $"https://uploads.github.com/organizations/{orgDatabaseId.EscapeDataString()}/gei/archive?name={archiveName.EscapeDataString()}";

response = await _client.PostAsync(url, httpContent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

renaming httpContent to streamContent:

Suggested change
response = await _client.PostAsync(url, httpContent);
response = await _client.PostAsync(url, streamContent);

@@ -1071,6 +1072,33 @@ mutation abortRepositoryMigration(
}
}

public virtual async Task<string> UploadArchiveToGithubStorage(string orgDatabaseId, bool isMultipart, string archiveName, Stream archiveContent)
{
using var httpContent = new StreamContent(archiveContent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I would call this variable streamContent for clarity as both stream and multipart from data are technically HttpContent"

Suggested change
using var httpContent = new StreamContent(archiveContent);
using var streamContent = new StreamContent(archiveContent);

Comment on lines +165 to +172
_log.LogVerbose(body is MultipartFormDataContent or StreamContent ? "HTTP BODY: BLOB" : $"HTTP BODY: {body.ToJson()}");

request.Content = body.ToJson().ToStringContent();
request.Content = body switch
{
MultipartFormDataContent multipartFormDataContent => multipartFormDataContent,
StreamContent streamContent => streamContent,
_ => body.ToJson().ToStringContent()
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking this can be simplified as we now either support a HttpContent or a JSON object (it cannot let me add a code suggestion):

            if (body is HttpContent httpContent)
            {
                _log.LogVerbose("HTTP BODY: BLOB");
                request.Content = httpContent;
            }
            else
            {
                var jsonBody = body.ToJson();
                _log.LogVerbose($"HTTP BODY: {jsonBody}");
                request.Content = jsonBody.ToStringContent();
            }

@@ -3462,6 +3463,62 @@ await _githubApi.Invoking(api => api.AbortMigration(migrationId))
.WithMessage(expectedErrorMessage);
}

[Fact]
public async Task UploadArchiveToGithubStorage()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public async Task UploadArchiveToGithubStorage()
public async Task UploadArchiveToGithubStorage_Should_Upload_Stream_Content()

var jsonResponse = $"{{ \"archiveId\": \"{expectedArchiveId}\" }}";

_githubClientMock
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<HttpContent>(), null))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make sure that PostAsync receives a StreamContent:

Suggested change
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<HttpContent>(), null))
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<StreamContent>(), null))



[Fact]
public async Task UploadArchiveToGithubStorageWithMultiPart()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use snaked cased test names throughout the project:

Suggested change
public async Task UploadArchiveToGithubStorageWithMultiPart()
public async Task UploadArchiveToGithubStorage_Should_Upload_Multipart_Content()

var jsonResponse = $"{{ \"archiveId\": \"{expectedArchiveId}\" }}";

_githubClientMock
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<HttpContent>(), null))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<HttpContent>(), null))
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<MultipartFormDataContent>(), null))

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.

2 participants