Initial commit #1261
Initial commit #1261
Conversation
Unit Test Results813 tests 813 ✅ 22s ⏱️ Results for commit 56931b4. ♻️ This comment has been updated with latest results. |
![github-advanced-security[bot]](https://avatars-ghuser.fzsz.win/in/57789?s=60&v=4){kind=small}
| @@ -92,11 +93,6 @@ public override void Validate(OctoLogger log) | |||
|
|
|||
| private void ValidateNoGenerateOptions() | |||
| { | |||
| if (BbsUsername.HasValue() || BbsPassword.HasValue()) | |||
There was a problem hiding this comment.
Removing this, I don't think it should be checked for with this change
There was a problem hiding this comment.
the check should probably still exist but the error message needs to be changed because this is validating the args for a no generate scenario where the cli doesn't generate the archive and the archive path or url is is provided.
That being said I'd recommend revising the error messages in a separate PR because there is more than just this one, for example the one on line 105 says "SSH or SMB download options can only be provided with --bbs-server-url." but based on the same logic the server url should always be provided.
There was a problem hiding this comment.
Since we now require the bbs-server-url, we always check for a username and password, even in the non-generate archive case.
--bbs-server-url enables the create of the bbsApiFactory, which then does the check for username and password.
| @@ -186,44 +206,6 @@ public void It_Throws_When_Kerberos_Is_Set_And_Bbs_Username_Is_Provided() | |||
| .WithMessage("*--bbs-username*--kerberos*"); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void Errors_If_BbsServer_Url_Not_Provided_But_Bbs_Username_Is_Provided() | |||
There was a problem hiding this comment.
Removing these tests as bbs-server-url is now required.
| @@ -198,6 +200,22 @@ private async Task<string> UploadArchiveToAws(string bucketName, string archiveP | |||
| return archiveBlobUrl; | |||
| } | |||
|
|
|||
| private async Task<string> UploadArchiveToGithub(string org, string archivePath) | |||
| { | |||
| #pragma warning disable IDE0063 | |||
There was a problem hiding this comment.
Is there a reason the warning was disabled? I think we can use using declaration here.
src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs
Outdated
Show resolved
Hide resolved
src/OctoshiftCLI.Tests/gei/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs
Outdated
Show resolved
Hide resolved
| { httpContent, "archive", archiveName } | ||
| }; | ||
|
|
||
| response = await _client.PostAsync(url, content); |
There was a problem hiding this comment.
A couple of observations:
- Even though this may work the
PostAsyncmethod will convert the data to JSON (basically base64 string) and will eventually createStringContentso I am not sure if this is the indented use here because we're now dealing with aStreamrather than string content. So We may want to refactor the SendAsync method to also support aMultiPartFormDataContentandStreamContent. It can simply check to see if the passed inbodyis either of those and not convert it to a string content. - When dealing with a stream we shouldn't log the entire body as we do here instead as I suggested in option 1, we can check the body and if it's a multipart form data or a stream content we can just say
BLOBorBinary datainstead of dumping the entire JSON encoded body!
|
Check in BBS MigrateRepoCommandArgs removed, relating to changes in this PR: #1057
Note: BBS Integration failures are related to this unrelated issue
ThirdPartyNotices.txt(if applicable)