Skip to content
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

Should fix the R2 bucket create endpoint #1035

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

Cyb3r-Jak3
Copy link
Contributor

Description

Should fix the R2 bucket creation API request

See cloudflare/workers-sdk#1653
Fixes #1033

Has your change been tested?

Unit tests pass

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

Should wait for wrangler to be merged to confirm that this is the correct fix.

@github-actions
Copy link
Contributor

changelog detected ✅

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2022

Codecov Report

Merging #1035 (23bde0b) into master (6c5ea4a) will increase coverage by 0.88%.
The diff coverage is 68.07%.

@@            Coverage Diff             @@
##           master    #1035      +/-   ##
==========================================
+ Coverage   49.06%   49.94%   +0.88%     
==========================================
  Files         108      115       +7     
  Lines       10428    10991     +563     
==========================================
+ Hits         5116     5490     +374     
- Misses       4200     4338     +138     
- Partials     1112     1163      +51     
Impacted Files Coverage Δ
access_application.go 76.52% <ø> (ø)
access_bookmark.go 72.44% <ø> (ø)
workers_account_settings.go 35.71% <35.71%> (ø)
workers_tail.go 52.00% <52.00%> (ø)
workers_subdomain.go 57.14% <57.14%> (ø)
lockdown.go 57.69% <68.42%> (+6.94%) ⬆️
filter.go 42.96% <72.72%> (+0.51%) ⬆️
firewall_rules.go 53.98% <75.75%> (+1.39%) ⬆️
r2_bucket.go 100.00% <100.00%> (ø)
workers.go 69.42% <100.00%> (+0.71%) ⬆️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Cyb3r-Jak3
Copy link
Contributor Author

Also, do wrangler API calls count as public documentation?
There is
https://github.com/cloudflare/wrangler2/blob/7ae059b3dcdd9dce5f03110d8ff670022b8ccf02/packages/wrangler/src/r2.ts#L17-L24
which doesn't exist in the API docs

@jacobbednarz
Copy link
Member

thanks, i'll with the R2 folks internally and see what we can do here.

Also, do wrangler API calls count as public documentation?

afraid not; the service team only maintains the API/developer docs. the wrangler/workers folks are in a bit of privileged position where they usually own the APIs they are using so are across the upcoming changes.

@sodabrew
Copy link
Contributor

Was the PR you just got merged 3 days ago in #1028 already broken by an API that was removed without notice?

@Cyb3r-Jak3
Copy link
Contributor Author

The API worked when I tested it. The back ended seemed to have changed without noticed as it broke, creating buckets from wrangler as well.

@sodabrew
Copy link
Contributor

sodabrew commented Aug 11, 2022

I will raise an issue with enterprise support. Pulling a public API that is actively in use is ridiculous.

It's still in the public docs: https://api.cloudflare.com/#r2-bucket-create-bucket
image

I will note that this API was mis-documented anyways. The documentation says to POST, but wrangler2 was using PUT. As a general rule, one should not POST to a non-existent resource to create it (as the documentation says), the correct HTTP verb for this case is PUT (as wrangler2 was doing). A more common approach is to POST to the collection URL, e.g. /accounts/<accountid>/r2/buckets which is what the two fix-it PRs are doing.

I will say again, though, under no circumstances should an in-use API have been yanked from production without notice.

@jacobbednarz
Copy link
Member

this was somewhat unintentional and the change is being rolled back so this isn't needed. thanks anyway!

@sodabrew
Copy link
Contributor

Ok, that's good. But also the discrepancy I observed re: PUT vs POST is worth sorting out. The docs say POST, but wrangler2 is doing a PUT. The better verb here is indeed PUT as it implies creation. Here's a good write up of the differences: https://restfulapi.net/rest-put-vs-post/

@Cyb3r-Jak3
Copy link
Contributor Author

@jacobbednarz This should be reopened. The API is changing to POST /r2/buckets with a JSON body of {"name": "<bucket>"}.

@jacobbednarz
Copy link
Member

I'll confirm with the team as they were looking into with issues when using tokens last I checked and were backing out the change until they could dig in further.

@jacobbednarz
Copy link
Member

there is still no ETA on the POST equivalent. i'll keep my ear to ground internally and see if we can get it added when it lands instead of having a PR stagnate.

@jacobbednarz jacobbednarz reopened this Aug 22, 2022
r2_bucket_test.go Outdated Show resolved Hide resolved
r2_bucket_test.go Outdated Show resolved Hide resolved
@jacobbednarz
Copy link
Member

the documentation for fixing the POST payloads is coming in the next couple of days. we'll revive this as it will be the way forward.

@jacobbednarz jacobbednarz merged commit 033c9d6 into cloudflare:master Aug 22, 2022
@github-actions github-actions bot added this to the v0.48.0 milestone Aug 22, 2022
github-actions bot pushed a commit that referenced this pull request Aug 22, 2022
@github-actions
Copy link
Contributor

This functionality has been released in v0.48.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

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.

R2 bucket creation is broken due to API change
4 participants