Skip to content

Conversation

@rockygeekz
Copy link

This PR adds a new convenience method:

UploadReleaseAssetFromRelease(ctx context.Context, release *RepositoryRelease, opts *UploadOptions, file *os.File)

Why?

GitHub's REST API returns the upload_url directly inside the RepositoryRelease object.
Currently, uploading a release asset requires callers to manually specify:

  • owner
  • repo
  • release ID
  • AND to configure a separate Upload API endpoint (especially for GHES)

This helper solves that by allowing users to upload an asset directly using the
release.UploadURL returned from the API.

What this PR does

  • Adds a new method UploadReleaseAssetFromRelease in repos_releases.go
  • Reuses the existing UploadReleaseAsset logic (addOptions, NewUploadRequest, client.Do)
  • Trims the {?name,label} template from upload_url before uploading
  • Includes code comments, documentation URL, and the //meta:operation directive so the codegen pipeline works correctly

Tests and validation

  • All tests passed (script/test.sh)
  • Code formatted (script/fmt.sh)
  • Generated files updated (script/generate.sh)
  • Lint passed (script/lint.sh)
  • No changes needed in openapi_operations.yaml

Reference

Fixes #3849

Thanks!

@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

❌ Patch coverage is 54.83871% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.27%. Comparing base (e279f70) to head (69a2c8b).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
github/repos_releases.go 54.83% 7 Missing and 7 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmlewis gmlewis changed the title repos: add UploadReleaseAssetFromRelease convenience helper feat: Add UploadReleaseAssetFromRelease convenience helper Dec 4, 2025
@klaernie
Copy link

klaernie commented Dec 4, 2025

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) {
Copy link
Collaborator

@gmlewis gmlewis Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rockygeekz
Copy link
Author

Thanks for the feedback!
I’ve added the tests and updated the URL handling as discussed. I’ll now work on improving the patch coverage by adding tests for the remaining branches so Codecov passes.

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.
I’ll push the next set of changes shortly.

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 4, 2025

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 testBadOptions for example.

@rockygeekz
Copy link
Author

Got it. I’ll use the existing helpers (like testBadOptions) to improve coverage. Working on it.

Comment on lines 519 to 527
// 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, "/")
Copy link

Choose a reason for hiding this comment

The 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 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).

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 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?

Copy link

@klaernie klaernie Dec 5, 2025

Choose a reason for hiding this comment

The 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 .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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
If not, please let me know.

Copy link
Author

Choose a reason for hiding this comment

The 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:

  • If the release provides an absolute upload_url, the client should use it as-is, including its host.
  • This avoids depending on Client.WithEnterpriseURLs() and ensures uploading works correctly on GHES, where the upload API endpoint can’t be inferred from the main API endpoint.
  • Only the URI-template portion ({?name,label}) should be handled, not by converting the URL to a relative path, but by making it usable for the upload.

Based on this, I’ll update the implementation so that:

  1. Absolute URLs are preserved completely (no conversion to a relative URL).
  2. Only the template section is handled/trimmed, with no changes to host or path.
  3. The helper will take a flexible input (io.Reader + size) to match NewUploadRequest.
  4. Tests will be updated to reflect this behavior.

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update

Thanks for the feedback! I've updated the implementation based on what you all suggested.

What changed:

  • Switched the helper to accept io.Reader + size (matching NewUploadRequest)
  • Strip only the {?name,label} URI-template portion
  • Preserve absolute upload URLs exactly as the API returns them
  • No path rewriting or conversion to relative URLs
  • Added full test coverage for:
    • absolute URLs
    • template stripping
    • nil release / nil reader
    • negative size
    • no opts
    • media type override
    • bad options
    • testNewRequestAndDoFailure branches

Result

This now works as intended for the GHES use-case — the upload endpoint comes directly from the release object without needing Client.WithEnterpriseURLs().

Next steps

If you want any refinements (like using a URI-template library), just let me know and I'll update it.

Thanks again!

…bsolute upload URLs

This adds a convenience helper that uploads a release asset using the
UploadURL returned in RepositoryRelease.

Changes included:
- Accept io.Reader + size instead of *os.File (consistent with NewUploadRequest)
- Strip only the URI-template portion
- Preserve absolute upload URLs exactly as provided (no path rewriting)
- Add comprehensive tests for all branches (nil release, nil reader, negative size,
  absolute URLs, media type override, bad options, and upload success cases)

This behavior ensures correct handling of GitHub Enterprise upload endpoints
and aligns with the intent described in google#3849.

Fixes google#3849.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Release Asset Upload URL is generated statically

4 participants