-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add support for API blob upload of release attachments #29507
Conversation
Ready for review after adding a test. |
defer tests.PrintCurrentTest(t)() | ||
|
||
req := NewRequestWithBody(t, http.MethodPost, assetURL, bytes.NewReader(buff.Bytes())). | ||
AddTokenAuth(token) |
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.
AddTokenAuth(token) | |
AddTokenAuth(token) | |
SetHeader("Content-Type", "application/octet-stream") |
Just so we test what is specced.
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.
Use the required Content-Type header to provide the media type of the asset. For a list of media types, see Media Types. For example:
application/zip
That's not specced. The Github docs state that you must pass the content type of the attachment. We do not use the content type, therefore application/octet-stream
is not really needed. If we want to make that header required too (even we don't use it), I need to add such a check.
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 adding a check is better.
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.
Right, we accept any content-type except multipart, so it's fine. I suppose GitHub will error when content-type is absent, but we can do that, I see no drawback.
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.
BTW, with openapi 3.0 or higher, one could do a oneOf check on content-type
in the schema validation, but we use swagger 2.0 and it does not support that.
We should eventually migrate to ideally openapi 3.1, but it will require a new validator module.
* upstream/main: Breaking summary for template refactoring (go-gitea#29395) [skip ci] Updated translations via Crowdin Fix incorrect cookie path for AppSubURL (go-gitea#29534) gitea.service: Remove syslog.target (go-gitea#29550) Add option to set language in admin user view (go-gitea#28449) Fix elipsis button not working if the last commit loading is deferred (go-gitea#29544) Fix incorrect relative/absolute URL usages (go-gitea#29531) Add support for API blob upload of release attachments (go-gitea#29507) Fix queue worker incorrectly stopped when there are still more items in the queue (go-gitea#29532) remove util.OptionalBool and related functions (go-gitea#29513) Rename Action.GetDisplayName to GetActDisplayName (go-gitea#29540) Make PR form use toast to show error message (go-gitea#29545)
Fixes #29502
Our endpoint is not Github compatible.
https://docs.github.com/en/rest/releases/assets?apiVersion=2022-11-28#upload-a-release-asset