[feat]: SDKs for ActionsArtifacts APIs #2782
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nickfloyd
approved these changes
Sep 25, 2023
There was a problem hiding this comment.
This looks great @thomhurst, just a little nit about some code doc clarity... it's definitely not super important, but it would be some nice cleanup.
This looks fantastic otherwaise, thank you for the effort here! ❤️
| .Any(item => item.Equals(contentType, StringComparison.OrdinalIgnoreCase)))) | ||
| { | ||
| responseBody = await responseMessage.Content.ReadAsByteArrayAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
I agree with the preference of using Stream instead of ByteArray here. Given that this has changed would you mind updating the related code docs around the Response implementation as well, just for clarity sake.
nickfloyd
changed the title
1 task
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #2780
Before the change?
After the change?
Methods to interact with the APIs listed here: https://docs.github.com/en/rest/actions/artifacts
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!
I couldn't add integration tests as there isn't any API to create artifacts or anything. But I've added a deserialization test. And then I've hooked into the existing HTTP functionality which should already be thoroughly tested.
I also slightly reworked the internal HTTP sending to receive binary file responses as Streams, rather than raw byte arrays. Artifacts could potentially be large file sizes, and loading these directly into a byte array may not be ideal for memory. So by utilising streams instead, we can reduce memory and write to the disk as we receive data.