-
Notifications
You must be signed in to change notification settings - Fork 625
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
Support Resource Bindings in UploadWorker #293
Conversation
@@ -167,15 +182,41 @@ func (api *API) ListWorkerScripts() (WorkerListResponse, error) { | |||
// UploadWorker push raw script content for your worker | |||
// | |||
// API reference: https://api.cloudflare.com/#worker-script-upload-worker | |||
func (api *API) UploadWorker(requestParams *WorkerRequestParams, data string) (WorkerScriptResponse, error) { | |||
func (api *API) UploadWorker(requestParams *WorkerRequestParams, data string, metadataParams *WorkerMetadataParams) (WorkerScriptResponse, error) { |
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.
there is no requirement to upload metadata and changing the public facing interface would require people to update their code to pass in a second nil parameter.
What do you think about updating this PR to keep this function as is and adding a new UploadWorkerWithResourceBindings
function to so we don't change API?
return api.doUploadWorker(uri, data) | ||
} | ||
|
||
// UploadNamedWorker push raw script content for your worker |
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.
Is this referred to as a NamedWorker
anywhere else?
@deuill?
res, err := api.makeRequestWithHeaders("PUT", uri, []byte(data), headers) | ||
var r WorkerScriptResponse | ||
headers.Set("Content-Type", writer.FormDataContentType()) | ||
res, err := api.makeRequestWithHeaders("PUT", uri, body.Bytes(), headers) |
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.
should we consider switching this to http.MethodPut
too to make it more consistent with line 219?
var r WorkerScriptResponse | ||
body := &bytes.Buffer{} | ||
writer := multipart.NewWriter(body) | ||
metadataPart, err := writer.CreateFormFile("metadata", "metadata.json") |
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.
we should probably not hardcode these as metadata.json
and script.js
are not the only available options
https://developers.cloudflare.com/workers/api/resource-bindings/webassembly-modules/
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Merged a similar solution in #363. Thanks @grobinson-cloudflare ! |
This PR makes it possible to bind Worker scripts to KV namespaces when uploading scripts via the API.
Types of changes
What sort of change does your code introduce/modify?
Checklist: