-
Notifications
You must be signed in to change notification settings - Fork 90
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
Post the response from the endpoint when log file posting succeed or failed #2771
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,7 +76,7 @@ public static SarifLog Load(Stream source, bool deferred = false) | |
/// <param name="filePath"></param> | ||
/// <param name="fileSystem"></param> | ||
/// <param name="httpClient"></param> | ||
/// <returns>If the SarifLog has been posted.</returns> | ||
/// <returns>If the SarifLog has been posted successfully.</returns> | ||
public static async Task<bool> Post(Uri postUri, | ||
string filePath, | ||
IFileSystem fileSystem, | ||
|
@@ -101,8 +101,16 @@ public static async Task<bool> Post(Uri postUri, | |
return false; | ||
} | ||
|
||
await Post(postUri, new MemoryStream(fileBytes), httpClient); | ||
Console.WriteLine($"Posted log file successfully to: {postUri}"); | ||
HttpResponseMessage response = await Post(postUri, new MemoryStream(fileBytes), httpClient); | ||
string responseText = await response.Content.ReadAsStringAsync(); | ||
|
||
if (!response.IsSuccessStatusCode) | ||
{ | ||
Console.WriteLine($"Error Posting log file to: {postUri}. Endpoint provided status code {response.StatusCode} and message: {responseText}"); | ||
return false; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems better to check response.IsSuccessStatusCode. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is, as opposed to doing a try/catch here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And I also think status code is useful to user, I think we should add it to the output as well |
||
Console.WriteLine($"Posted log file successfully to: {postUri}. Endpoint provided status code {response.StatusCode} and message: {responseText}"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I'm realizing we should revisit this code soon. We probably shouldn't be writing directly to the Console, and instead refactor the APIs at this layer and a few more up above to pass a Context down, so we can access a proper logger. I'll leave it for Michael to decide if we need that in this change. I can see cases in both directions. |
||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what the format of the responseText is going to be and what it contains. Is it possible to run this locally and provide a sample in the PR description? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is generic, |
||
} | ||
|
||
|
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.
copied the code section,
but made some changes to the message, please review.
I used generic form: "Endpoint provided message"