feat: Add UploadReleaseAssetFromRelease convenience helper #3851
Conversation
… release.UploadURL (fixes google#3849)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3851 +/- ##
==========================================
- Coverage 92.46% 92.27% -0.19%
==========================================
Files 199 199
Lines 14240 14339 +99
==========================================
+ Hits 13167 13232 +65
- Misses 884 901 +17
- Partials 189 206 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gmlewis
changed the title
|
I wonder if "rendering" the template URL by stripping the curly braces is the best approach - should GitHub change their URL and move these variables into e.g. path components it would instantly break. Should one maybe add the dependency for a uritemplate module? |
| // 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) { |
There was a problem hiding this comment.
Using a file *os.File here is unnecessarily restrictive.
Note that the method that this function calls (NewUploadRequest) takes:
func (c *Client) NewUploadRequest(urlStr string, reader io.Reader, size int64, mediaType string, opts ...RequestOption) (*http.Request, error) {
which allows uploading of data from a much more flexible range of sources.
Please adopt the same API in this helper method.
|
Thanks for the feedback! Regarding the URI-template approach, I’m open to implementing that as well if the maintainers prefer it. Thanks also for the note about accepting a more flexible input than *os.File. I’ll update the helper to match the NewUploadRequest API (io.Reader + size) and adjust the tests accordingly. If there are any other suggestions or preferred directions, I’m happy to update the PR. |
|
Also, please see if you can improve the code coverage stats by taking advantage of some of the test helper functions you see being used in other tests like |
|
Got it. I’ll use the existing helpers (like testBadOptions) to improve coverage. Working on it. |
| // 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.
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 Client.WithEnterpriseURLs().
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).
There was a problem hiding this comment.
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
Client.WithEnterpriseURLs().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).
@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.
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 .goreleaser.yaml in the forked project to enable goreleaser to publish the release and upload the binaries. This than means, that in order to contribute changes made in our GHES back to the upstream project we must always strip the commits changing the goreleaser configuration, so we do not break upstreams build process or leak internal information.
However since the workflow is running in GitHub Actions the v3 API endpoint for the GHES is provided in the workflow's github context, so goreleaser and/or goreleaser-action have theoretically all information needed in order to work without explicit configuration.
But for the Upload API this case is different:
- the API endpoint is not officially disclosed in the documentation, and only listed in the examples
- the API endpoint is however returned almost fully formed by the v3 API when obtaining a release (either by fetching or creating it)
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.
There was a problem hiding this comment.
@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.
If not, please let me know.
4 participants
This PR adds a new convenience method:
Why?
GitHub's REST API returns the
upload_urldirectly inside theRepositoryReleaseobject.Currently, uploading a release asset requires callers to manually specify:
This helper solves that by allowing users to upload an asset directly using the
release.UploadURLreturned from the API.What this PR does
UploadReleaseAssetFromReleaseinrepos_releases.go{?name,label}template fromupload_urlbefore uploading//meta:operationdirective so the codegen pipeline works correctlyTests and validation
script/test.sh)script/fmt.sh)script/generate.sh)script/lint.sh)openapi_operations.yamlReference
Fixes #3849
Thanks!