-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add UploadReleaseAssetFromRelease convenience helper #3851
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
Changes from 3 commits
ed70edb
69a2c8b
a7a6dcb
501de8f
5f2fc0c
e29988e
072f1f5
0f7fc4d
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 |
|---|---|---|
|
|
@@ -488,3 +488,55 @@ func (s *RepositoriesService) UploadReleaseAsset(ctx context.Context, owner, rep | |
| } | ||
| return asset, resp, nil | ||
| } | ||
|
|
||
| // UploadReleaseAssetFromRelease uploads an asset using the UploadURL that's embedded | ||
| // in a RepositoryRelease object. | ||
| // | ||
| // This is a convenience wrapper that extracts the release.UploadURL (which is usually | ||
| // templated like "https://uploads.github.com/.../assets{?name,label}") and uploads | ||
| // the provided data (reader + size) using the existing upload helpers. | ||
| // | ||
| // GitHub API docs: https://docs.github.com/rest/releases/assets#upload-a-release-asset | ||
| // | ||
| //meta:operation POST /repos/{owner}/{repo}/releases/{release_id}/assets | ||
| func (s *RepositoriesService) UploadReleaseAssetFromRelease(ctx context.Context, release *RepositoryRelease, opts *UploadOptions, reader io.Reader, size int64) (*ReleaseAsset, *Response, error) { | ||
|
Contributor
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 don't understand why we need to pass the entire func (s *RepositoriesService) UploadReleaseAssetFromRelease(ctx context.Context, uploadURL string, opts *UploadOptions, reader io.Reader, size int64) (*ReleaseAsset, *Response, error) {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 would then mean that the consumer needs to know that the upload URL needs to be extracted from a The implemented way makes it trivial for users - they know that they need to supply a release, and probably already have one at hand from creating it.
Contributor
Author
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. Pushed the final updates. absolute upload URLs are preserved, template trimming is correct, media-type handling now matches UploadReleaseAsset, and tests fully cover all paths. Everything passes locally. Let me know if you'd like any further tweaks! |
||
| if release == nil || release.UploadURL == nil { | ||
| return nil, nil, errors.New("release UploadURL must be provided") | ||
| } | ||
| if reader == nil { | ||
| return nil, nil, errors.New("reader must be provided") | ||
| } | ||
| if size < 0 { | ||
| return nil, nil, errors.New("size must be >= 0") | ||
| } | ||
|
|
||
| // Strip URI-template portion (e.g. "{?name,label}") if present. | ||
| uploadURL := *release.UploadURL | ||
| if idx := strings.Index(uploadURL, "{"); idx != -1 { | ||
| uploadURL = uploadURL[:idx] | ||
| } | ||
|
|
||
| // IMPORTANT: preserve absolute upload URLs. Do NOT convert to relative paths. | ||
| // addOptions will append name/label query params (same behavior as UploadReleaseAsset). | ||
| u, err := addOptions(uploadURL, opts) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| mediaType := defaultMediaType | ||
| if opts != nil && opts.MediaType != "" { | ||
| mediaType = opts.MediaType | ||
| } | ||
|
|
||
| req, err := s.client.NewUploadRequest(u, reader, size, mediaType) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| asset := new(ReleaseAsset) | ||
| resp, err := s.client.Do(ctx, req, asset) | ||
| if err != nil { | ||
| return nil, resp, err | ||
| } | ||
| return asset, resp, nil | ||
| } | ||
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.
Maybe we can add a new example that demonstrates how to use this wrapper?