-
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 2 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 |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import ( | |
| "io" | ||
| "mime" | ||
| "net/http" | ||
| "net/url" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
@@ -488,3 +489,71 @@ 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 file 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, file *os.File) (*ReleaseAsset, *Response, error) { | ||
|
Collaborator
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. Using a which allows uploading of data from a much more flexible range of sources. Please adopt the same API in this helper method. |
||
| if release == nil || release.UploadURL == nil { | ||
| return nil, nil, errors.New("release UploadURL must be provided") | ||
| } | ||
| if file == nil { | ||
| return nil, nil, errors.New("file must be provided") | ||
| } | ||
|
|
||
| // Extract upload URL. | ||
| uploadURL := *release.UploadURL | ||
|
|
||
| // If uploadURL contains a template, strip it (e.g. "{?name,label}"). | ||
| if idx := strings.Index(uploadURL, "{"); idx != -1 { | ||
| uploadURL = uploadURL[:idx] | ||
| } | ||
|
|
||
| // If uploadURL is absolute (starts with http/https), parse and use only the path. | ||
| if strings.HasPrefix(uploadURL, "http://") || strings.HasPrefix(uploadURL, "https://") { | ||
| if uParsed, err := url.Parse(uploadURL); err == nil { | ||
| uploadURL = uParsed.Path | ||
| } | ||
| } | ||
|
|
||
| // Defensive: always remove any leading '/' so client gets a relative path. | ||
| uploadURL = strings.TrimPrefix(uploadURL, "/") | ||
|
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 was not expecting the conversion of the upload URL to a relative URL - that would make the entire point of using the upload URL from the release needlessly depend on the client upload API URL which might have been set with My intention for the feature request was also to remove the need to set the upload API endpoint manually - since the GitHub core API is easily discoverable in GitHub Actions context, but the upload API is not directly discoverable (which it doesn't need to be, since it is defined by the Release the upload is for).
Collaborator
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.
@klaernie - can you please give a hypothetical example of the flow that you are envisioning so that we have a more clear picture of the issue you are trying to solve? Or maybe just walk through the steps you have in mind using pseudo-code as to how you picture this new helper method to work? 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. Essential the story is quite a short one: I needed to build a project using goreleaser locally (forking to an internal GitHub Enterprise instance). Since the project in question is built with GitHub Actions and the goreleaser action specifically I only needed to supply the right GHA variables for publishing the resulting docker image, so the image ends up in the correct registry. However for creating a release and uploading the binaries to the release it was not that easy. For this I first need to find out the API endpoints of the API on the internal GHES, once for the v3 API and once for the upload API. Both URLs then need to be set in the However since the workflow is running in GitHub Actions the v3 API endpoint for the GHES is provided in the workflow's But for the Upload API this case is different:
To me this means that we (as a library implementing the API) were never supposed to hardcode the Upload API endpoint, but always should have extracted the URL for uploading from the release. And we are sure to definitely need the information about the release anyway, since uploading does not work without knowledge of the GitHub internal release ID - which one can only obtain by fetching a release by name or iterating all releases. To put this in pseudocode this flow is always as such: release, resp, err := client.Repositories.CreateRelease(ctx, owner, repo, &RepositoryRelease{ ... })
for asset := range assets {
... := client.Repositories.UploadReleaseAssetFromRelease(ctx, release, &UploadOptions{Name: asset.name, Label: asset.label}, asset.content)
}I hope this makes the use case a lot clearer, I definitely fell into the trap of assuming too much of my knowledge to be implicitly known instead of making it explicit.
Collaborator
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. @rockygeekz - do you have enough information to proceed, or do you have questions about this? (Please make sure to read the edited version above and not the email to be truly up-to-date.) I, myself, have not dug deeply into the issue yet, but if you two can figure it out, then wonderful... please proceed.
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. Thanks for the detailed explanation - this makes the intended behavior fully clear. I now understand the core requirement:
Based on this, I’ll update the implementation so that:
If you’d like to move to a proper URI-template implementation later, I’m happy to follow up in another PR - for now I’ll align with the clarified behavior above. Working on the updates now. Thanks again for the clarification!
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. UpdateThanks for the feedback! I've updated the implementation based on what you all suggested. What changed:
ResultThis now works as intended for the GHES use-case — the upload endpoint comes directly from the release object without needing Next stepsIf you want any refinements (like using a URI-template library), just let me know and I'll update it. Thanks again! |
||
|
|
||
| // addOptions will append query params for name/label (same as UploadReleaseAsset) | ||
| u, err := addOptions(uploadURL, opts) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| stat, err := file.Stat() | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if stat.IsDir() { | ||
| return nil, nil, errors.New("the asset to upload can't be a directory") | ||
| } | ||
|
|
||
| mediaType := mime.TypeByExtension(filepath.Ext(file.Name())) | ||
| if opts != nil && opts.MediaType != "" { | ||
| mediaType = opts.MediaType | ||
| } | ||
|
|
||
| req, err := s.client.NewUploadRequest(u, file, stat.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 | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.