-
Notifications
You must be signed in to change notification settings - Fork 82
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
Initial commit #1261
base: main
Are you sure you want to change the base?
Initial commit #1261
Conversation
Unit Test Results813 tests 813 ✅ 22s ⏱️ Results for commit 56931b4. ♻️ This comment has been updated with latest results. |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this, I don't think it should be checked for with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped a couple more comments.
{ httpContent, "archive", archiveName } | ||
}; | ||
|
||
response = await _client.PostAsync(url, content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of observations:
- Even though this may work the
PostAsync
method will convert the data to JSON (basically base64 string) and will eventually createStringContent
so I am not sure if this is the indented use here because we're now dealing with aStream
rather than string content. So We may want to refactor the SendAsync method to also support aMultiPartFormDataContent
andStreamContent
. It can simply check to see if the passed inbody
is 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
BLOB
orBinary data
instead 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)